Created attachment 311669 [details] [review]
Demo1

Here's a first demo.

There's still *a lot* to do, but this patch lets us evaluate whether we'd like to have this user experience or not. Feedback is welcome :)

Hmm, ok, feedback :-)

I think having the feature is fine, and probably even required for good touch usability.

However, I don't like the approach the patch takes: you're introducing _more_ view dependency (pixels, char height) into the view-independent parts (VteScreen, etc).

I had rather thought that sub-row scrollability would be view-only, ie only use the fractional part of the adjustment when deciding what and where to paint/invalidate, and when translating gdk event coords to grid coords.

Doing so would keep this out of the model (which is important for the eventual model-view split) and even be simpler, I think.

(In reply to Christian Persch from comment #2)

> However, I don't like the approach the patch takes: you're introducing
> _more_ view dependency (pixels, char height) into the view-independent parts
> (VteScreen, etc).

scroll_delta is already part of VteScreen, so it's really not view-independent. Of course it'd be nice to have this split, and I also didn't like introducing char_height there.

Actually the first internal version of the patch went with a separate scroll_delta_denominator in VteScreen, but it eventually had the same value than char_height and since char_height was available everywhere it was simple to search-n-replace to the already existing variable.

Another approach could be to go with floats (1.0 standing for 1 char height), but I'm really afraid with the possible cumulative problems arising from not being able to represent exactly 1 pixel.

> I had rather thought that sub-row scrollability would be view-only

I really wouldn't want to separate whole-row scrollability from the sub-row one. I also had the idea to keep scroll_delta containing rows and introduce another variable for sub-row pixels. So far this would only be a little bit more cumbersome than the current approach. But moving these two to separate classes sounds like nightmare.

Long story short: I don't like the current approach too much (just as you don't like it), but don't quite like the other possibilities either.

I guess I was afraid of floats because I had the wrong approach in my mind.

I wanted scroll_delta and related variables to contain the exact pixel offset, e.g. in case of floats to contain "exactly" the value of let's say 1/13. And then you add this up 13 times and you won't get 1.0. (But this isn't an issue since there's no (and won't be a) user action to scroll by 1 pixel, and even if there was, it'd be easy to workaround.)

Instead, we can make scroll_delta contain any double value and not do any rounding at all, and keep its current semantics (1.0 standing for 1 character row). Rounding to integer pixels would be done by the displaying part, doing (long)(scroll_delta * char_height).

Moreover, we'd no longer need mouse_smooth_scroll_delta that cumulates the tiny scroll amounts until it becomes actionable; instead it could be stored in scroll_delta right away.

What's a bit tricky and I can't see what's the right place to handle it is to decide if a scrolling event actually resulted in the scroll position changing, so that we don't repaint the UI if it wasn't actually scrolled away.

Does this approach make sense?

Bug 738956 (the lack of duplicate reports) suggests a double is large enough to contain the values with the precision that we'll need, without noticeable degradation of smoothness over time.

(In reply to Egmont Koblinger from comment #3)
> scroll_delta is already part of VteScreen, so it's really not
> view-independent.

Yes, but let's not make it worse :-)

> > I had rather thought that sub-row scrollability would be view-only
> 
> I really wouldn't want to separate whole-row scrollability from the sub-row
> one. I also had the idea to keep scroll_delta containing rows and introduce
> another variable for sub-row pixels. So far this would only be a little bit
> more cumbersome than the current approach. But moving these two to separate
> classes sounds like nightmare.

Hmm maybe I wasn't clear with what I meant. I meant that the view simply kept a 'pixel delta' to offset the row-based view (with values 0..char_height-1).

(In reply to Egmont Koblinger from comment #4)
> Instead, we can make scroll_delta contain any double value and not do any
> rounding at all, and keep its current semantics (1.0 standing for 1
> character row). Rounding to integer pixels would be done by the displaying
> part, doing (long)(scroll_delta * char_height).

Yes, I think I like this approach better. (And simply replacing long->double in the model at least doesn't make the view dependncy worse than it is.)
 
> Moreover, we'd no longer need mouse_smooth_scroll_delta that cumulates the
> tiny scroll amounts until it becomes actionable; instead it could be stored
> in scroll_delta right away.

Yes.
 
> What's a bit tricky and I can't see what's the right place to handle it is
> to decide if a scrolling event actually resulted in the scroll position
> changing, so that we don't repaint the UI if it wasn't actually scrolled
> away.

I think that's simply replacing abs(dy * char_height) >= 1.0 instead of the current != 0 check in vte_terminal_handle_scroll()?

> Does this approach make sense?

I think it does, yes :-)

> Bug 738956 (the lack of duplicate reports) suggests a double is large enough
> to contain the values with the precision that we'll need, without noticeable
> degradation of smoothness over time.

Right, the double's 53 bits of precision should be quite enough.

> I think that's simply replacing abs(dy * char_height) >= 1.0 instead of the
> current != 0 check in vte_terminal_handle_scroll()?

Hmm actually that might not work for successive sub-row scrolls that are individually below the threshold but cumulative above it.

But a comparision of the deltas in pixel space ({old,new}_value * char_heigth) should do it.

(In reply to Christian Persch from comment #5)

> > I really wouldn't want to separate whole-row scrollability from the sub-row
> > one. I also had the idea to keep scroll_delta containing rows and introduce
> > another variable for sub-row pixels. So far this would only be a little bit
> > more cumbersome than the current approach. But moving these two to separate
> > classes sounds like nightmare.
> 
> Hmm maybe I wasn't clear with what I meant. I meant that the view simply
> kept a 'pixel delta' to offset the row-based view (with values
> 0..char_height-1).

I think we're mostly on the same page. It's possible to have two values: a "major" (the integer part, exactly the current scroll_delta) and a "minor" one ranging 0..char_height-1. But it makes doing maths unnecessarily more cumbersome.

What I'm not sure if I understand correctly from your words is whether you'd store these two values next to each other (e.g. both in VteScreen), or somewhere separated from each other (like the "major" belonding to the model, the "minor" belonging to the view). I'd definitely vote "no" for the latter.

> > Does this approach make sense?
> 
> I think it does, yes :-)

Cool, I'll go with this one then, and we'll see how it ends up.

One more thing to think about... I have already formed an opinion which I'm not sharing right now because I have to run :) so I'd let you think about it if you wish.

What to do with the padding?

It's 1px by default, but can be larger. Do we want to paint there? What and how exactly?

And what to do with the extra padding at the bottom if the window size is not grid-aligned?

(In reply to Egmont Koblinger from comment #7)
> I think we're mostly on the same page. It's possible to have two values: a
> "major" (the integer part, exactly the current scroll_delta) and a "minor"
> one ranging 0..char_height-1. But it makes doing maths unnecessarily more
> cumbersome.
> 
> What I'm not sure if I understand correctly from your words is whether you'd
> store these two values next to each other (e.g. both in VteScreen), or
> somewhere separated from each other (like the "major" belonding to the
> model, the "minor" belonging to the view). I'd definitely vote "no" for the
> latter.

I'd have stored them separately (pixel delta in the view), but with the "double" approach, it's unnecessary now.

(In reply to Egmont Koblinger from comment #8)
> One more thing to think about... I have already formed an opinion which I'm
> not sharing right now because I have to run :) so I'd let you think about it
> if you wish.
> 
> What to do with the padding?
> 
> It's 1px by default, but can be larger. Do we want to paint there? What and
> how exactly?

Padding is outside the drawable area (that's its entire raison d'être), so no painting there :-)
 
> And what to do with the extra padding at the bottom if the window size is
> not grid-aligned?

I'd say that in the case of the height not being an integer multiple of the char height (which can happen in the maximised and fullscreen cases, or in apps that don't set the geometry hints), the view should be flush to the _bottom_ of the allocated area, which means that at the _top_ there is space for part of one row available, and it should be filled with that row's content, if it has any, i.e. if there is scrollback available. So on the alternate screen, there is nothing there. (Hmm… that means the extra empty space on no-scrollback-available is now at the top instead of at the bottom, but I don't think that matters too much.)

(In reply to Christian Persch from comment #9)

> Padding is outside the drawable area (that's its entire raison d'être), so
> no painting there :-)

I had the same conclusion, with different reasoning. Normally (when scrolled to the bottom in a grid-aligned window) I'd like to keep the current look of g-t, and definitely don't want to see the bottom pixel row of the previous line of text at the top of my window. It'd just look buggy and irritate me. And having a selective logic (only paint there if [whatever condition]) would result in a weird scrolling experience.

Note that we currently have an exception: the outline cursor of non-focused windows. We should keep that exception; as the outlined cursor scrolls out, it should be able to paint in the innermost 1px of the padding.

> > And what to do with the extra padding at the bottom if the window size is
> > not grid-aligned?
> 
> I'd say that in the case of the height not being an integer multiple of the
> char height (which can happen in the maximised and fullscreen cases, or in
> apps that don't set the geometry hints), the view should be flush to the
> _bottom_ of the allocated area, which means that at the _top_ there is space
> for part of one row available, and it should be filled with that row's
> content, if it has any, i.e. if there is scrollback available. So on the
> alternate screen, there is nothing there. (Hmm… that means the extra empty
> space on no-scrollback-available is now at the top instead of at the bottom,
> but I don't think that matters too much.)

This was my first approach, but it'd introduce an undesired behavior with fullscreen non-alternate screen apps. You'd see a bit of irrelevant stuff at the top of the window that does not belong to the app, and hence is misleading. So I wouldn't do this.

A trivial approach is still not to paint in gap, in which case it's irrelevant if the gap is at the top or the bottom.

Another approach that I think could work is to leave the extra area at the bottom. With the scrollbar being at its default position, we wouldn't paint anything there. But as you start scrolling, it could show the text that's being scrolled downwards.

Created attachment 311698 [details] [review]
Demo2

The float approach...

Created attachment 311715 [details] [review]
Demo3

Getting better...

Works nicely in the latest version:

- Painting the actual terminal area correctly

- Highlighting text with mouse

- Nice experience on resize or font size change

Current bugs I'm aware of:

- It does draw in the padding area. I guess we need a cairo_region_intersect_rectangle() at the right place; I just failed to find what that right place would be in a couple of attempts.

- Dingu regex matches are off. That code uses row offsets relative to the viewport rather than to the beginning of time (as scroll_delta does), which is a concept we'll no longer have, so I first need to refactor the code to think in scroll_delta-like values.

- Forward search for a string might find a match in the half-displayed bottom line and then it doesn't snap it to be fully visible. Should be trivial to fix.

Created attachment 311720 [details] [review]
Demo4

Dingu and search are fixed. Painting in the padding is still an issue.

After drawing the background in the padding region, you can clip the cairo context to the content area; that should fix it.

Created attachment 311721 [details] [review]
Demo5

Dingu really fixed this time.

The public API method vte_terminal_match_check() doesn't make too much sense anymore, since it takes cell coordinates relative to the current viewport. I guess we should deprecate it...?

Similar: there are a11y events about scrolling (and about text getting deleted/added), we'd need to figure out what to do with them.

Yes, we should deprecate that. There's code all over vte users that do match_check(x / char_width, y / char_height), will will now be totally wrong. Maybe even fixing this function to always return 'no match' should be considered.
And then vte_terminal_get_char_height/width should be deprecated too (their only other use is computing the geometry hints, and we have vte_terminal_get_geometry_hints for that already.

As for a11y, I think we could simply continue to act as if scrolling were still grid based.

Created attachment 311807 [details] [review]
Demo6

I managed to figure out the clipping, yay! (It needs a bit more work at the cursor, but basically it's okay.)

This version also paints in the bottom gap in case the window size is not grid-aligned. Could you please test if you like the behavior? (If we choose this approach, I'll go ahead and complete it – mouse selection, dingus match aren't yet perfect there.)

Created attachment 312206 [details] [review]
Demo7

Fix preedit's position

Tested the latest patch and I like it :-)

Haven't reviewed the patch itself, but noticed that it increases the number of -Wfloat-equal warnings...

(In reply to Christian Persch from comment #21)
> Tested the latest patch and I like it :-)

Glad to hear it :) I also like this scrolling experience a lot!

There are still a few issues to be addressed with the extra padding at the bottom (if the window size is not properly aligned), e.g. extending the selection downwards, recognizing dingus.

> Haven't reviewed the patch itself, but noticed that it increases the number
> of -Wfloat-equal warnings...

They're okay. They occur when we set GtkAdjustment only if it actually changed. So far these were comparing the adjustment's double against scroll_delta's long, implicitly converting the double to long. Now they compare two doubles.

We should figure out how to silence gcc.

I added a static inline _vte_double_equal which silences gcc, but IMHO it should only be used when actually '==' is what we want. I suspect that in most cases, we only care about equality at pixel scale (ie when multiplied with char_heigth or char_width) or when rounded (floor? ceil?) to the nearest integer.

For the scrollbar, I think it's better to allow absolutely arbitrary value without any rounding.

We should check for quality in pixel scale when repainting the screen, you've already mentioned this in comment 5. Thanks for reminding: this is indeed missing from the current patch. Now we repaint even when scrolling was so small that the pixel offset didn't move.

We should check for equality* ...

+        /* Clip vertically only, for the sake of smooth scrolling. */

Does not clipping away the left/right padding really make a difference in scrolling speed?

+        /* Re-clip, allowing 1 more pixel row for the cursor. */
+        /* FIXME: only do so for outlined cursor!!! */

So we paint the outline cursor outside its cell? I really need to fix this as a follow-up...

Let's get this in quickly even if it's not perfect; we're at the start of the cycle and have all the time to improve it. I want to some cleanup of the code (remove all this 'add padding here, subtract padding there' business in rendering and event handling by just doing it once) but don't want you to have merge conflicts all over...

(In reply to Christian Persch from comment #26)

> Does not clipping away the left/right padding really make a difference in
> scrolling speed?

I deliberately don't want to crop there to let antialiasing overflow, see bug 734740. I'll add a comment.

> So we paint the outline cursor outside its cell? I really need to fix this
> as a follow-up...

It's been so ever since I remember, and I wouldn't want to change it. Other terminals draw the 1px border inside the cell, making it hard to read the cell's letter when not focused. Vte draws it outside of the cell, making it easy to read that letter. I like this one better.

> Let's get this in quickly even if it's not perfect; we're at the start of
> the cycle and have all the time to improve it. I want to some cleanup of the
> code (remove all this 'add padding here, subtract padding there' business in
> rendering and event handling by just doing it once) but don't want you to
> have merge conflicts all over...

Okay, I'll try to do a bit more cleanup and submit soon.

Not sure about your workflow; whenever I touch something padding-related, I found it useful to have a giant padding for a couple of days, let's say top=20px, left=30px (different values also help catching copy-paste errors where the axes are mixed up).

(In reply to Christian Persch from comment #26)

> I want to some cleanup of the
> code (remove all this 'add padding here, subtract padding there' business in
> rendering and event handling by just doing it once)

I was thinking about it a bit more. The current situation is indeed complex, easy to break, but at least works correctly. I'm not sure if it just evolved to be like this – I'm afraid it was doomed to evolve to be complex like this.

There are several situations where we do need to be perfectly aware of the padding.

E.g. painting: The outline cursor does legally extend to the padding area. The extra padding does need to be cleared when there's nothing to show there (we used to have artifacts remaining there).

E.g. mouse handling: We need to know when to start scrolling the selection (when the mouse enters the padding). We also confine clicks in the padding area to the actual cells, this is very useful in fullscreen mode (you can click on the very top of the screen and the app still receives it). See also bug 724253.

So I'm afraid you can't subtract top/left padding once and for all and then ignore about it. Scattered parts of the code need to be aware of them.

Or you can subtract it once, and allow x, y coordinates to be negative, denoting the top/left padding area. This would simplify the math at some places; however, the right edge would become (allocation_width - padding.left) instead of allocation_width [and similarly to bottom], so that'd be a bit more complicated. Also division with negative numbers suck, e.g. if the left 1px margin is denoted with x == -1, converting to cell number is no longer as simple as dividing by char_width, since that'd give 0 instead of the expected -1.

I'm wondering that perhaps keeping (0,0) for the whole canvas (incl. padding) might be the simplest, and have some convenience macros converting between cells and pixels that are used throughout the code, getting rid of inline multiplications or divisions as much as possible.

Created attachment 312611 [details] [review]
Demo8

Fixed: mouse reporting when scrolled back.

Discovered a new bug: Changes are not painted correctly near the bottom of the screen if scrolled back.

Example way to reproduce: 1. Produce some output (e.g. "ls -l"). 2. Start mc. 3. Disable alternate screen (from another terminal: echo -ne '\e[?1049l' > /dev/pts/[thisterminal]). 4. Press ^L to refresh the screen. 5. Scroll back with the scrollbar by about half a page. 6. Drag the mouse on the filemanager panels.

Created attachment 312612 [details] [review]
Demo9

Fix: the just discovered bug.

I also changed not to repaint the padding and extra padding if adjacent real contain is repainted – let me know if it's buggy and artifacts remain in the padding.

Created attachment 312616 [details] [review]
Demo10

Fixed: dingu recognition and autoscroll inside the extra bottom padding.

Do you think there will be folks who dislike this change and request the previous behavior?

We could add an API and a checkbox in g-t. The implementation I think would be as simple as a different logic within the pixel_to_row() and row_to_pixel() methods; all the rest could stay on the new infrastructure.

There will always be complainers :-) but I don't think we need to offer a pref here for now.

Okay. Just in case too many people complain, it's good to know that it can be done easily.

(In reply to Christian Persch from comment #26)

> I want to some cleanup of the
> code (remove all this 'add padding here, subtract padding there' business in
> rendering and event handling by just doing it once)

One thing that would really become handy (maybe even before I submit smooth scrolling) is if we didn't have to call gtk_widget_get_allocation() all the time to get the widget's dimensions; rather, on resize these values would be copied to VteTerminal (and have reasonable fake values there if the widget is not realized - if it's needed at all).

In addition to (or instead of) the allocation's width/height it could be handy to store the extra right and bottom padding's size (watch out: needs to be updated on font change).

In my patch I started having this at more and more places to compute the topmost and bottommost (perhaps partially) visible rows:

gtk_widget_get_allocation (&terminal->widget, &allocation);
top_row = _vte_terminal_pixel_to_row(terminal, 0);
bottom_row = _vte_terminal_pixel_to_row(terminal,
  allocation.height - terminal->pvt->padding.top - terminal->pvt->padding.bottom - 1);

It really deserves to be something simpler. Or moved to a helper method, but then that would have to call gtk_widget_get_allocation(). I have no clue how expensive it is to call this method.

Adding a helper method for that sounds good to me. gtk_widget_get_allocation isn't expensive, but we can certainly cache it in our size_allocate method.

Created attachment 312653 [details] [review]
Demo11

Several cleanups. Still a couple more to go.

Note1: In vte_terminal_handle_scroll() I changed _vte_terminal_scroll_region() to _vte_invalidate_all(). Actually _vte_terminal_scroll_region does nothing else than invalidates cells (it has a very bad name) and figuring out parameters that make sense wouldn't be trivial.

Note2: In vte_terminal_paint_cursor() I deliberately dropped the hilite stuff. It was about underlining the block cursor that's over a dingu match (so that its bottom pixel row is inverse) which in my opinion doesn't look good, and it was broken anyways, only worked on the first screenful of data because it compared screen-relative and scrollback-relative coordinates.

Looks like vte_terminal_scroll_region was a hold-over from ages ago when scrolling would just copy parts of the screen contents around. It's fine to remove it.

Created attachment 312764 [details] [review]
Demo12

A few more cleanups. a11y left to do, as well as saving a screen redraw on subpixel movements.

Christian, could you please start reviewing the patch?

Questions I have in particular:

* pixel_to_row() and friends: Should they stay as they are; or should I add way more trivial counterparts that operate on the x coordinate so that the caller code is nicer (is analogous for x and y); or should they - in the spirit of size_to_grid_size - operate on both coordinates at once?

* when calling paint_cursor(): The paint_cursor() method itself is unaware of the cr, it's the caller that handles it (just as for painting the regular content), but for one certain cursor shape we have a different clipping rectangle, so the caller also checks the cursor shape. It's quite ugly. Any idea how to do this nicer?

* I still get tons of warnings on float equality test (and sure this change adds more); is there a reason you didn't port most of these checks to the new method?

I updated the patch to apply cleanly to master (pushed to wip/bug746690 branch). Review later in the day :-)

(In reply to Egmont Koblinger from comment #39)
> * pixel_to_row() and friends: Should they stay as they are; or should I add
> way more trivial counterparts that operate on the x coordinate so that the
> caller code is nicer (is analogous for x and y); or should they - in the
> spirit of size_to_grid_size - operate on both coordinates at once?

IMHO it's ok as is for now. I want to do a general cleanup of coordinates later, but let's not expand the size of *this* patch too much :-)
 
> * when calling paint_cursor(): The paint_cursor() method itself is unaware
> of the cr, it's the caller that handles it (just as for painting the regular
> content), but for one certain cursor shape we have a different clipping
> rectangle, so the caller also checks the cursor shape. It's quite ugly. Any
> idea how to do this nicer?

Hmm yes, it's a bit ugly. You could add a simple vte_draw_clip() and use that to add the clip in paint_cursor() itself.  Or just draw inside the cell (comment 26, but you didn't like that too much :-) ). Or we could come up with another way to indicate 'not focused' in the cursor... maybe

> * I still get tons of warnings on float equality test (and sure this change
> adds more); is there a reason you didn't port most of these checks to the
> new method?

I didn't want to silence any warning without having investiaged the site thoroughly, which I didn't do for the remaining ones (and iirc they're scrolling related, so were going to be changed by this patch anyway).

BTW, I don't think I'll get to the review tonight, sorry :/

No worries, take your time! :-)

I was curious and googled if anyone else has thought of this possible feature.  Apart from the iterm2 counterpart of this bugreport (https://gitlab.com/gnachman/iterm2/issues/3209) I've only found one more at https://www.reddit.com/r/elementaryos/comments/29pvtz/does_no_one_care_about_smooth_scrolling/.  It's interesting to read the conflicting opinions, and a possible compromise:

"""
during scrolling, terminal feels exactly like any other smooth-scrolling app — it's scrolling at iota increments, not at line-height increments. But when you STOP scrolling, it "snaps" into place at the nearest line. Basically how combo boxes work on iOS >7.
"""

I guess "STOP scrolling" should mean the fingers no longer touch the touchpad, otherwise it could be confused with slow scrolling and that could be a very bad experience.  Is there an API for that at all?  Also not sure whether this feature would improve the UX or not (it might actually be frustrating, don't know).  (On the other hand, Ctrl+Shift+Up/Down and Shift+PageUp/PageDown do snap to rows, so it's available for those who want this functionality.)

Review of attachment 312764 [details] [review]:

A few comments below. Haven't checked all the math yet, so there will be another round of review :-)

::: src/vte.cc
@@ +439,3 @@
+         * and scrollbar being dragged back a bit. */
+        /* Confine top padding clicks while still in pixels. */
+        y = MAX(y, 0);

I think this should just return FALSE for clicks that are outside the grid area, so for x < 0 or x >= columns * char_width or y < 0 or y >= rows * char_height, so this MAX, the MIN and the CLAMP below can go.

@@ +488,3 @@
 	if (column_count == terminal->pvt->column_count &&
+            row_count == terminal->pvt->row_count &&
+            terminal->pvt->screen->scroll_delta == terminal->pvt->screen->insert_delta) {

I don't understand why the scroll_delta == insert_delta check here?

@@ +509,3 @@
 	rect.width -= rect.x;
 
+        rect.y = terminal->pvt->padding.top + _vte_terminal_row_to_pixel(terminal, row_start) - 1;

Previous code substracted scroll_delta from rows at the start of this function, and thus operated on small numbers here. Now row_to_pixel gets the possibly large numbers ... maybe we could store double scroll_delta and a long first_visible_row and use that here?

@@ +6457,3 @@
 	/* If the pointer hasn't moved to another character cell, then we
 	 * need do nothing. */
+	if (x / terminal->pvt->char_width  == terminal->pvt->mouse_last_x / terminal->pvt->char_width &&

If this deliberately doesn't use mouse_last_col/row, add a comment why?

@@ +7559,3 @@
 		if (always_grow) {
 			/* New endpoint is before existing selection. */
+			if (_vte_terminal_pixel_to_row(terminal, y) < start->y / height ||

Cache the result if _vte_terminal_pixel_to_row here (and elsewhere if it's used more than once), since it's not exactly free.

@@ +7906,3 @@
 	terminal->pvt->mouse_last_x = x;
 	terminal->pvt->mouse_last_y = y;
+        _vte_terminal_mouse_pixels_to_grid (terminal,

mouse_last_col/row will be undefined if this function returns FALSE (same problem in more places than here). Maybe add a variant of it that always translates the coordinates (assigning -1 for outside on left, column_count for outside on right, and similarly for top/bottom) ?

(In reply to Christian Persch from comment #44)

> I think this should just return FALSE for clicks that are outside the grid
> area, so for x < 0 or x >= columns * char_width or y < 0 or y >= rows *
> char_height, so this MAX, the MIN and the CLAMP below can go.

It's a really great feature of fullscreen vte that you can click on let's say the topmost pixel row of the screen (the padding) and it still acts like a click in the topmost character row. It makes it easier to click there as you can just quickly move the mouse all the way to the top, and don't have to retreat it by a pixel. I'll add a comment.

Will get back to the rest of your comments later.

(In reply to Egmont Koblinger from comment #45)

> It's a really great feature of fullscreen vte that you can click on let's
> say the topmost pixel row of the screen (the padding) and it still acts like
> a click in the topmost character row. It makes it easier to click there as
> you can just quickly move the mouse all the way to the top, and don't have
> to retreat it by a pixel.

Well, that's the theory. The practice is bug 756463.

(In reply to Christian Persch from comment #44)

> @@ +488,3 @@
>  	if (column_count == terminal->pvt->column_count &&
> +            row_count == terminal->pvt->row_count &&
> +            terminal->pvt->screen->scroll_delta ==
> terminal->pvt->screen->insert_delta) {
> 
> I don't understand why the scroll_delta == insert_delta check here?

You're right. It was my misunderstanding, I believed that if the entire "normal" area is updated but we're scrolled back in the history then we'd still get a spurious screen update.

> @@ +509,3 @@
>  	rect.width -= rect.x;
>  
> +        rect.y = terminal->pvt->padding.top +
> _vte_terminal_row_to_pixel(terminal, row_start) - 1;
> 
> Previous code substracted scroll_delta from rows at the start of this
> function, and thus operated on small numbers here. Now row_to_pixel gets the
> possibly large numbers ... maybe we could store double scroll_delta and a
> long first_visible_row and use that here?

It's a generic issue with the entire patch that numbers can get very large. See e.g. _vte_terminal_scroll_delta_pixel() which shouldn't exist if we were worried about it.

It would indeed be nicer to avoid such big numbers. However, no-one seems to have ever hit bug 738956, and the number of bytes per line is typically bigger than the character height, so you'll hit that bug first and not this one.

> @@ +6457,3 @@
>  	/* If the pointer hasn't moved to another character cell, then we
>  	 * need do nothing. */
> +	if (x / terminal->pvt->char_width  == terminal->pvt->mouse_last_x /
> terminal->pvt->char_width &&
> 
> If this deliberately doesn't use mouse_last_col/row, add a comment why?

Done (explanation added). Note that the explanation is only valid for y, for x I keep the same pattern for symmetry reasons. (I find this area in vte quite a mess and I don't feel like cleaning it up more than necessary :))

> @@ +7559,3 @@
>  		if (always_grow) {
>  			/* New endpoint is before existing selection. */
> +			if (_vte_terminal_pixel_to_row(terminal, y) < start->y / height ||
> 
> Cache the result if _vte_terminal_pixel_to_row here (and elsewhere if it's
> used more than once), since it's not exactly free.

Done.

> @@ +7906,3 @@
>  	terminal->pvt->mouse_last_x = x;
>  	terminal->pvt->mouse_last_y = y;
> +        _vte_terminal_mouse_pixels_to_grid (terminal,
> 
> mouse_last_col/row will be undefined if this function returns FALSE (same
> problem in more places than here). Maybe add a variant of it that always
> translates the coordinates (assigning -1 for outside on left, column_count
> for outside on right, and similarly for top/bottom) ?

Sigh. Looks like I'll have to do some cleanup.

I think the proper approach would be to go with two sets of last_x/y/col/row values. One for application mode (row in [0..height-1], values confined, none of the four variables updated when clicking in the scrollback area), and one for highlighting (row relative to beginning of history, values unclamped).

Submitted to master to get feedback from more people.

TODO:

- Don't update the screen if the scroll amount is so small that it didn't change the pixel offset.

- Double check the a11y bits, fix/revive the assertion there.

- Setting a special clipping area for the outlined rectangle area is very ugly, maybe figure out how to clean up the code.

- Fix/cleanup mouse last_x/y/col/row (see previous comment).

- Avoid working with huge pixel offset numbers (see previous comment).

Apparently FinalTerm does smooth scrolling when new data is produced. I've never tried this emulator, but I like how it looks at https://youtu.be/bvROuipepmg 0:08.

Smooth sub-cell scrolling works for me if I put a VteTerminal inside a GtkScrolledWindow and scroll using the touchpad. See bug 733210

Is there something else left to be done here?

Smooth scrolling was introduced in 0.44.

Not sure how it's related to the GtkScrolledWindow refactoring. I'm glad it still works (although this is fair expectation), and sure it wouldn't work with GtkScrolledWindow on its own without the work that was done in this bug. Seems to me the two are orthogonal. I might easily miss something, though.

This bug was kept open to address a few minor cleanups mentioned in comment 48.

(In reply to Egmont Koblinger from comment #51)
> Smooth scrolling was introduced in 0.44.
> 
> Not sure how it's related to the GtkScrolledWindow refactoring. I'm glad it
> still works (although this is fair expectation), and sure it wouldn't work
> with GtkScrolledWindow on its own without the work that was done in this
> bug. Seems to me the two are orthogonal.

Indeed. You are right. I confused myself while building vte and g-t with multiple gtk+ versions.

Comment on attachment 312764 [details] [review]
Demo12

(Something like this was committed, so marking obsolete.)

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vte/-/issues/2184.