Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search boundaries and start directly in word search mode #214

Merged
merged 7 commits into from
Mar 10, 2018

Conversation

svensp
Copy link
Contributor

@svensp svensp commented Mar 6, 2018

My attempt to implement #210

Adds an interface to manage search boundries by setting different implementations.
Includes implementations:

  • complete file
  • screen
  • screen start to cursor
  • cursor to screen end

Adds new Actions:

  • find word on screen
  • find word from screen start to cursor
  • find word from cursor to screen end

When testing it with ./gradlew runIde -Project=$(pwd) the hints are only drawn after I type a key but that also happens when I use the AceLineAction from master so I'm not sure if that is a bug in my code. Either way I haven't found a way around that.

@svensp svensp changed the title Wordsearch Search boundries and start directly in word search mode Mar 6, 2018
@breandan breandan changed the title Search boundries and start directly in word search mode Search boundaries and start directly in word search mode Mar 6, 2018
@helmus
Copy link

helmus commented Mar 6, 2018

Happy to pay out this bounty sources for this pr if it get's released and solves the latency issue!
https://www.bountysource.com/issues/46543096-acejump-lagging-on-large-files

@svensp
Copy link
Contributor Author

svensp commented Mar 8, 2018

This PR doesn't really improve general performance while searching the whole file.

It does allow easy switching between searching the whole file and only on the screen thought. And the new commands will be fast because they only search on screen. If you want the basic AceJump functionality but only searching on screen then how about adding a new Action which triggers the normal AceJump in screen mode? Bind the screen mode AceJump to one key and file mode AceJump to another - hit the key that fits your current need instead of going to the settings and (un-)toggle screen mode.

That would be pretty easy to add

@helmus
Copy link

helmus commented Mar 8, 2018

Thanks again, I appreciate your time on this. The initial performance degradation was introduced by searching the whole file instead of just the screen. if that option could be restored it fixes the issue 100%. I wouldn't spend any time on optimizing the whole file search, I can't think of a single rational for it, it just doesn't apply to how this feature works ( adding visual markers on the screen )

@breandan
Copy link
Collaborator

breandan commented Mar 8, 2018

I think the best solution is the one @svensp proposes: to break up AceJump screen modes into separate actions which users can bind and use however they wish. Some users have expressed support for the new search functionality, while a vocal minority have opposed it, mostly due to perceived latency introduced when editing large files.

Interestingly, the latency is not introduced by searching the file (which happens instantly just like Ctrl+F), but assigning available tags to search results, which needs to be recomputed on every keystroke. With further optimization I am confident we can eliminate #161, so the two modes are seamlessly integrated.

Getting back to the PR, you mentioned:

the hints are only drawn after I type a key but that also happens when I use the AceLineAction

After using AceLineAction, you need to press another key? If so, this is definitely broken. However I cannot reproduce this on the latest version. Is the goal of this PR to tag every word on screen (before and/or after the caret), or just words matching a search string? The way most AceJumps are implemented is the former (ie. there is no search, only tagging, or a single character search, followed by tag and select).

By comparison, this AceJump supports either a single character search, or a multi-character search, followed by tag and select. The difference between these two modes is invisible to the user (current latency issues notwithstanding). A user could type a single character, followed by a tag, or multiple consecutive characters, followed by a tag. The trick to this feature is, we must never assign a tag somewhere that could be misinterpreted as a string elsewhere in the document.

Back to the performance solution here. As I see it, there are two users of AceJump. Those who want to search, and those who don't care about search. We should allow both groups to use AceJump in a way that makes them most productive. The reason I like this PR as it is implemented, is that it tags words immediately when you invoke the AceWordAction. We are trying to minimize the round trip latency between when a user thinks they want to jump to a location, and jumping to that location. This process should take as few keystrokes as possible.

@svensp
Copy link
Contributor Author

svensp commented Mar 8, 2018

The goal of this PR is indeed to tag all words visible on the screen, before and/or after the caret(depending on the action), thus eliminating the 'type target' stage.

I do believe the extra key necessary for the tags to appear is somehow related to building it on my machine so if it does not happen when you try it that's totally fine with me.

So for the performance issue the actual problem is that it gets regenerated on every key stroke, so Tags are generated as often as the word(or part of the word) that is typed?
I don't know how easy this is to implement in kotlin but I believe usually you'd start the tagging as asynchronous process with a very small delay(~0.2 seconds) and cancel the previously active timer.
This means the tagging is only done once the user has finished typing the word instead of for every key stroke - as long as the next key is pressed before the small delay has passed. Supporting varying typing speeds would be as easy as setting a sensible default delay but allowing to customize it on the settings page

@breandan breandan merged commit 1e11cd3 into acejump:master Mar 10, 2018
@breandan
Copy link
Collaborator

breandan commented Mar 10, 2018

usually you'd start the tagging as asynchronous process with a very small delay(~0.2 seconds) and cancel the previously active timer

Yep, this is exactly how we have implemented it, but it's not a perfect solution. The delay itself introduces additional latency. We could have this be a user configurable parameter, but it adds unnecessary complexity IMO. Note that it is still possible to continue searching once the tags have been assigned (since we assign tags very carefully).

I suspect most users concerned with latency are not aware you can type multiple search characters, so they just press a single character and wait for tags to appear. In fact, it would be faster to type two search characters and one tag character to jump, than one search character and two tag characters. This is why I'm happy with the solution you came up with - there is either search, or no search. Without search, you can only jump to characters on the screen, but will need at most two characters. Thanks, Sven!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants