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

Allow selection to be independent from cursor. #652

Closed
wants to merge 2 commits into
base: master
from

Conversation

2 participants
@mlgh

mlgh commented Nov 16, 2016

Add endselx, endsely fields to window copy data - now we use them
instead of current cursor position.
If the cursor is moved and dragtype is not CURSORDRAG_NONE,
then corresponding selx,sely (or endselx,endsely) will be moved with cursor.
Add "stop-dragging-selection" command to copy mode.

Your Name
Allow selection to be independent from cursor.
Add endselx, endsely fields to window copy data - now we use them
instead of current cursor position.
If the cursor is moved and dragtype is not CURSORDRAG_NONE,
then corresponding selx,sely (or endselx,endsely) will be moved with cursor.
Add "stop-dragging-selection" command to copy mode.
@nicm

This comment has been minimized.

Contributor

nicm commented Nov 23, 2016

I don't really like this and I don't think it is necessary. The selection should always end where the cursor is. We don't use the cursor for anything else in copy mode, so I think that is fine.

I'm not entirely clear what problems you are trying to solve, but one issue with scrolling is probably that we move the cursor to the position of the mouse when any mouse key is pressed (right at the start of window_copy_command), probably this should only be done if it is actually needed (that is, not on a wheel press).

I think that the scroll-up/scroll-down commands should move the cursor (and thus the selection also), to make the wheel work more intuitively. But to do that we should probably just permit the cursor to be off screen.

@mlgh

This comment has been minimized.

mlgh commented Nov 24, 2016

Well, after this patch I was able to throw away other terminal emulators and switch to tmux on my desktop.
My usual workflow is: C-b, page up multiple times, then select a chunk of text I'm interested in. Then I usually pageup/pagedown a little bit more to see if the selected chunk is all I need and if all is ok, I copy it to clipboard. If I see that I've not selected something, I issue 'other-end' command and modify selection bounds.
The second use case is connected with mouse: I press mouse button to start selecting some text, then scroll up until I see where the chunk I'd like to copy ends, after I release mouse button I also press page up/page down (or scroll) a bunch of times to see if this is all I need.

Therefore I need 2 modes: page up and drag selection with cursor, page up cursor only.
Also since I'm mostly using keyboard, after I selected a chunk of text and page up'd a bunch of times I may wish to select a new chunk of text. With current pull request it's absolutely okay, since the cursor is on screen, so I just press space and start a new selection. (I don't like the idea that the cursor will be off screen because then there will be no non-frustrating way to control it from keyboard, this is more a text-editor feature, then a terminal emulator)

Also from the point of code I like the idea that at every moment we explicitly have beginning and end of selection in absolute coordinates. This makes it easier to see where selection bounds are really used.

TLDR: mouse scroll is not the only case when it would be desirable to be able to scroll independetly from selection, pageup/pagedown is also the case. basically, any command that moves a cursor may be desired not to drag the selection. It's just a nice side-effect that it fixes behaviour with mouse and scrolling.

And it would be great if we could make this binding default.
bind-key -T copy-mode-vi MouseDragEnd1Pane send -X stop-dragging-selection
The idea is simple: you want mouse wheel to drag selection? Then just don't release the mouse button! Oh, you have released it by accident? Bind 'other-end' command somewhere and fire that key. Now mouse wheel drags selection like before.
That's how it works in other terminal emulators (termite, urxvt) except that in all other terminal emulators after you accidentally release mouse button, there is no way to keep dragging selection which makes tmux 10x cooler.

Since this PR doesn't change any old behavior (just adds a command to detach selection from cursor), and I believe there many tmux users like me, who have suffered from this problem, see #642 maybe we could still put this commit into master, perhaps experimentally, and if you discover some architectural problems with this approach, just roll it back?

@nicm

This comment has been minimized.

Contributor

nicm commented Nov 24, 2016

@mlgh

This comment has been minimized.

mlgh commented Nov 24, 2016

I think it should work like xterm, where you can make a selection and
then scrolling (wheel or S-PageUp) will not move the selection (that is,
the selection can go off screen)

That's exactly how it behaves after this PR :)

but I don't think we need any new commands. At least, not at the same time

But what is the problem with new commands? If some functionality is missing from tmux, it seems like it deserves its own command.

@nicm

This comment has been minimized.

Contributor

nicm commented Nov 24, 2016

I think you are on the right track with adding endselx/endsely. I thought we could do without endselx/endsely and just allow cx/cy to be offset from the start of the history, but that would be inconvenient.

@nicm

This comment has been minimized.

Contributor

nicm commented Nov 24, 2016

If you can make it so that a drag on an existing selection extended that selection, that would be good. Requiring people to bind a key and then press it to continue the selection is not intuitive and I don't think many people will use it. We already have too many commands.

@nicm

This comment has been minimized.

Contributor

nicm commented Nov 24, 2016

OK scrap that, actually I think you may need a new command after all, I don't think you can do it without it.

@nicm

This comment has been minimized.

Contributor

nicm commented Nov 24, 2016

What do you need screen_hide_selection for?

@mlgh

This comment has been minimized.

mlgh commented Nov 24, 2016

I'd like to summarize discussed points

  1. Suppose we throw away this PR and implement a new one where we allow scroll-up/scroll-down/page-up/page-down to move cursor outside of screen.
    1a) Suppose cursor is off-screen, what should pressing "left arrow" do?
    1a.1) move the cursor and leave current viewpoint as is? that's dangerous and unintuitive, I accidentally pressed arrows a bunch of times and now my selection is messed.
    1a.2) move the cursor and move viewpoint to the cursor? (like other-end does) It seems ok, but imagine you have a large log, you select a piece of it, than scroll up long way, and then accidentally press arrow - now you are back at selection. That's frustrating.
    1b) Suppose cursor is off-screen and viewpoint is somewhere else. You see a chunk of text and you want to select it. How would you teleport the cursor to viewpoint?
  2. Suppose we leave current PR as is and don't add any new default keybindings. In that case default behavior is not changed. Whoever wants the new behavior, will find it in man page or some tmux tutorials e.g. bind-key -T copy-mode-vi MouseDragEnd1Pane send -X stop-dragging-selection and bind-key -T copy-mode-vi <somekey> send -X other-end and they will be responsible for how they utilize them, so no one will be frustrated. Perhaps this is not cool since no-one will hear about this feature.
  3. Leave the current PR as is and add default key-bindings. In this case after you release mouse, you will have to do something to continue dragging selection. From the top of the head I don't see any obvious candidate that will do ok.
    3a) Left mouse button continues selection if it exists instead of starting new one. This is frustrating, this will require you to clear-selection every time you need to select something else.
    3b) Just don't release left mouse button when selecting something (like it's done in GUI terminal emulators). If you release it by accident, you will have to issue something like other-end in order to keep dragging selection. In case we do this, everyone will know about this feature and in case they don't like it, they will bind MouseDrag1End to whatever they like. This will frustrate some people, but it will be temporary since whoever doesn't like this feature will be able to turn it off. Perhaps we could add it to man page
@nicm

This comment has been minimized.

Contributor

nicm commented Nov 24, 2016

We are not going to change the default behaviour, you will always need to change something to make finishing a selection not copy.

@nicm

This comment has been minimized.

Contributor

nicm commented Nov 24, 2016

I somehow managed to get it so that I had a selection but it was invisible and now I can't reproduce :-/.

Also it looks like other-end is full of redraw bugs, but I think they are existing problems.

@mlgh

This comment has been minimized.

mlgh commented Nov 24, 2016

What do you need screen_hide_selection for?

I introduce hidden member in struct screen_sel for the following purpose:
Whenever the selection is off screen, we need a way to hide it, since screen_sel->sx screen_sel->sy, screen_sel->ex, screen_sel->ey will select some zone on screen. Since there is nothing to select, there are 2 ways to disable selection

  1. turn screen_sel->lineflag to LINE_SEL_NONE. In this case it would forget the previous lineflag value.
  2. turn screen_sel->hidden to 1. This means that somewhere there is selection, but it's just off screen. Whenever window_copy_update_selection will see that it's on screen, it will turn hidden to 0 and selection will be visible again.
@mlgh

This comment has been minimized.

mlgh commented Nov 24, 2016

We are not going to change the default behaviour, you will always need to change something to make finishing a selection not copy.

Got it. So it could still land up in man page and online tutorials.

I somehow managed to get it so that I had a selection but it was invisible and now I can't reproduce :-/.

Are you sure that you haven't lost it? 'other-end' should bring you back to selection.
Maybe you clicked somewhere in the right upper end and it was hidden under the [100/4000] scroll position bar?

Also it looks like other-end is full of redraw bugs, but I think they are existing problems.

Hmm, before this PR I indeed saw lots of redraw bugs in other-end, but how would you trigger them now? If selection fits the current screen, it will just move the cursor. If it doesn't it will redraw the full screen.
The only redraw bug I know is when you scroll with a wheel up and down while dragging the selection and move mouse quickly, then some parts of text will not be redrawn and look torn.

@nicm

This comment has been minimized.

Contributor

nicm commented Nov 24, 2016

Ah yes, it clears it via screen_set_selection.

I have applied this now, with some cleanup; I also renamed the command to stop-selection. It will be in GitHub next time it syncs from the OpenBD repo.

Thanks!

@nicm nicm closed this Nov 24, 2016

@mlgh

This comment has been minimized.

mlgh commented Nov 24, 2016

Awesome! Thanks a lot!

@nicm

This comment has been minimized.

Contributor

nicm commented Nov 24, 2016

I can trigger redraw bugs with other-end and the mouse wheel pretty easily but I am pretty sure they are existing problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment