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
EXPERIMENTAL visual mode from selection #519
base: master
Are you sure you want to change the base?
EXPERIMENTAL visual mode from selection #519
Conversation
As mentioned in #511, I've created a build of this branch and put it on an "experiment" update site: I'm concerned with the fact that @albertdev keeps finding little issues that this introduces (the textWidget vs. ITextViewer issue, the |
Does my solution differ a lot from what you had in mind? I did introduce an option to toggle it as a sort of escape-hatch, so if we change the default to "disabled" it could in theory be included in the unstable build without harm. I only used it for about a day back in July, and it's then that I noticed those small things. Maybe I should try it again... |
No, your solution doesn't differ at all from what I had in mind. In fact, you took it one step further and made it configurable. But I wasn't comfortable with the solution in the first place, it isn't your implementation that concerned me. This might just be the way I use Vrapper, but to me, this fixes something I never use at the risk of destabilizing features I do use. That's why I don't want a "half-way" solution. I don't want to destabilize |
I am going to try the build from experiment site and see if the added issues are going to be a bigger deal than the missing sync. |
I did some limited testing today and it worked very well for renames and Next (
and use quickfix to create this method. You should then be able to enter a new name for it by pressing I can also confirm that after Please note that I am out most of the rest of the week, so may not test it much more than this for now. Thanks to @albertdev and @keforbes for getting the patch this far. |
I hit a weird bug that is most likely the side effect of this patch. If you use "Extract to local variable" followed by |
If you disable the fix by setting |
@keforbes Sorry about the late response, but I can confirm that the issue goes away with |
@albertdev, what do you think about this Pull Request? Do you think it's still worth merging in? |
I've been thinking about it, but it's still not ready for stable. In the mean time, I did think about implementing select mode. Basically, it behaves like Notepad or Eclipse use their selections: you can extend using Shift + Left / Right, pressing an alphanumeric key replaces the selection with that key and pressing For Vrapper, it's a mode very like Insert mode (i.e. we let pretty much any character pass straight to the editor). For Eclipse, this is just its native selection mode. |
I'm going to close this pull request... I'm not any closer to fixing this prototype. The idea about select mode can better be moved to another issue should there be interest, though I guess it will just confuse users when they try to invoke a command and replace the selection with that character instead. I have been thinking about doing something with Command Listeners though, it might be possible to recognize that a command id is on a white-list after which we reset Vrapper's selection state. |
Eclipse has a few tricks of its own to make a selection: Expand selection to left element, to right element, to enclosing element or Restore previous selection. These are done using the menu or keyboard, so the SelectionVisualHandler needs to differentiate between selections from Vrapper and those of Eclipse.
The EclipseHistoryService needed to be changed because it would clear the selection without triggering the SelectionVisualHandler, and that would keep Vrapper stuck in Visual mode even when there was no selection.
This behaves exactly like Eclipse's selection: any movement clears the selection and moves back to plain insert, any alphanumeric characters replace the selection with that character. Extending with Shift+Movement works as usual but starting Select mode is not yet implemented. For now it can only be entered when 'visualother' is set and a selection is started in Insert mode.
I've been working on this once more after @mmbradle told me in #674 that you could actually swap between normal and visual mode without much fuss. I fixed @haridsv's problem with the The other problem @haridsv reported with What this prototype doesn't yet do:
It does give hope that we could finally integrate Eclipse's selections with Vrapper. |
780cc17
to
721a817
Compare
imo Vrapper should always go to visual mode when a piece of text is selected. I tried Vrapper with this branch and it was really counter intuitive to be inside select mode, that I would normally go into only by configuring 'selectmode' option or 'g_h' from normal mode. I tried to change SelectionVisualHandler so it would only enter visual mode, but noticed the problem:
as a result, I got the renamed variable fully selected but Vrapper in normal mode. I think we can get a partial solution with Command Listeners as @albertdev commented. Will give it a try in https://github.com/pedrosans/vrapper/tree/visual-mode-sync and share the results next. |
@pedrosans You can now find it at https://github.com/albertdev/vrapper/commits/EXPERIMENTAL-eclipse-motion-textobjects - it is currently based on the old experimental code in this Pull Request so that I could compare the It's out of date and might break in interesting places, but it did include a configuration mechanism through which Vrapper plugins could register extra commands (e.g. a Python plugin registering python-only commands). |
@pedrosans |
As requested in issue #511, a setting
visualother
/vo
which enables syncing an Eclipse selection with Vrapper's visual mode.I noticed that it immediately went wrong if you would run something like
dd
u
. The reason is because the following happens:It turns out that if you clear the selection of the textwidget instead of the ITextViewer, no selection change listeners on the ITextViewer will be called. Result: the selection is empty but Vrapper still thinks it should be in Visual mode.
I worked around that with the second commit.
EDIT: It might still do weird things when renaming stuff, or using
Ctrl + Space
for completing commands. Eclipse tends to select stuff at those times, rapidly switching modes and leaving Vrapper in a bad state.