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 775642 - unable to disable "scroll on output" in build output panel
unable to disable "scroll on output" in build output panel
Status: RESOLVED WONTFIX
Product: gnome-builder
Classification: Other
Component: editor
Flatpak Nightly Channel
Other Linux
: Normal normal
: ---
Assigned To: GNOME Builder Maintainers
GNOME Builder Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-12-05 14:41 UTC by Felix Schwarz
Modified: 2017-12-17 12:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Disable 'scroll-on-output' in build log panel (2.96 KB, patch)
2016-12-12 11:56 UTC, Kritarth
none Details | Review
Disable 'scroll-on-output' in build log panel (3.91 KB, patch)
2016-12-12 16:38 UTC, Kritarth
none Details | Review
Disable 'scroll-on-output' in build log panel (3.87 KB, patch)
2016-12-12 18:31 UTC, Kritarth
committed Details | Review
Correction in patch for disabling scroll-on-output in build log panel (1.10 KB, patch)
2016-12-13 18:16 UTC, Kritarth
none Details | Review

Description Felix Schwarz 2016-12-05 14:41:29 UTC
The build output panel always scrolls down when new output was received so I can't read previous output while the build is still running. It would be nice to have some kind of "scroll on output" setting or "just" replicate the behavior of gnome-terminal.
Comment 1 Kritarth 2016-12-12 11:56:40 UTC
Created attachment 341812 [details] [review]
Disable 'scroll-on-output' in build log panel

Currently, the build log panel is bound to scroll to end whenever new output
is added to the buffer using gtk_text_view_scroll_to_mark. In order to disable
this auto-scrolling I added horizontal and vertical GtkAdjustments. With these
adjustments I check if the user has scrolled back or right manually.
expression1: value >= upper - page_size - offset
This expression returns true if the user has not scrolled vertically back.
The 'offset' is set at 30 (may need further tweaking).
Detection of horizontal scrolling is done by
expression2: value <= 3.0
3.0 is selected because the left-margin of text_view is set at 3.

if (expression1 && expression2) then
	set the vertical adjustment to upper - page_size
	apply this adjustment to the container
Comment 2 Kritarth 2016-12-12 12:01:17 UTC
Hi, This is my first patch so please guide me how I can improve this further.
Thanks
Comment 3 sébastien lafargue 2016-12-12 13:02:08 UTC
Review of attachment 341812 [details] [review]:

::: plugins/build-tools/gbp-build-log-panel.c
@@ +41,3 @@
+
+  GtkAdjustment *hadjustment;
+  GtkAdjustment *vadjustment;

you need to align params with previous fields here

@@ +125,3 @@
+      gtk_adjustment_get_upper (self->vadjustment) -
+      gtk_adjustment_get_page_size (self->vadjustment) - 30.0  &&
+      gtk_adjustment_get_value(self->hadjustment) <= 3.0 )

Don't use *magic numbers* in the code (30.0, 3.0), use #define for this with a meaningful name

Use a well-named temporary variable to simplify the condition,
it'll ease the understanding of what is done for people reading/working on the code later.

@@ +135,1 @@
 }

you don't need to get and re-set the adjustment, just set_value is enough
and if you use a temp var to compute get_upper - get_page_size previously, you can re-use it here.
Comment 4 Kritarth 2016-12-12 16:38:36 UTC
Created attachment 341831 [details] [review]
Disable 'scroll-on-output' in build log panel

Currently, the build log panel is bound to scroll to end whenever new output
is added to the buffer using gtk_text_view_scroll_to_mark. In order to disable
this auto-scrolling I added horizontal and vertical GtkAdjustments. These
adjustments are initialised in gbp_build_log_panel_init(). They point to
the adjustments of GtkScrolledWindow scroller. With these adjustments I check
if the user has scrolled back or right manually.

expression1: value >= upper - page_size - offset
This expression returns true if the user has not scrolled vertically back.
The 'offset' is defined as VERTICAL_AUTOSCROLL_TOLERENCE - this means the
limit upto which scroll-on-output will still work even if the user has
manually scrolled.

Similarly HORIZONTAL_AUTOSCROLL_TOLERENCE is defined.
Detection of horizontal scrolling is done by
expression2: value <= HORIZONTAL_AUTOSCROLL_TOLERENCE
HORIZONTAL_AUTOSCROLL_TOLERENCE is set to 3 because the left-margin of text_view is also set at 3.

if (expression1 && expression2) then
	set the vertical adjustment to upper - page_size
	apply this adjustment to the container
Comment 5 sébastien lafargue 2016-12-12 17:35:40 UTC
Review of attachment 341831 [details] [review]:

::: plugins/build-tools/gbp-build-log-panel.c
@@ +107,3 @@
+                                                    gtk_adjustment_get_page_size (self->vadjustment) -
+                                                    VERTICAL_AUTOSCROLL_TOLERENCE;
+

most of the time we try to not mix variables declaration and affection, so do:

gboolean h_scroll_left;

then after the asserts:

h_scroll_left = ....

Also i was thinking about doing:

gdouble last_page_pos;
....
last_page_top = gtk_adjustment_get_upper (self->vadjustment) - gtk_adjustment_get_page_size (self->vadjustment)

so that we can re-use the result in the upcoming block (l132). minor optimization of course.

@@ +133,3 @@
+      gtk_adjustment_set_value (self->vadjustment,
+                                gtk_adjustment_get_upper (self->vadjustment) -
+                                gtk_adjustment_get_page_size (self->vadjustment));

and here we can re-use your last_page_top

@@ +134,3 @@
+                                gtk_adjustment_get_upper (self->vadjustment) -
+                                gtk_adjustment_get_page_size (self->vadjustment));
+      gtk_scrolled_window_set_vadjustment (self->scroller, self->vadjustment);

no need to affect the adjustment to the scroller again,
unless we need to change it (itself, not its value), it stay the same for the lifetime of the scroller.

@@ +135,3 @@
+                                gtk_adjustment_get_page_size (self->vadjustment));
+      gtk_scrolled_window_set_vadjustment (self->scroller, self->vadjustment);
+    }

as the block is now: gtk_adjustment_set_value (self->adjustment, last_page_top);
we can even skip the block definition and use a one-liner like:

if (v_scroll_bottom && h_scroll_left)
  gtk_adjustment_set_value (self->adjustment, last_page_top);
Comment 6 Kritarth 2016-12-12 17:49:26 UTC
(In reply to sébastien lafargue from comment #5)
> Review of attachment 341831 [details] [review] [review]:
> 
> ::: plugins/build-tools/gbp-build-log-panel.c
> @@ +107,3 @@
> +                                                   
> gtk_adjustment_get_page_size (self->vadjustment) -
> +                                                   
> VERTICAL_AUTOSCROLL_TOLERENCE;
> +
> 
> most of the time we try to not mix variables declaration and affection, so
> do:
> 
> gboolean h_scroll_left;
> 
> then after the asserts:
> 
> h_scroll_left = ....
> 
> Also i was thinking about doing:
> 
> gdouble last_page_pos;
> ....
> last_page_top = gtk_adjustment_get_upper (self->vadjustment) -
> gtk_adjustment_get_page_size (self->vadjustment)
> 
> so that we can re-use the result in the upcoming block (l132). minor
> optimization of course.
> 
> @@ +133,3 @@
> +      gtk_adjustment_set_value (self->vadjustment,
> +                                gtk_adjustment_get_upper
> (self->vadjustment) -
> +                                gtk_adjustment_get_page_size
> (self->vadjustment));
> 
> and here we can re-use your last_page_top
> 
> @@ +134,3 @@
> +                                gtk_adjustment_get_upper
> (self->vadjustment) -
> +                                gtk_adjustment_get_page_size
> (self->vadjustment));
> +      gtk_scrolled_window_set_vadjustment (self->scroller,
> self->vadjustment);
> 
> no need to affect the adjustment to the scroller again,
> unless we need to change it (itself, not its value), it stay the same for
> the lifetime of the scroller.
> 
> @@ +135,3 @@
> +                                gtk_adjustment_get_page_size
> (self->vadjustment));
> +      gtk_scrolled_window_set_vadjustment (self->scroller,
> self->vadjustment);
> +    }
> 
> as the block is now: gtk_adjustment_set_value (self->adjustment,
> last_page_top);
> we can even skip the block definition and use a one-liner like:
> 
> if (v_scroll_bottom && h_scroll_left)
>   gtk_adjustment_set_value (self->adjustment, last_page_top);

Thank you very much for your guidance. About the minor optimization that you are suggesting. The message is added to buffer and then we need get_upper(). We cannot re-use the get_upper() - get_page_size() because get_upper() is updated after the buffer is updated and we need the updated get_upper().
Please correct me if I have this wrong.
Comment 7 sébastien lafargue 2016-12-12 17:57:55 UTC
hmm, maybe but playing with adjustment can be tricky, a lot is done async via signals, and as we add line by line, it's probably still working in both cases. want to test the values with a few printf ?
Comment 8 Kritarth 2016-12-12 18:20:12 UTC
yes, it is also working. that is probably working only because the lines are being added one at a time.
So i will make the other changes now.
Comment 9 Kritarth 2016-12-12 18:31:17 UTC
Created attachment 341838 [details] [review]
Disable 'scroll-on-output' in build log panel

Currently, the build log panel is bound to scroll to end whenever new output
is added to the buffer using gtk_text_view_scroll_to_mark. In order to disable
this auto-scrolling I added horizontal and vertical GtkAdjustments. These
adjustments are initialised in gbp_build_log_panel_init(). They point to
the adjustments of GtkScrolledWindow scroller. With these adjustments I check
if the user has scrolled back or right manually.

expression1: value >= upper - page_size - offset
This expression returns true if the user has not scrolled vertically back.
The 'offset' is defined as VERTICAL_AUTOSCROLL_TOLERENCE - this means the
limit upto which scroll-on-output will still work even if the user has
manually scrolled.

Similarly HORIZONTAL_AUTOSCROLL_TOLERENCE is defined.
Detection of horizontal scrolling is done by
expression2: value <= HORIZONTAL_AUTOSCROLL_TOLERENCE
HORIZONTAL_AUTOSCROLL_TOLERENCE is set to 3 because the left-margin of text_view is also set at 3.

if (expression1 && expression2) then
        set the vertical adjustment to upper - page_size
        apply this adjustment to the container
Comment 10 sébastien lafargue 2016-12-12 19:31:55 UTC
Attachment 341838 [details] pushed as 60b5efe - Disable 'scroll-on-output' in build log panel
Comment 11 Felix Schwarz 2016-12-12 19:45:45 UTC
Kritarth: Thank you very much for implementing this :-)
Comment 12 Felix Schwarz 2016-12-13 17:25:11 UTC
Hmm, somehow the feature doesn't work as expected. When looking at the builder output I see wild jumps.

I created a short screen cast to illustrate what I'm seeing:
https://www.schwarz.eu/video/2016-12-13-gnome-builder-scrolling.webm

At 0:04 I scroll down to the bottom. At 0:09 there is some new output but you can see that the vertical scroll bar is not at the bottom. This is only one of several effects I observed. Sometimes I also see horizontal jumps and when there is a lot of output it is hardly possible to scroll up (wild jumping up and down).

Should I open a new bug?
Comment 13 Kritarth 2016-12-13 18:01:23 UTC
I think the minor optimization as mentioned in the comments above is causing the issue.
Could you please test another patch that i will upload next?
Comment 14 Kritarth 2016-12-13 18:16:01 UTC
Created attachment 341904 [details] [review]
Correction in patch for disabling scroll-on-output in build log panel

Instead of reusing the old value for vadjustment, now value after
buffer update is used.
Comment 15 sébastien lafargue 2016-12-13 18:21:46 UTC
Felix, can you try with the Kritarth new patch ?
Comment 16 Felix Schwarz 2016-12-13 18:31:34 UTC
I'd love to but currently my Builder builds fail - see posting on the builder mailing list.
Comment 17 Christian Hergert 2016-12-18 00:13:50 UTC
I reverted the patches associated with these because they break the normal scroll-on-output case.

It's really tricky to get that right (I'm not actually sure it's possible to get it right currently, due to the internal animation sequences that textview performs on the adjustments).

What we probably need to do (for 3.24 timeframe) is switch from textview to using VteTerminal (like we do for run output) but without a PTY. Simply use the "feed" API to the terminal to push data from the build log to the terminal widget.

That will probably be dependent on me landing the wip/chergert/pipeline branch, because that will drastically simplify the logging system.
Comment 18 sébastien lafargue 2016-12-18 00:19:22 UTC
then we can use:
vte_terminal_set_scroll_on_output

and add a contextual menu entry with an accelerator.

I keep this bug opened.
Comment 19 Felix Schwarz 2017-11-23 20:51:26 UTC
(In reply to Christian Hergert from comment #17)
> What we probably need to do (for 3.24 timeframe) is switch from textview to
> using VteTerminal (like we do for run output) but without a PTY. Simply use
> the "feed" API to the terminal to push data from the build log to the
> terminal widget.

I just noticed your commits 648824b9 ("buildui: use IdeTerminal for build output"), 3a21268b ("pipeline: add VtePty helper to attach to launcher") and the rest of the series. With these commits can we now implement the missing feature described in this bug?
Comment 20 Christian Hergert 2017-11-24 03:19:55 UTC
Indeed.

Someone can put a patch together that does:

 1) Add new gsetting keys to org.gnome.builder.terminal
 2) Bind the setting in ide-terminal.c to VteTerminal:scroll-on-output
 3) Register a terminal preferences in ide-preferences-builtin.c
Comment 21 Felix Schwarz 2017-12-17 10:24:15 UTC
I checked builder nightly and I have to say I'm inclined to close this bug as "WONTFIX": Builders new terminal works exactly like I wanted it to. By default it scrolls on output but when I scroll up it stops scrolling on output. Once I'm back at the newest messages it restarts scrolling.

So any objections against closing this?
Personally I'd rather try fixing other things in builder which bother me than implementing yet another preference which I don't need.
Comment 22 Christian Hergert 2017-12-17 12:11:06 UTC
Sure