Tags inside folded regions

[chylex]

Describe the bug Currently search results include offsets inside folded regions, which the user either cannot see (i.e. folded imports) or jump to (i.e. rendered comments), so I don't think there's any reason to include them? If I exclude folded regions from search, it leads to better tags.

For example, here is a test where a copyright comment, import region, and several comments are folded.
Top is current behavior (2210 results across the whole file), bottom excludes folded regions from search results (840 results).

Notice that on the bottom screenshot, you have more tags like "FF", "GG", "JJ" that are easier to type.

If you don't see any reason to continue searching inside folded regions, I can commit a fix.

[breandan]

If you don't see any reason to continue searching inside folded regions, I can commit a fix.

Sure, go for it! I wonder if isn't easier to somehow assign less "desirable" tags to search results within folded regions, so that if the user expands the region whilst AceJump is active, the tags will still be available.

It would also be neat to automatically collapse regions between search results (see #255), so that sections of text without a match can be temporarily hidden to fit more tags on the screen (with some context buffer).

[chylex]

Well it wouldn't be easier but it would be possible, if you think that's better then I can go that route.

I would also hide tags inside folded regions from the tag canvas, because it seems they currently render on the starting offset of the folded region, which is misleading for users and possibly wastes time because of all the intersection checks that will fail.

[breandan]

It sounds like a cool feature, feel free to implement it however you think is most maintainable for the project long term. I generally prefer solutions that will minimize the number edge cases and are not too surprising to the user.

I'll try to give some more context where I'm coming from. Since AceJump is used for both (1) targeting and (2) discovery, it needs to balance both selection and search. On the one hand, (1) helps users jump to text they are looking at as quickly as possible, and on the other, (2) mimics the role of the "Find" tool. Sometimes we need to compromise on (2) because there are too results many to tag or the file is too large to exhaustively search, which is reasonable.

There are currently a few feature requests for improving the tag assignment algorithm (e.g., #378, #441, #402). My gut instinct is we want to improve the tag assignment algorithm to make it context-aware. Each tag location has some metadata describing its local context that can be used during assignment to match convenient tags (i.e., unigrams or easily-accessible bigrams) to likely locations (i.e., locations that the user is most likely to select).

The way I would personally implement the feature you are describing here is to annotate each tag location with an attribute val isInsideFoldedRegion: Boolean, then just skip assigning tags inside folded regions whose attribute is true, like we already do when there are "too many results".

For example of this scenario, open a largish file and type a frequently-occurring character, e.g., e, then scroll downwards. You will eventually see highlighted tag locations which are tagless (depicted below).

If you continue the query to match one of those tagless highlighted locations, they will eventually be assigned a tag:

This should match the behavior for tag locations within collapsed regions. If a user initiates a search, then pauses and chooses to expand a folded region, they will see tagless highlighted locations. Upon resuming the query, those highlighted locations will be assigned tags, just like we currently do in the "too many results" scenario.

We can imagine other modal criteria that make a tag location unsuitable for tagging, so adding such attributes to tag locations would allow us to decouple the presentation and assignment of tags. It should avoid needing to add special logic to deal with the canvas and tracking visible and logical positions, or tying us to the IntelliJ Platform which may later evolve. Does that make sense? Open to your thoughts.

[chylex]

My scope was a bit more narrow, my plan was to assign the tags in folded regions but with lowest priority, and then TagCanvas would have a set of tag offsets which are currently in folded regions, which would be updated via a fold listener, and tags in that set would not be considered for rendering. It would let someone accidentally jump to a tag that is not visible, but that's possible already for tags that are outside view.

If tags had attached metadata, such as whether they are in a folded region, there would need to be some way to update that metadata across all tags, which to me seems more complicated than effectively isolating the mutations to a single place in TagCanvas.

However, I don't immediately see an issue with implementing your solution, but rather than attaching metadata to tags, the tagger would check which search occurrences are in folded regions during tag assignment, and skip them. If you say that unassigned tags will be assigned when the search query updates, then this should work? I've been using a rather heavily modified version of AceJump so I've forgotten the details of how the original works.

[breandan]

It would let someone accidentally jump to a tag that is not visible, but that's possible already for tags that are outside view.

This was reported in #442 and fixed in f64e25a, we no longer allow jumping to off-screen tags. So if the tags are assigned to collapsed regions within the scroll boundaries, we should be consistent and take the same approach to avoid accidental selection. I think the simplest implementation would be to highlight but skip assigning tags to folded regions, but it’s your call. If the metadata is too complicated, it should be possible to check the highlights dynamically on each keystroke to see if they are within a folded section. Maybe you can submit the PR you had in mind and we can iterate from there?

[chylex]

Ah I see, in that case I will look into unifying the logic for what counts as off-screen so jumping and rendering use the same code.