GNOME Bugzilla – Bug 775642
unable to disable "scroll on output" in build output panel
Last modified: 2017-12-17 12:11:06 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.
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
Hi, This is my first patch so please guide me how I can improve this further. Thanks
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.
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
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);
(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.
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 ?
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.
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
Attachment 341838 [details] pushed as 60b5efe - Disable 'scroll-on-output' in build log panel
Kritarth: Thank you very much for implementing this :-)
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?
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?
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.
Felix, can you try with the Kritarth new patch ?
I'd love to but currently my Builder builds fail - see posting on the builder mailing list.
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.
then we can use: vte_terminal_set_scroll_on_output and add a contextual menu entry with an accelerator. I keep this bug opened.
(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?
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
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.
Sure