Refactoring discussion

[chylex]

Continued from #341.

I haven't added documentation for many things yet, planning to do that at some point. EditorOffsetCache is a character position cache, I consider it to be valid for only one frame but theoretically it could be used until the user scrolls, however that would add overhead of re-checking the view for probably very little benefit (the main use comes from site ordering and by the time you press the second key, usually you exclude enough occurrences and tags it doesn't matter that you rebuild the cache).

One idea I've meant to implement for a while is a trie-based index.

I wanted to use a PATRICIA trie for tag comparisons in Map<String, _>, because it turns out computing hashes (interning tags seems to have helped a bit) and comparing strings gets expensive when you do it so many times. I've written a PATRICIA trie in Rust and C#, was considering porting them to Kotlin but not feeling up to the task while there's still work to be done on the refactor. I'd be careful about any other kinds of tries - from my experiments with several types of hand-written trie structures in Rust, PATRICIA tries were the only ones to outperform hash maps (and translating to C# gave me huge gains over a classic trie too); there's also a lot of misinformation about how exactly PATRICIA should be implemented, so that's fun.

I haven't considered tries for indexing n-grams, I don't know how worth it it'd be - the ratio of editor modifications and scrolling vastly outnumbers the amount of times I navigate via AceJump. I think with extremely long documents where you won't be able to tag everything (my refactor removes the old limit but it still ends up hitting one), a possible idea would be to not limit the amount of tags, but to limit the amount of sites before they're assigned. Even if some sites get missed, I find it difficult to imagine using AceJump in a huge file and being mad about a few missed tags when they get increasingly sparse as you move away from the cursor anyway.

[chylex]

Unfortunately I didn't think to write down everything I noticed as I went, but here's a list of things I found and fixed/changed:

Tags/highlights

• Text highlights use visual positions of characters, so they work with inlay hints, code lens, and should also work with non-monospace fonts
• Tags do the same to an extent, but due to how occupied position set works it won't work for non-monospace fonts
• Left-aligned tags render in full alpha, just darker; also removed the dark overlay for regex searches
• Regex + typing the first character of a tag would sometimes filter out legitimate tag markers - they would still work when typed, but wouldn't appear visually
• Typing the first character of a tag hides it from a marker, that's just a personal experiment + laziness about rendering, you might find it too jarring to include
• No bulk editing with fewer than 1000 highlight changes because switching to bulk edit mode takes a few ms

Jumping

• Target mode now selects single symbols that are not parts of a word
• Jump to End mode now jumps after a symbol that is not part of a word (instead of acting like normal Jump)
• Holding Shift while in Target mode includes the whole word in the selection
• Holding Shift while in Jump to End mode selects everything between the caret and the end of the jumped-to word (instead of acting like normal Jump)
• Tag selecting works by binary searching the results (has issues with target mode at the moment)
• I literally just thought of this now - holding Shift in Define mode should navigate to the type definition, because that's how default keybinds (Ctrl+B / Ctrl+Shift+B) work
• Removed scrolling actions, maybe that's something to put back but I actually assigned Page Up/Down actions to custom keybinds before I even realized AceJump could do the scrolling already

Other

• All actions are defined in plugin.xml, so no need to hack around editor actions
• Keyboard layout didn't update until restarting IDEA, that's fixed by moving all layout-related state to KeyLayoutCache that gets properly invalidated when changing settings
• ExternalUsage functions need to be reinstated, or break compatibility; since SessionManager can actually deal with multiple sessions (one per editor) at once, at minimum there need to be new ExternalUsage functions with the editor as parameter, and the old ones either removed or changed to automatically detect which editor they came from
• More fastutil collections and optimized collection use, partly to compensate for worse performance caused mostly by proper offset position calculations
• Temporarily removed pinyin support; everything uses AceUtil's Editor.immutableText extension where pinyin character mapping can be reinstated, but to avoid reconstructing the entire text all the time, I'd look at every use of immutableText and only remap pinyin wherever necessary (i.e. length calculations, new line or space checks, etc. should be fine with original text)
• Removed several tag-in-view checks in tagger, not entirely sure about the point (possibly workarounds for sites with accidental ambiguous tags if that's a problem?)
• Removed skimming, can be re-added if needed

[chylex]

As horrifying as it is, non-monospace font tags

(looks like right-side tags are a bit off, so WIP screenshot)

[chylex]

The latest version of the branch is almost "finished" (for where I wanted to get personally, not ready for PR). There are a few bugs I know of in TODOs and some documentation and refactoring is still TBD, but the functionality of all jump modes and query processing feels solid.

[chylex]

Commit chylex@feaffb3 should have the last bugs fixed, and a good enough documentation. I'd appreciate if you gave it a try, the most jarring changes will probably be the re-tagging after starting a new word in the search query, and tag navigation via Enter in crowded areas because it causes re-layouting of tags to ensure the caret tag is always visible. The latter could be pretty easily improved (see TODO in TagCanvas), the former not so easily if you want to keep my solution's 100% conflict avoidance between tags and search query.

Once you respond, I can re-add things like pinyin support and ExternalUsage functions for compatibility to make it PR-able.

EDIT: I updated the commit with a small fix.

The refactor should also solve the following issues, usually by virtue of simply not having the code that causes them anymore:

• Assertion failed: ScrollingModelImpl.removeVisibleAreaListener #338 (removed removeVisibleAreaListener call and the whole "re-tag on scroll" feature in general, unsure if this would be a problem if it was re-added into the session-based code)
• Initialization Error #336 (removed AceUtil.defaultEditor)
• CodeVision interfering with backward navigation #329 (all boundary checks are offset- and xy-based with no assumptions about lines)
• Incorrect jump location when selecting tag in line mode  #327 (needs thorough testing but regex searches now use Solver to assign all tags, so if the special treatment of regexes was the cause then it should be fixed, and I cannot repro on the example file)
• Word motion paints incorrect labels during jump to two-letter tag #310 (regex searches use Solver to create non-conflicting tags, allowing non-regex search query continuations and preventing the issue)
• Incorrect horizontal tag placement when parameter hints enabled #233 (offset- and xy-based highlights and tags work with irregular fonts and inlay hints)
• Generated tags can visually overlap #228 (tags use bounding boxes that slightly extend left and right to check for overlaps and hide tags that are too close, however there is (still?) an issue where 1-space intentations cause one letter from the tag to be rendered out of view)
• AceJump stops working after switching context #187 (SessionManager can deal with multiple editors and properly disposes of stale sessions in disposed editors)
• Destination component not connected to component tree hierarchy #147 (removed AceUtil.runLater)
• Unexpected caret move request #132 (removed AceUtil.runNow)
• Multiple-editors and split-screen related issues #71 (not sure how this bug works, but if "Search whole file" is enabled then there should no longer be any "random" visible tags missing unless it really is impossible to create a tag without conflicts)

[breandan]

Hi @chylex, this is looking great! I have been using the humongous-refactor branch for a few days and just had a few comments:

• Tagging feels much smoother. Nice work!

• Pressing Tab inserts the tab character at the cursor location and causes all proceeding tags to shift backwards. One solution here is to disable all edits to the document while AceJump is enabled. Was it necessary to remove manually activated scrolling? Calculating the center of the next group of matches was a bit messy, but something I found useful.

• Passing a word boundary now resets the tags. I think we discussed this somewhere, but I forget what the outcome was exactly. For example, if we have the following situation: a b -> a -> a[Y] b -> Space -> a [Y]b -> b -> a b[F] -> f -> a |b. Is this behavior expected? Instead, I would have expected the following behavior: a b -> a -> a[Y] b -> Space -> a [Y]b -> b -> a b[Y] -> y -> |a b. Which behavior do you think is more appropriate?

• Search-based auto-scrolling no longer works (fine), but mistyping or even typing a valid on-screen character can still cause a jump to a location off the screen. On a few occasions, I pressed a key (Y) to selecting a tag on screen (YY) and jumped to an offscreen tag. If we are going to disable scrolling, it should be impossible to select or jump to a location offscreen. If a keypress is invalid (i.e. does not lead to a string, tag, or partial tag on screen), it should be discarded.

• Partial tag selection no longer highlights the matching tag character, but changes the visible tag. Was this intentional and do you think it has an advantage? At first I found it a bit jarring, but now I have no opinion. The only minor issue that could be improved here is if the tag is on the right hand side, selecting the first character leaves a space between the site and the tag. e.g. site -> e -> site[YQ] -> y -> site [Q]. The tag should always be adjacent to the jump destination if possible. Not a big deal.

That's all I had for now, but feel free to submit a PR and we can discuss next steps there. Again, thank you for all your hard work on the refactoring and new features, really looking forward to getting this out to users.

[chylex]

Thanks for the feedback!

Tagging feels much smoother.

Glad to hear that, profiling shows it's actually slightly slower overall because of the higher precision of view bounds checking, but perhaps the fixed bulk editing optimization and reduced memory allocations are what makes it feel smoother.

Pressing Tab inserts the tab character at the cursor location and causes all proceeding tags to shift backwards.

Interesting, I'll see if there's a way to block edits completely.

Was it necessary to remove manually activated scrolling?

No, I actually think it can work more precisely with xy-based tagging now, but since I never use AceJump with whole-file search I just wanted to get the refactor done and left out features I didn't use. If you could describe the intended behavior for the scrolling and how it should work with long lines, I can reimplement it for the PR.

Passing a word boundary now resets the tags. I think we discussed this somewhere, but I forget what the outcome was exactly.

For my use, I wanted to have zero possible conflicts between tags and search. I think you were against the idea of resetting tags, but the only other way to avoid conflicts would be to avoid letters on the rest of the whole line, because heuristics - like the previous one with taking 3 extra letters past the word boundary - caused me problems basically anytime I tried to search past the boundary. It's easy to modify the behavior however you'd like though, so I'm open to changing it for the PR.

(caret position comment) Which behavior do you think is more appropriate?

I find it more natural to jump to the new position of the tag, the jump is sort of a mental reset for me. I think perhaps a better way would be to implement the "move caret to tag + search query length" idea, then you wouldn't need to worry about where you started, but I think for that feature it'd also be useful to have a new "Jump to Start" action alongside the existing "Jump to End", but I don't know if there's a nice way to do it without having 5 jump modes to scroll through.

I don't know how other people use AJ, but I find that a lot of times my brain wants to type letters first and only then thinks about which jump mode to use, which leads to a bit of awkwardness where I activate acejump, type a few letters, then change the mode, then type the tag, and I'm wondering if there's a better way to deal with that.

In either case, if you don't like word bound resets, then removing resets would also keep the caret at the original position.

mistyping or even typing a valid on-screen character can still cause a jump to a location off the screen
If we are going to disable scrolling, it should be impossible to select or jump to a location offscreen. If a keypress is invalid (i.e. does not lead to a string, tag, or partial tag on screen), it should be discarded.

I was not sure how exactly whole-file search was supposed to work, since I saw a lot of code related to view bounds checking and it was a bit confusing, since I'd expect that if you did want to only search in visible areas, you wouldn't use whole-file search. I did add automatic scrolling to the closest visible tag when there are tags off-screen and all on-screen tags become invalidated, so it should be possible to start searching for a word that's off-screen, and it should automatically jump to the closest one with the possibility to use Enter/Shift+Enter to navigate between them further.

I don't know why pressing a tag character jumped you to an off-screen tag, was it closer to the caret than the on-screen tag you were aiming for?

It would be useful to have test cases for how exactly whole-file search and scrolling should work (probably human test cases, because the unit test fixture editor has dimensions of zero which causes problems with view bounds, not sure if there's a non-hacky way around that).

Partial tag selection no longer highlights the matching tag character, but changes the visible tag. Was this intentional and do you think it has an advantage?

It was intentional, not sure if it has a user advantage, it just simplified rendering and colors a little bit (but also made it worse for non-monospace fonts I think, so in the end it's a bit of a wash). Leaving a space was intentional to avoid jumps, since you're most likely looking at the tag as you're typing it. Overall I don't know if I want to keep it like this, it works ok for me but I think there are more issues with it than with highlighting the first tag letter.

[chylex]

I think I will experiment with a different jump mode system where you type the tag and then get a selection of different jump modes with single letter shortcuts to perform them. I think it could be extremely powerful and make it possible to have special actions like "add caret" or "find usages" or "delete line" without adding endless modes, colors, and shortcuts.

Not something for this refactor, but I'll play with it and see how it feels.

[chylex]

EDIT: Experiment above is now in #350

[chylex]

The humongous-refactor branch was updated:

Prevent editing document while AceJump is active
Make jump mode cycling wrap around & add shortcut to cycle modes in reverse
Add all regex search patterns to keymap

[chylex]

I noticed that if there are too many results, narrowing the search down and assigning vacant results can cause conflicts. Not sure if that was a problem pre-refactor, but I'll fix it in the refactor branch later.