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.

Sign up for GitHub

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

Jump to bottom

Fix selection in scrollback #1171

Merged
merged 4 commits into from
Mar 13, 2018
Merged

Fix selection in scrollback #1171

merged 4 commits into from
Mar 13, 2018

Conversation

chrisduerr
Copy link
Member

@chrisduerr chrisduerr commented Mar 10, 2018 •
edited
Loading

There were a few issues with selection in scrollback that were mainly
off-by-one errors. This aims at fixing these issues.

This also fixes a bug that currently exists in master where the last
cell is not selected when the mouse leaves the window to the right.

This fixes #1159 and fixes #1182.

chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Mar 10, 2018
@chrisduerr
Fix crash when selection leaves viewport
61a383b 
There was an issue where alacritty tries to convert the lines in a
selection to the on-screen lines even when the selection is not on the
screen. This results in a crash.

To prevent this from happening the selection now is not shown if it is
off the screen.

There currently still is a bug that when the selection is at the top of
the screen but still half visible, it will not show the top line as
selected but start in the second line.
This bug should be resolved with
alacritty#1171.

This fixes alacritty#1148.
@chrisduerr chrisduerr mentioned this pull request Mar 10, 2018
Merged
jwilm pushed a commit that referenced this pull request Mar 13, 2018
@chrisduerr @jwilm
Fix crash when selection leaves viewport
fce67e6 
There was an issue where alacritty tries to convert the lines in a
selection to the on-screen lines even when the selection is not on the
screen. This results in a crash.

To prevent this from happening the selection now is not shown if it is
off the screen.

There currently still is a bug that when the selection is at the top of
the screen but still half visible, it will not show the top line as
selected but start in the second line.
This bug should be resolved with
#1171.

This fixes #1148.
@jwilm
Copy link
Contributor

jwilm commented Mar 13, 2018

This has conflicts on current scrollback branch

@chrisduerr chrisduerr mentioned this pull request Mar 13, 2018
Closed
@chrisduerr
Fix selection in scrollback
a32e7a2 
There were a few issues with selection in scrollback that were mainly
off-by-one errors. This aims at fixing these issues.

This also fixes a bug that currently exists in master where the last
cell is not selected when the mouse leaves the window to the right.
@chrisduerr
Fix multi-line selection with single cell end
45742b3 
When the user selected multiple lines, dragging the selection downwards,
and then leaves the cursor to the left side of the first cell, the first
cell was still incorrectly selected. This has been fixed.

The selection also did not update if the mouse was outside of the
window, now all movement events are accpeted even when the mouse is
outside of the window. This allows updating the selection when the user
is dragging the cursor too far.

Mouse movement and click events outside of the window are not
propagated, these are only used for updating the selection.
@chrisduerr
Copy link
Member Author

I've rebased on master, fixed a tiny bug with selecting the first cell in a multi-line selection and am now propagating mouse events outside of the window to update selection.

The screen does not scroll automatically yet, but this is probably better done in a different PR.

@chrisduerr
Fix buggy selection when scrolling down
f64c57f 
When scrolling down with a selection on screen the first line was not
properly selected. This has been fixed by making sure the selection
always starts in the first cell when it is only partially visible.
@chrisduerr
Fix selection starting in first cell
d8112a0 
When selecting to the top and starting in the first cell, alacritty
would crash. These cases have been fixed and now selection should be
completely working.
@jwilm
Copy link
Contributor

jwilm commented Mar 13, 2018

Is this ready to go? You mentioned elsewhere about improving the selection clipping behavior in this PR.

@chrisduerr
Copy link
Member Author

After fixing the edge-cases for the remaining issues, the span_simple method feels like a bit of a mess to me. If all other scrollback issues are resolved, this would probably be fine, but I'd like to re-think this approach while there still are some other open issues and we have some time.

There definitely are quite a few edge-cases for this, but it might be interesting to try and rewrite this completely with these edge-cases in mind to see if it can be cleaned up. Right now it seems like it's more edge-cases than anything else really.

@jwilm
Copy link
Contributor

jwilm commented Mar 13, 2018

The span_simple is ironically named huh.. 🙃

The crux of the problem is that we need to be able to handle single cell selection without triggering a selection on every simple click. If you have ideas on how to approach that without the idea of "sides" of a cell, then that would simplify things dramatically.

@chrisduerr
Copy link
Member Author

I don't think the idea of sides is bad. Even if we wouldn't want to pass sides, we'd probably still do something like remove one from column when at the right under certain circumstances, which would be sides again, only handled in the method above it.

I think with my implementation I've got a bit confused by the SpanType. With my approach of fixing these issues it always kinda got into the way because I just wanted to go from cell XY to cellXY including everything from start to end. Properly using this might help with some edge cases.

I think by just rewriting the method from scratch without having to understand what's already there, I might be able to get a better result. Worst-case I'd probably re-implement the same thing again. It's a short method so I think it's worth exploring.

For now I'd rather keep this open and I'll get back to it this weekend or earlier when I have the time. If I didn't find any better solution until the other things are fixed, it's probably fine to merge it as-is.

@jwilm jwilm merged commit d9be2d2 into alacritty:scrollback Mar 13, 2018
chrisduerr added a commit to chrisduerr/alacritty that referenced this pull request Apr 14, 2018
@chrisduerr @jwilm
Fix crash when selection leaves viewport
1d6365a 
There was an issue where alacritty tries to convert the lines in a
selection to the on-screen lines even when the selection is not on the
screen. This results in a crash.

To prevent this from happening the selection now is not shown if it is
off the screen.

There currently still is a bug that when the selection is at the top of
the screen but still half visible, it will not show the top line as
selected but start in the second line.
This bug should be resolved with
alacritty#1171.

This fixes alacritty#1148.
jwilm pushed a commit that referenced this pull request Jun 2, 2018
@chrisduerr @jwilm
Fix crash when selection leaves viewport
231ef51 
There was an issue where alacritty tries to convert the lines in a
selection to the on-screen lines even when the selection is not on the
screen. This results in a crash.

To prevent this from happening the selection now is not shown if it is
off the screen.

There currently still is a bug that when the selection is at the top of
the screen but still half visible, it will not show the top line as
selected but start in the second line.
This bug should be resolved with
#1171.

This fixes #1148.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S - scrollback
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrisduerr @jwilm