WIP: Partial highlighting

[harshad1]

This PR adds partial highlighting support discussed in https://github.com/gsantner/markor/discussions/1749

1. When text is changed we recompute the spans for the whole text
2. When we scroll, we apply the highlighting for the visible region +- height

This works rn but lots of work remains to be done

• [x] Documentation / docstrings
• [x] Several spans were disabled - these need to be fixed and re-enabled
• [ ] Lots and lots of testing (Will need help testing Zim, especially, as I don't use the zim format)
• [ ] I removed all the profiling code to reduce clutter. Need to make a call and all it all back.

[harshad1]

@gsantner given the scope of this change, I think it may be best to hold off until a future release - I will want to use it daily for several weeks before I am happy.

[harshad1]

Editing / scrolling in the main activity is significantly slower than in the document activity. I think this may be related to usage of RelativeLayout - keeps requiring redraws. We should look at alternative ways to getting the same UI.

https://www.androidcode.ninja/solved-android-edittext-lag/

[harshad1]

@gsantner On my device, I can now edit a 700kb file with highlighting with no trouble.

[gsantner]

RelativeLayout slow

Suggesting ConstraintLayout, or check something else if it has the same performance hit like RelativeLayout.
Though I suggest to let (layout related) changes out of this pull request.

[harshad1]

@gsantner Yes. I will make a layout change in a separate PR, if it still seems necessary after I am done with this pr.

[gsantner]

I can check as well

[harshad1]

Quick update: this effort may not work out after all.

The issues I can't seem to get around satisfactorily are:

1. Adding / removing spans from a SpannableStringBuilder is very slow. On a file with many spans per line (especially todo.txt) scrolling becomes jerky
   • 150ms to add 1000 spans. Each line in a todo.txt file can have 10+ spans.
2. Adding / removing metric affecting spans causes the edit text to jump to the cursor. I have some workarounds, but these are janky,
3. Scrolling with metric affecting spans sometimes causes rendering issues.

On the bright side, this works amazingly well with a very large file with just 1 - 2 spans per line.

[gsantner]

interesting issue I found on androidX migration - it can happen that a final initalized member is still null, while it's explicitley set a value in at top. This is because in this case some stuff in constructor already does some calls, while the object isn't fully ready yet, but calling derived methods.

In this case, overridden method addTextChangedListener method was crashing :smile: .

Pushed a guard for that on master

[harshad1]

Lol I noticed this last night. I just pushed changes to nuke the activelisteners system and to just use a flag :) I will resolve the differences with master.

[gsantner]

Oh ok, thought it's somehow related to my androidx changes and the behaviour there

[gsantner]

One thing you wanted to know is whether androidX brings notable general performance improvement on Editor.

I migrated to androidX and updated all required libs/code on the androidsdk-upgrade branch. You can try that out, or merge both (+partial) together in some new local branch to try it combined :D.

[harshad1]

I can check as well

Please let me know if you are able to reduce the layout nesting (I guess using constraintlayout). The bottleneck preventing this pr from working fully is DynamicLayout reflow being slow. I think reducing the layout complexity may help.

[gsantner]

Which ones are the important ones? From above I read it's mainly at MainActivity (which is bad, but enough when it works fast at opened document window).

Or is it also DocumentActivity/DocumentFragment?

And yes, RelativeLayout is pretty popular used here :D.

[harshad1]

I think we need to experiment with both (main activity should definately move away from relativelayout though) . I'll clean up the PR tonight so it is in a stable state. Then I will try it with alternative Layouts and see if the performance improves to an acceptable level.

Jul. 23, 2022 10:22:08 Gregor Santner @.***>:

Which ones are the important ones? From above I read it's mainly at MainActivity (which is bad, but enough when it works fast at opened document window).

Or is it also DocumentActivity/DocumentFragment?

— Reply to this email directly, view it on GitHub[https://github.com/gsantner/markor/pull/1757#issuecomment-1193159095], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAOZ3TG7ZGNPXAXLHSIVADDVVQS37ANCNFSM53IPWORQ]. You are receiving this because you were assigned.[Tracking image][https://github.com/notifications/beacon/AAOZ3TGAWPFPB7EUQENGW4LVVQS37A5CNFSM53IPWOR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOI4PCTNY.gif]

[gsantner]

merged some layout improvements, they are on master branch

[gsantner]

What I disabled by default at some point was the "bigger headings" feature, because its resource intensive.

With partial highliting, do you think its safe to enable it by default again?

[harshad1]

I am still not sure partial highlighting will actually work. Still trying to solve some performance and tearing issues.

If it does work then the bigger headings can probably be default - enabled again. I will test this.

Jul. 26, 2022 16:22:13 Gregor Santner @.***>:

What I disabled by default at some point was the "bigger headings" feature, because its resource intensive.

With partial highliting, do you think its safe to enable it by default again?

— Reply to this email directly, view it on GitHub[https://github.com/gsantner/markor/pull/1757#issuecomment-1196078306], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAOZ3TGFG6G2DT3AGJCVVYLVWBXKJANCNFSM53IPWORQ]. You are receiving this because you were assigned.[Tracking image][https://github.com/notifications/beacon/AAOZ3TCYWVLAOZQVKPXWDN3VWBXKJA5CNFSM53IPWOR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOI5FLJYQ.gif]

[gsantner]

Either way, thank you for the efforts

[gsantner]

were my few changes helpful? I guess it didn't make a dramatic difference

[harshad1]

were my few changes helpful? I guess it didn't make a dramatic difference

I think so. 10% or so, especially in main activity. Kinda hard to tell as I am making a large number of changes. If they are not disrupting anything else, I think we should keep them.

[harshad1]

Yeah. I tried separating static and dynamic spans at first, but it was too slow. Will try again now that we've improved the performance.

Jul. 30, 2022 02:10:42 Gregor Santner @.***>:

@.**** commented on this pull request.

---

In app/src/main/java/net/gsantner/markor/ui/hleditor/Highlighter.java[https://github.com/gsantner/markor/pull/1757#discussion_r933776642]:

 }

-}

• protected final void createUnderlineHexColorsSpans() {

•    createColoredUnderlineSpanForMatches(HEX_CODE_UNDERLINE_PATTERN, m -> new ColorUnderlineSpan(Color.parseColor(m.group(1)), 3f), 1);
  

• }
• // We do not implement UpdateLayout or Parcelable for performance reasons
• public static class HighlightSpan extends CharacterStyle implements UpdateAppearance, Callback.r1<Object, Matcher> {

when you dont get far this way, an alternative is to exclude the dividers from the range/screen-height stuff

— Reply to this email directly, view it on GitHub[https://github.com/gsantner/markor/pull/1757#discussion_r933776642], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAOZ3TFW25QUGKT25E7IPPDVWTWRBANCNFSM53IPWORQ]. You are receiving this because you were assigned.[Tracking image][https://github.com/notifications/beacon/AAOZ3TC7QSATFFGAKFJX6D3VWTWRBA5CNFSM53IPWOR2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOH32SMSI.gif]

[harshad1]

Ok. I think I have solved both the tearing and the jump issues - complexity is getting high though :)

Now this requires much more testing and some more perf improvements before we can merge.

[harshad1]

@gsantner This is now done.

What is left is:

• [ ] Add profiling code back if really needed
• [ ] Cleanups
• [ ] Documentation / comments
• [ ] More testing, especially on Zim (which I don't use)

I added an option to disable dynamic highlighting to settings (default enabled)

[gsantner]

@harshad1
So Google Play gave green light with Markor file access now, with the 2.10.1 release, i.e. it passed review --- with the androidsdk-upgrade branch containing AndroidX.
As consequence I merged that merge request so everything is one code base. (-> some merge conflicts related to androidx renamings in your PR)

In case you encounter issues, please let me know. I just got a mail today saying the new update is very bad and it worked better before...but without saying what is wrong...so I don't know whats up 😄 .

[harshad1]

@gsantner Aside from the issues with PendingIntent noted above we need to add a dialog to request all file permissions.

If I upgrade / install over an earlier release I don't get a request for all file access and I just see empty folders with all my files missing. Had to go into app permissions and manually give it all file access

[gsantner]

also webview fileacess issue (images/font) is what I heard today.

regarding permission, manage file permissions should be asked as soon you click the red new file button, or open a file...does it not?

[harshad1]

regarding permission, manage file permissions should be asked as soon you click the red new file button, or open a file...does it not?

I get the permission request when I click the new button. But it should check this on startup every time, I think. Otherwise I can see the folder structure, but no files.

[gsantner]

added more permission prompts, and webview file access - pushed to master
+ pending intent changes

I'm seeking towards a short hotfix release in a hour or so, though not tagged/only version code increase - meaning F-Droid users will keep non-androidx for this one.

[harshad1]

Iirc I factored those calls out and reduced the number but didn't remove all of them. We may have to revisit this.

We may also have to revisit temp files etc to mitigate android's shitty FS.

Right now though I am using partial highlighting + androidx and everything is working fine

Aug. 11, 2022 11:41:23 Gregor Santner @.***>:

wasnt there also some merge request which also removed many if (checkpermission) { fileaction } condition guards?

— Reply to this email directly, view it on GitHub[https://github.com/gsantner/markor/pull/1757#issuecomment-1212358387], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAOZ3TEFZPXR7OKEDPHOP2DVYVCNDANCNFSM53IPWORQ]. You are receiving this because you were mentioned.[Tracking image][https://github.com/notifications/beacon/AAOZ3TEYVHPMIR66KKI3PKTVYVCNDA5CNFSM53IPWOR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOJBBR54Y.gif]

[gsantner]

I did some great advancements/achievements with newer android file handling, like opening files from other apps without direct file access.

What I added is kind of a proxy file which automatically writes back/forth to/from the contentresolver Uri stuff....while still everything is java.io.File based.
That of course is for one file, but thats the main use, to open some specific file from some app. So nothing for file browser, but still great addition for Interoptability with other apps.

See also my comment in the discussion thread. If you want, feel free to give it a try as well.