After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 733210 - Support animated and touch scrolling
Support animated and touch scrolling
Status: RESOLVED OBSOLETE
Product: gnome-terminal
Classification: Core
Component: general
git master
Other All
: Normal normal
: ---
Assigned To: GNOME Terminal Maintainers
GNOME Terminal Maintainers
: 747709 795859 (view as bug list)
Depends on: 766569
Blocks:
 
 
Reported: 2014-07-15 15:21 UTC by Debarshi Ray
Modified: 2021-06-10 20:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screen-container: Support animated and touch scrolling (4.31 KB, patch)
2014-07-15 15:44 UTC, Debarshi Ray
none Details | Review
screen-container: Remove undefined public method (1.11 KB, patch)
2015-02-26 16:00 UTC, Debarshi Ray
committed Details | Review
screen-container: Remove wrong compiler attribute (1.06 KB, patch)
2015-02-26 16:01 UTC, Debarshi Ray
committed Details | Review
screen-container: Support animated and touch scrolling (4.53 KB, patch)
2015-02-26 16:01 UTC, Debarshi Ray
none Details | Review
Support animated and touch scrolling (14.08 KB, patch)
2016-05-12 19:15 UTC, Debarshi Ray
none Details | Review
Support animated and touch scrolling (15.70 KB, patch)
2016-05-13 10:49 UTC, Debarshi Ray
committed Details | Review
[gtk+] Handle GTK_SIZE_REQUEST_CONSTANT_SIZE (2.30 KB, patch)
2016-05-16 12:36 UTC, Debarshi Ray
none Details | Review
screen-container: Fix build with USE_SCROLLED_WINDOW and gtk+ >= 3.19.5 (3.73 KB, patch)
2016-11-04 12:38 UTC, Debarshi Ray
committed Details | Review
screen-container: Bubble the screen's size through the scrolled window (1.58 KB, patch)
2016-11-04 12:39 UTC, Debarshi Ray
none Details | Review
screen-container: Enable GtkScrolledWindow when gtk+ >= 3.22.0 (752 bytes, patch)
2016-11-04 12:41 UTC, Debarshi Ray
none Details | Review

Description Debarshi Ray 2014-07-15 15:21:24 UTC
In gtk+ 3.13.x GtkScrolledWindow animates the scrolling motion. See:

commit 3dcd0a24b1871c71e667df180334b4b861fbbc52
Author: Matthias Clasen <mclasen@redhat.com>
Date:   Mon Jun 30 18:12:39 2014 -0400

    GtkScrolledWindow: Enable animated scrolling
    
    We use gtk_adjustment_enable_animation to enable animated
    updates of the adjustments. Currently, this is enabled
    unconditionally, and with a duration that is hardcoded.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=732376

Touch or thumb scrolling also comes for free with GtkScrolledWindow.

Let's put the VteTerminal widget in a GtkScrolledWindow instead of creating our own vertical GtkScrollbar, so that we don't have to write our own code for these.
Comment 1 Debarshi Ray 2014-07-15 15:44:15 UTC
Created attachment 280733 [details] [review]
screen-container: Support animated and touch scrolling
Comment 2 Christian Persch 2014-08-16 17:12:36 UTC
Comment on attachment 280733 [details] [review]
screen-container: Support animated and touch scrolling

I don't like this approach. GtkScrolledWindow is for use when your content can adapt to size changes, which doesn't work with vteterminal. Abusing it by hiding its scrollbar behind its back is too hacky.
Comment 3 Debarshi Ray 2015-02-26 13:26:12 UTC
(In reply to Christian Persch from comment #2)
> I don't like this approach. GtkScrolledWindow is for use when your content
> can adapt to size changes, which doesn't work with vteterminal.

These days VteTerminal can rewrap the content. Or were you talking about something else?
Comment 4 Debarshi Ray 2015-02-26 16:00:37 UTC
Created attachment 297999 [details] [review]
screen-container: Remove undefined public method
Comment 5 Debarshi Ray 2015-02-26 16:01:07 UTC
Created attachment 298000 [details] [review]
screen-container: Remove wrong compiler attribute
Comment 6 Debarshi Ray 2015-02-26 16:01:35 UTC
Created attachment 298001 [details] [review]
screen-container: Support animated and touch scrolling
Comment 7 Matthias Clasen 2015-03-02 19:05:29 UTC
Review of attachment 298001 [details] [review]:

Looks good to me, otherwise. Great that the EXTERNAL policy found an actual use case!

::: src/terminal-screen-container.c
@@ +107,3 @@
+                                  GTK_POLICY_NEVER,
+                                  GTK_POLICY_ALWAYS);
+  gtk_container_add (GTK_CONTAINER (priv->sw), GTK_WIDGET (priv->screen));

it would be more idiomatic to say

gtk_scrolled_window_new (NULL, NULL);
gtk_container_add (GTK_CONTAINER (priv->sw), GTK_WIDGET (priv->screen));

and have the two deal with their adjustments internally...
Comment 8 Matthias Clasen 2015-03-02 19:06:21 UTC
Review of attachment 298001 [details] [review]:

Looks good to me, otherwise. Great that the EXTERNAL policy found an actual use case!

::: src/terminal-screen-container.c
@@ +107,3 @@
+                                  GTK_POLICY_NEVER,
+                                  GTK_POLICY_ALWAYS);
+  gtk_container_add (GTK_CONTAINER (priv->sw), GTK_WIDGET (priv->screen));

it would be more idiomatic to say

gtk_scrolled_window_new (NULL, NULL);
gtk_container_add (GTK_CONTAINER (priv->sw), GTK_WIDGET (priv->screen));

and have the two deal with their adjustments internally...
Comment 9 Matthias Clasen 2015-03-02 19:32:47 UTC
Review of attachment 298001 [details] [review]:

Looks good to me, otherwise. Great that the EXTERNAL policy found an actual use case!

::: src/terminal-screen-container.c
@@ +107,3 @@
+                                  GTK_POLICY_NEVER,
+                                  GTK_POLICY_ALWAYS);
+  gtk_container_add (GTK_CONTAINER (priv->sw), GTK_WIDGET (priv->screen));

it would be more idiomatic to say

gtk_scrolled_window_new (NULL, NULL);
gtk_container_add (GTK_CONTAINER (priv->sw), GTK_WIDGET (priv->screen));

and have the two deal with their adjustments internally...
Comment 10 Christian Persch 2015-03-02 21:03:29 UTC
(In reply to Debarshi Ray from comment #3)
> (In reply to Christian Persch from comment #2)
> > I don't like this approach. GtkScrolledWindow is for use when your content
> > can adapt to size changes, which doesn't work with vteterminal.
> 
> These days VteTerminal can rewrap the content. Or were you talking about
> something else?

Let me rephrase this. GtkScrolledWindow's implementation of GtkWidgetClass::get_preferred_* and ::size_allocate are monstrously complex, and I don't see any evidence that they handle GTK_POLICY_EXTERNAL correctly (should be same as NEVER, I think?) nor do they handle a GTK_SIZE_REQUEST_CONSTANT_SIZE child correctly (size_allocate uses |if (gtk_widget_get_request_mode (child) == GTK_SIZE_REQUEST_HEIGHT_FOR_WIDTH) { do HFW } else /* GTK_SIZE_REQUEST_WIDTH_FOR_HEIGHT */ { do WFH }| so it doesn't appear to even consider a CONSTANT_SIZE child).

What I would like is a *simple* scroll container that just forwards the request/allocate to the child and just tacks on the scrollbar(s).

(In reply to Debarshi Ray from comment #0)
>     We use gtk_adjustment_enable_animation to enable animated
>     updates of the adjustments. Currently, this is enabled
>     unconditionally, and with a duration that is hardcoded.

Do I understand correctly what this does: if adjustment is at value v_0 and, either programmatically, or by user interaction, should now be v_1, the value is animated progressively from v_0 to v_1 instead of being set directly to v_1?

If so, I don't see how this is a desirable thing for a terminal. This means that we'll have to load, decrypt, decompress, decode a whole range of intermediate scrollback content just to display it for 1/60th of a second...
can this part be disabled programmatically ?

> Touch or thumb scrolling also comes for free with GtkScrolledWindow.

How does this interact with selection, dingus, or mouse tracking mode?
Comment 11 Matthias Clasen 2015-03-02 21:48:32 UTC
(In reply to Christian Persch from comment #10)
 
> Let me rephrase this. GtkScrolledWindow's implementation of
> GtkWidgetClass::get_preferred_* and ::size_allocate are monstrously complex,
> and I don't see any evidence that they handle GTK_POLICY_EXTERNAL correctly
> (should be same as NEVER, I think?) 

No. The point of EXTERNAL is very much that it is different from NEVER. Why else would we add another enum value ? NEVER forces the scrolledwindow to make the entire child visible. EXTERNAL allows the child to be scrolled.

I agree that GtkScrolledWindows allocation code is complicated. It could be much simpler if we gave up on 'traditional' scrollbars and always used overlays.
Comment 12 Carlos Garnacho 2015-03-02 22:14:13 UTC
(In reply to Christian Persch from comment #10)
> (In reply to Debarshi Ray from comment #3)
> > (In reply to Christian Persch from comment #2)
> > > I don't like this approach. GtkScrolledWindow is for use when your content
> > > can adapt to size changes, which doesn't work with vteterminal.
> > 
> > These days VteTerminal can rewrap the content. Or were you talking about
> > something else?
> 
> Let me rephrase this. GtkScrolledWindow's implementation of
> GtkWidgetClass::get_preferred_* and ::size_allocate are monstrously complex,
> and I don't see any evidence that they handle GTK_POLICY_EXTERNAL correctly
> (should be same as NEVER, I think?) nor do they handle a
> GTK_SIZE_REQUEST_CONSTANT_SIZE child correctly (size_allocate uses |if
> (gtk_widget_get_request_mode (child) == GTK_SIZE_REQUEST_HEIGHT_FOR_WIDTH) {
> do HFW } else /* GTK_SIZE_REQUEST_WIDTH_FOR_HEIGHT */ { do WFH }| so it
> doesn't appear to even consider a CONSTANT_SIZE child).
> 
> What I would like is a *simple* scroll container that just forwards the
> request/allocate to the child and just tacks on the scrollbar(s).

What I think we aim on the GTK+ side is having all scroll/device details (touch, kinetic scroll, smooth scroll, plain old 4..7 buttons...) in a single point, instead of duplicated/copied across.

> 
> (In reply to Debarshi Ray from comment #0)
> >     We use gtk_adjustment_enable_animation to enable animated
> >     updates of the adjustments. Currently, this is enabled
> >     unconditionally, and with a duration that is hardcoded.
> 
> Do I understand correctly what this does: if adjustment is at value v_0 and,
> either programmatically, or by user interaction, should now be v_1, the
> value is animated progressively from v_0 to v_1 instead of being set
> directly to v_1?
> 
> If so, I don't see how this is a desirable thing for a terminal. This means
> that we'll have to load, decrypt, decompress, decode a whole range of
> intermediate scrollback content just to display it for 1/60th of a second...
> can this part be disabled programmatically ?

This is just true if changing between distant regions of the buffer. I wouldn't think this is the most usual operation? Wouldn't this be acceptable for PgUp/Down increments?

> 
> > Touch or thumb scrolling also comes for free with GtkScrolledWindow.
> 
> How does this interact with selection, dingus, or mouse tracking mode?

Just fine, FYI this is enabled by default, there's good chances this is a must somewhere else.
Comment 13 Christian Persch 2015-03-03 18:44:59 UTC
Actually testing the patch reveals several problems:

* Lots of warnings on console: (gnome-terminal-server:16272): Gtk-WARNING **: Toplevel size doesn't seem to directly depend on the size of the geometry widget from gtk_window_set_geometry_hints(). The geometry widget might not be in the window, or it might not be packed into the window appropriately

* Creating tabs and switching between them shrinks the window

* Even just running 'ls' at the prompt shrinks the window

I guess the above are all due to the warning.

* Checking "Show scrollbar" in prefs doesn't work anymore: there is no scrollbar visible while not moving the mouse; when moving the mouse over the terminal but outside the scrollbar area, a thick rounded rectangle shows up, which only turns into a (transparent, ugh) scrollbar when the mouse if over it. 

* The scrollbar is now transparent on top of the content, instead of intransparent and beside the content, as it should be. This means that (depending on font size), it's impossible to start selecting in the last 1-2 char cells of each row. Filed as bug .

I think both of these can be fixed by setting the overlay-scrolling property to FALSE, which we should therefore do.

(In reply to Carlos Garnacho from comment #12)
> > > Touch or thumb scrolling also comes for free with GtkScrolledWindow.
> > 
> > How does this interact with selection, dingus, or mouse tracking mode?
> 
> Just fine, FYI this is enabled by default, there's good chances this is a
> must somewhere else.

Have you actually tested this on a touch device? Because I don't see how it *can* work if the scrolled window is intercepting events over the terminal.
Comment 14 Matthias Clasen 2015-03-03 18:47:48 UTC
(In reply to Christian Persch from comment #13)

> * The scrollbar is now transparent on top of the content, instead of
> intransparent and beside the content, as it should be. This means that
> (depending on font size), it's impossible to start selecting in the last 1-2
> char cells of each row. Filed as bug .

Thats not a bug. It wouldn't be an overlay scrollbar if it wasn't, you know, overlayed.

> 
> Have you actually tested this on a touch device? Because I don't see how it
> *can* work if the scrolled window is intercepting events over the terminal.

It is not intercepting events that are not vertical swipes.
Comment 15 Christian Persch 2015-03-03 18:51:42 UTC
(In reply to Matthias Clasen from comment #14)
> Thats not a bug. It wouldn't be an overlay scrollbar if it wasn't, you know,
> overlayed.

Right. I found the overlay-scrolling property and just forgot to take out the bit about filing a bug.
 
> > Have you actually tested this on a touch device? Because I don't see how it
> > *can* work if the scrolled window is intercepting events over the terminal.
> 
> It is not intercepting events that are not vertical swipes.

Selecting usually is a vertical swipe. And in (full) mouse tracking mode, *all* events need to be send to the terminal.
Comment 16 Debarshi Ray 2015-03-10 12:46:55 UTC
(In reply to Christian Persch from comment #13)
> Actually testing the patch reveals several problems:
> 
> * Lots of warnings on console: (gnome-terminal-server:16272): Gtk-WARNING
> **: Toplevel size doesn't seem to directly depend on the size of the
> geometry widget from gtk_window_set_geometry_hints(). The geometry widget
> might not be in the window, or it might not be packed into the window
> appropriately
> 
> * Creating tabs and switching between them shrinks the window
>
> * Even just running 'ls' at the prompt shrinks the window

Oops, sorry. I did not test it with tabs. I thought we fixed the shrinking issue in bug 743395 , but apparently not.

> I guess the above are all due to the warning.

Possibly. They don't look as harmless as I assumed them to be. I will take a look.

> * Checking "Show scrollbar" in prefs doesn't work anymore: there is no
> scrollbar visible while not moving the mouse; when moving the mouse over the
> terminal but outside the scrollbar area, a thick rounded rectangle shows up,
> which only turns into a (transparent, ugh) scrollbar when the mouse if over
> it. 
> 
> * The scrollbar is now transparent on top of the content, instead of
> intransparent and beside the content, as it should be. This means that
> (depending on font size), it's impossible to start selecting in the last 1-2
> char cells of each row. Filed as bug .
> 
> I think both of these can be fixed by setting the overlay-scrolling property
> to FALSE, which we should therefore do.

How about adding an option in preferences to toggle the overlay-scrolling ?
Comment 17 Christian Persch 2015-04-11 19:57:36 UTC
*** Bug 747709 has been marked as a duplicate of this bug. ***
Comment 18 Christian Persch 2015-04-11 19:58:31 UTC
(In reply to Debarshi Ray from comment #16)
> How about adding an option in preferences to toggle the overlay-scrolling ?

Sure.
Comment 19 Egmont Koblinger 2015-11-02 21:40:14 UTC
I've tried this now (just as a user; haven't looked at the code). A couple of remarks:


(In reply to Christian Persch from comment #10)

> Do I understand correctly what this does: if adjustment is at value v_0 and,
> either programmatically, or by user interaction, should now be v_1, the
> value is animated progressively from v_0 to v_1 instead of being set
> directly to v_1?
> 
> If so, I don't see how this is a desirable thing for a terminal. This means
> that we'll have to load, decrypt, decompress, decode a whole range of
> intermediate scrollback content just to display it for 1/60th of a second...
> can this part be disabled programmatically ?

This is what happens currently if you drag the legacy scrollbar, and it was never raised as an issue. The animation is quite smooth with a 10k scrollback and is a cool eye-candy and is consistent with the rest of the apps. Consider a file manager with tons of files and icons to display for them based on mime type, or even actual thumbnail for pictures. I don't think rendering the content is any cheaper there. So I wouldn't be worried about this.

I'm not sure if the scrollbar's implementation is clever enough (but if not then it should be made smarter) to skip frames if the app is slow rendering.


> How does this interact with selection, dingus, or mouse tracking mode?

I haven't seen any problem here (apart from the obvious that starting the selection in the last column becomes troublesome).


(In reply to Carlos Garnacho from comment #12)

> > If so, I don't see how this is a desirable thing for a terminal. This means
> > that we'll have to load, decrypt, decompress, decode a whole range of
> > intermediate scrollback content just to display it for 1/60th of a second...
> > can this part be disabled programmatically ?
> 
> This is just true if changing between distant regions of the buffer. I
> wouldn't think this is the most usual operation? Wouldn't this be acceptable
> for PgUp/Down increments?

You're right. For PgUp/Down increments we hardly ever load/decrypt/decompress, since the most recently read 64 kB chunk of the scrollback buffer is cached in its decrypted/decompressed form.


By the way: PgUp/Down animates the scrolling in Files, but not in g-t (Shift-PgUp/Down). No clue what it'd take source code wise to change it, but it would be cool to see that behavior.


(In reply to Christian Persch from comment #15)

> Selecting usually is a vertical swipe.

Could you please elaborate? Isn't that a vertical simple mouse movement? (mouse: move vs. scroll wheel; touchpad: one vs. two fingers)


> And in (full) mouse tracking mode,
> *all* events need to be send to the terminal.

I see no behavioral change here. In mouse tracking mode, vertical scrolls are sent to the terminal.


(In reply to Debarshi Ray from comment #16)

> How about adding an option in preferences to toggle the overlay-scrolling ?

Do we need that?  Even though not user-friendly to set, making an overlay scrollbar non-overlay is just a matter of CSS.  E.g. set the overlay scrollbar's width to 5px and vte's right padding to 6px, voilà, it no longer overlays the actual content :)


One thing I find terribly irritating, but it could also be just a CSS issue. At least in Ubuntu Wily's default desktop (Unity, Ambiance), the scrollbar appears as soon as I move the mouse or produce output (the adjustment's parameters change), and disappears after a short inactivity. This results in a continuous flicker of a prominent orange-ish color.

My expectation: moving the mouse somewhere not over the scrollbar, or changing the terminal's contents (and implicitly scrolling by producing output) shouldn't change the look of the scrollbar, apart from the necessary adjustment of its height. A prominent change in appearance (color and/or width) should only occur if I move the mouse there, or scroll explicitly with a keycombo. Is it solveable with overlay scrollbar?
Comment 20 Christian Persch 2016-02-21 15:32:39 UTC
So does this work better with gtk+ master now that geometry support has been broken? In any case, there are some things that need to be done:

* GtkScrolledWindow need to be fixed to support GTK_SIZE_REQUEST_CONSTANT_SIZE widgets.

* We can't use GtkScrolledWindow unconditionally, since it will break (at least) on older gtk, so commit 41eb2ec73653adc5fd36fa73cebdc8a6169f8516 reverted and made to only use GtkScrolledWindow on new gtk.

* Need to confirm this does not break selection, nor mouse tracking mode.
Comment 21 Debarshi Ray 2016-05-12 09:23:20 UTC
Since VteTerminal handles GtkWidget::scroll-event, putting it inside a GtkScrolledWindow is not enough to make kinetic scrolling work. See bug 721893
Comment 22 Christian Persch 2016-05-12 11:55:00 UTC
(In reply to Carlos Garnacho from comment #12)
> > If so, I don't see how this is a desirable thing for a terminal. This means
> > that we'll have to load, decrypt, decompress, decode a whole range of
> > intermediate scrollback content just to display it for 1/60th of a second...
> > can this part be disabled programmatically ?
> 
> This is just true if changing between distant regions of the buffer. I
> wouldn't think this is the most usual operation? Wouldn't this be acceptable
> for PgUp/Down increments?

I'm mostly concerned about this for when you've scrolled way up, and then we snap to bottom on output or keypress. So IMHO setting the adjustment programmatically without direct user interaction via the scrollbar (or kinetic once that's enabled) should *never* animate.

(In reply to Debarshi Ray from comment #21)
> Since VteTerminal handles GtkWidget::scroll-event, putting it inside a
> GtkScrolledWindow is not enough to make kinetic scrolling work. See bug
> 721893

Let's not get sidetracked too much here. I think we should *first* use GtkScrolledWindow as the container (without overlay scrollbars), and only as follow-ups worry about making kinetic scrolling work and enabling overlay scrollbars.

So for simply making use of GtkScrolledWindow, comment 21 and bits of comment 13 need to be addressed.
Comment 23 Christian Persch 2016-05-12 11:57:43 UTC
Oh, and we need a way to remove those annoying dashed lines that are added at the borders of the GtkScrolledWindow if there's content scrolled out in that direction.
Comment 24 Matthias Clasen 2016-05-12 12:26:24 UTC
(In reply to Christian Persch from comment #23)
> Oh, and we need a way to remove those annoying dashed lines that are added
> at the borders of the GtkScrolledWindow if there's content scrolled out in
> that direction.

Use css
Comment 25 Debarshi Ray 2016-05-12 19:15:26 UTC
Created attachment 327735 [details] [review]
Support animated and touch scrolling

I have reworked the patch against gtk-3-20.

It needs the first two gnome-terminal patches from bug 760944

Creating tabs doesn't shrink the window anymore. The window expands on the creation of the first tab, and then shrinks back to the old height when the last tab is gone. No more WARNINGS.

Overlay scrolling has been switched off.
Comment 26 Debarshi Ray 2016-05-12 19:28:57 UTC
(In reply to Christian Persch from comment #20)
> * We can't use GtkScrolledWindow unconditionally, since it will break (at
> least) on older gtk, so commit 41eb2ec73653adc5fd36fa73cebdc8a6169f8516
> reverted and made to only use GtkScrolledWindow on new gtk.

I assume that you are worried about gtk+ < 3.20, right?

> * Need to confirm this does not break selection, nor mouse tracking mode.

Mouse tracking mode should work because VteTerminal steals the GtkWidget::scroll-event from the GtkScrolledWindow. (This is why kinetic scrolling is not working at the moment.)
Comment 27 Debarshi Ray 2016-05-13 10:20:00 UTC
(In reply to Matthias Clasen from comment #9)
> Review of attachment 298001 [details] [review] [review]:
> ::: src/terminal-screen-container.c
> @@ +107,3 @@
> +                                  GTK_POLICY_NEVER,
> +                                  GTK_POLICY_ALWAYS);
> +  gtk_container_add (GTK_CONTAINER (priv->sw), GTK_WIDGET (priv->screen));
> 
> it would be more idiomatic to say
> 
> gtk_scrolled_window_new (NULL, NULL);
> gtk_container_add (GTK_CONTAINER (priv->sw), GTK_WIDGET (priv->screen));
> 
> and have the two deal with their adjustments internally...

Passing NULL and letting GtkScrolledWindow create a new GtkAdjustment and pass it to VteTerminal is not working the same way as getting the GtkAdjustment out of VteTerminal and giving it to GtkScrolledWindow. A brief investigation suggests that the page-increment is different, which causes it to scroll by 1 line instead of page-increment/10 lines when using the mouse's scroll wheel.

One could say that it is a bug in the GtkScrollable implementation. For the purposes of this bug, let's keep doing what the old has been doing so far. ie. the latter option.
Comment 28 Debarshi Ray 2016-05-13 10:49:10 UTC
Created attachment 327779 [details] [review]
Support animated and touch scrolling

* Remove undershoot indicators.
* Ensure GtkScrolledWindow is only used with newer gtk+
Comment 29 Debarshi Ray 2016-05-13 11:00:38 UTC
This also gives us smooth sub-cell scrolling with touch pads. See bug 746690
Comment 30 Christian Persch 2016-05-15 18:41:52 UTC
Review of attachment 327779 [details] [review]:

Before I review this more in depth, I have a question:

> GtkScrolledWindow doesn't use the natural height and width
> of its child widget if the scrollbar policy is not NEVER. Instead it
> uses the minimum size. We don't want that because the minimum size of
> a VteTerminal is 1x1.

Wouldn't an adding support to GtkScrolledWindow for GTK_SIZE_REQUEST_CONSTANT_SIZE widget looks essentially like this new code in terminal-scrolled-window? If so, I'd really prefer this part to be done inside gtk+.

> we override GtkScrolledWindow's
> get_preferred_height and get_preferred_width virtual methods to make
> it use the natural size.

This will still run GtkScrolledWindow's size-allocate function which doesn't handle CONSTANT_SIZE children...

::: src/terminal-screen-container.c
@@ +21,3 @@
 #include "terminal-debug.h"
 
+#if GTK_CHECK_VERSION (3, 19, 5)

Let's do

#if gtk 3.19.5
#define USE_SCROLLED_WINDOW
#endif

and then use that new define everywhere below. Makes it easier by just having to change one place when one wants to try both code paths.

@@ +134,3 @@
+
+  gtk_container_add (GTK_CONTAINER (container), priv->scrolled_window);
+  gtk_widget_show_all (priv->scrolled_window);

I prefer individual gtk_widget_show() for each widget instead of show-all.
Comment 31 Debarshi Ray 2016-05-16 12:34:27 UTC
(In reply to Christian Persch from comment #30)
> Review of attachment 327779 [details] [review] [review]:
> 
> Before I review this more in depth, I have a question:
> 
> > GtkScrolledWindow doesn't use the natural height and width
> > of its child widget if the scrollbar policy is not NEVER. Instead it
> > uses the minimum size. We don't want that because the minimum size of
> > a VteTerminal is 1x1.
> 
> Wouldn't an adding support to GtkScrolledWindow for
> GTK_SIZE_REQUEST_CONSTANT_SIZE widget looks essentially like this new code
> in terminal-scrolled-window? If so, I'd really prefer this part to be done
> inside gtk+.
> 
> > we override GtkScrolledWindow's
> > get_preferred_height and get_preferred_width virtual methods to make
> > it use the natural size.
> 
> This will still run GtkScrolledWindow's size-allocate function which doesn't
> handle CONSTANT_SIZE children...

I actually wrote a quick patch against gtk_scrolled_window_allocate for GTK_SIZE_REQUEST_CONSTANT_SIZE, but that didn't do the trick. I will attach it shortly.

As far as I understand, size-allocate is called after the size has already been allocated. The way GtkScrolledWindow's get_preferred_height/_width works, by the time we are in gtk_scrolled_window_allocate, the allocated size is already too small.
Comment 32 Debarshi Ray 2016-05-16 12:36:00 UTC
Created attachment 327973 [details] [review]
[gtk+] Handle GTK_SIZE_REQUEST_CONSTANT_SIZE

This is the rough gtk+ patch that I sketched out to see if GtkScrolledWindow can be improved for GTK_SIZE_REQUEST_CONSTANT_SIZE children. It didn't make any difference.
Comment 33 Debarshi Ray 2016-05-16 12:42:58 UTC
(In reply to Debarshi Ray from comment #29)
> This also gives us smooth sub-cell scrolling with touch pads. See bug 746690

Scratch this. Sub-cell scrolling with touch pads already works since 0.44. Sorry for the confusion.
Comment 34 Carlos Garnacho 2016-05-16 15:38:44 UTC
AFAICS the size requisition code in attachment 327779 [details] [review] is virtually the same than gtk_scrolled_window_measure(), except:

https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c#n1751 and
https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c#n1777

It seems that GtkScrolledWindow falls in this case through the last branch (vertical scrollbar is visible through policy and you disable overlay indicators), so GtkScrolledWindow settles for the minimum/natural scrollbar height despite the child size.

I'm tbh unsure if GTK_SIZE_REQUEST_CONSTANT_SIZE is meant to magically fix this or is rather somewhat tangential, as the choice to obey scrollbar or content requisitions are not based on size or request modes, rather policy ones.
Comment 35 Debarshi Ray 2016-05-17 15:08:22 UTC
VteTerminal uses GTK_SCROLL_NATURAL by default, unlike all the GtkScrollable implementations inside gtk+. Therefore, one option is to special-case GTK_SCROLL_NATURAL children in GtkScrolledWindow's size requisition code. See bug 766569
Comment 36 Christian Persch 2016-08-01 17:38:02 UTC
Comment on attachment 327779 [details] [review]
Support animated and touch scrolling

I've committed a version of this part, minus the terminal-scrolled-window.[ch] files, disabled (#if 0) by default. To enable this (changing to a #if GTK_CHECK_VERSION(whatever)), GtkScrolledWindow needs to be fixed.
Comment 37 Christian Persch 2016-09-04 13:00:22 UTC
+terminal-window scrolledwindow undershoot.top,
+terminal-window scrolledwindow undershoot.bottom {
+  background: none;
+}

BTW: is this guaranteed to completely disable this (including the repeated invalidations from the animation), or might themes do something beyond background on overshoot/undershoot? Ie wouldn't be display:none be more appropriate for this?
Comment 38 Debarshi Ray 2016-11-04 12:38:03 UTC
Created attachment 339114 [details] [review]
screen-container: Fix build with USE_SCROLLED_WINDOW and gtk+ >= 3.19.5
Comment 39 Debarshi Ray 2016-11-04 12:39:54 UTC
Created attachment 339116 [details] [review]
screen-container: Bubble the screen's size through the scrolled window
Comment 40 Debarshi Ray 2016-11-04 12:41:22 UTC
Created attachment 339117 [details] [review]
screen-container: Enable GtkScrolledWindow when gtk+ >= 3.22.0
Comment 41 Debarshi Ray 2017-10-20 16:22:10 UTC
(In reply to Debarshi Ray from comment #38)
> Created attachment 339114 [details] [review] [review]
> screen-container: Fix build with USE_SCROLLED_WINDOW and gtk+ >= 3.19.5

Hey Christian, we should probably merge this because it fixes up commit 0dd655c569f8b81
Comment 42 Christian Persch 2017-10-20 17:42:50 UTC
Comment on attachment 339114 [details] [review]
screen-container: Fix build with USE_SCROLLED_WINDOW and gtk+ >= 3.19.5

Hmm... why move the code around? AFAICT just removing the #ifdef around the widget_class declaration should fix this, shouldn't it?
Comment 43 Debarshi Ray 2017-10-20 18:07:03 UTC
(In reply to Christian Persch from comment #42)
> Comment on attachment 339114 [details] [review] [review]
> screen-container: Fix build with USE_SCROLLED_WINDOW and gtk+ >= 3.19.5
> 
> Hmm... why move the code around? AFAICT just removing the #ifdef around the
> widget_class declaration should fix this, shouldn't it?

Mainly because !USE_SCROLLED_WINDOW and GTK_CHECK_VERSION(3, 19, 5) cases are mutually independent. I am happy to re-arrange it differently.
Comment 44 Christian Persch 2017-10-20 18:20:13 UTC
Comment on attachment 339114 [details] [review]
screen-container: Fix build with USE_SCROLLED_WINDOW and gtk+ >= 3.19.5

Ok, don't bother, just commit as-is.
Comment 45 Debarshi Ray 2017-10-22 07:42:22 UTC
Comment on attachment 339114 [details] [review]
screen-container: Fix build with USE_SCROLLED_WINDOW and gtk+ >= 3.19.5

Ok, thanks. Pushed to master.
Comment 46 Christian Persch 2019-11-20 22:46:37 UTC
*** Bug 795859 has been marked as a duplicate of this bug. ***
Comment 47 GNOME Infrastructure Team 2021-06-10 20:49:21 UTC
-- 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/gnome-terminal/-/issues/7479.