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 785801 - UI: transition model disconnected from the sidebar switcher
UI: transition model disconnected from the sidebar switcher
Status: RESOLVED FIXED
Product: gnome-usage
Classification: Other
Component: general
unspecified
Other Linux
: High critical
: ---
Assigned To: GNOME Usage maintainer(s)
GNOME Usage maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2017-08-04 08:36 UTC by Jakub Steiner
Modified: 2018-04-13 15:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
performance-view: Merge views into scrolled window (5.86 KB, patch)
2017-09-12 13:28 UTC, Felipe Borges
none Details | Review
performance-view: Merge views into scrolled window (6.26 KB, patch)
2017-09-13 12:43 UTC, Felipe Borges
none Details | Review
performance-view: Add scrolling animation (7.52 KB, patch)
2017-10-02 14:25 UTC, Petr Štětka
none Details | Review
screencast with animation (1.39 MB, video/webm)
2017-10-02 15:04 UTC, Petr Štětka
  Details
performance-view: Merge views into scrolled window (6.16 KB, patch)
2017-11-24 16:15 UTC, Petr Štětka
none Details | Review
performance-view: Add scrolling animation (9.00 KB, patch)
2017-11-24 16:15 UTC, Petr Štětka
none Details | Review
performance-view: Merge views into scrolled window (6.22 KB, patch)
2017-11-28 13:38 UTC, Felipe Borges
none Details | Review
graph-stack-switcher: Change graphSwitcherButton to GtkRadioButton (4.93 KB, patch)
2018-01-15 15:23 UTC, Petr Štětka
committed Details | Review
performance-view: Merge views into scrolled window (6.17 KB, patch)
2018-01-15 15:23 UTC, Petr Štětka
committed Details | Review
performance-view: Add scrolling animation (5.85 KB, patch)
2018-01-15 15:23 UTC, Petr Štětka
committed Details | Review

Description Jakub Steiner 2017-08-04 08:36:17 UTC
There is a neat transition between the CPU and memory views that unfortunately creates a model of a single view that isn't reflected by the view switcher on the sidebar.

Either we should have the CPU + memory views a single scrolled view where the sidebar acts like an anchor, or we drop the sliding transition in favor of blends or nothing at all.
Comment 1 Petr Štětka 2017-08-07 11:59:46 UTC
> Either we should have the CPU + memory views a single scrolled view where the sidebar acts like an anchor, or we drop the sliding transition in favor of blends or nothing at all.

That might be nice. But I think ScrolledWindow will be quite long.

So, what is your suggestion?
Comment 2 Felipe Borges 2017-09-12 13:28:19 UTC
(In reply to Petr Štětka from comment #1)
> That might be nice. But I think ScrolledWindow will be quite long.

Actually it practically feels the same. See the patch bellow.
Comment 3 Felipe Borges 2017-09-12 13:28:35 UTC
Created attachment 359627 [details] [review]
performance-view: Merge views into scrolled window

Usage has a GtkStack with up/down transitions that communicate a
single page behavior anchored by the thumbnails in the sidebar.

Instead of using a GtkStack we can manually scroll a single view
and truly give a single page experience.
Comment 4 Felipe Borges 2017-09-12 13:29:20 UTC
Review of attachment 359627 [details] [review]:

Petr, if you decide to go with this approach (patch), you should work on a second patch to animate the scrolling.
Comment 5 Felipe Borges 2017-09-13 12:43:03 UTC
Created attachment 359714 [details] [review]
performance-view: Merge views into scrolled window

Usage has a GtkStack with up/down transitions that communicate a
single page behavior anchored by the thumbnails in the sidebar.

Instead of using a GtkStack we can manually scroll a single view
and truly give a single page experience.
Comment 6 Felipe Borges 2017-09-13 12:43:47 UTC
This updated version also updates the buttons at the sidebar, which were missing in the initial implementation.
Comment 7 Petr Štětka 2017-10-02 14:25:42 UTC
Created attachment 360776 [details] [review]
performance-view: Add scrolling animation

When we click on the buttons at the sidebar it should scroll to view
position with animation.
Comment 8 Petr Štětka 2017-10-02 14:29:45 UTC
Review of attachment 359714 [details] [review]:

It's looks good. Based on your patch, I did next patch which adds an animation.
Comment 9 Felipe Borges 2017-10-02 14:30:31 UTC
(In reply to Petr Štětka from comment #8)
> Review of attachment 359714 [details] [review] [review]:
> 
> It's looks good. Based on your patch, I did next patch which adds an
> animation.

That's great. Can you attach a quick screencast so the designers can check it out?
Comment 10 Petr Štětka 2017-10-02 15:04:54 UTC
Created attachment 360787 [details]
screencast with animation
Comment 11 Petr Štětka 2017-10-02 15:09:22 UTC
(In reply to Petr Štětka from comment #8)
> That's great. Can you attach a quick screencast so the designers can check
> it out?

Yes, I added screencast.


However, I think that before we can push it to master, some things should be fixed or explored:

1) Default height of single view or minimum height of view

2) When refreshing the list of processes (every x sec.) will change the height of view and it moves next view up or down. We should probably add an anchor, or set the fixed height of the list of processes or something similar to solve this..
Comment 12 Felipe Borges 2017-10-30 15:36:54 UTC
Review of attachment 360776 [details] [review]:

Thanks for your patch!

Please drop the adwaita-dark.css changes.

::: src/graph-stack-switcher.vala
@@ +78,3 @@
             this.sub_views[button_number].get_allocation(out alloc);
 
+            const uint DURATION = 200;

I think GtkStack has a default transition time of 400. Lets stick to that.

@@ +80,3 @@
+            const uint DURATION = 200;
+            var clock = get_frame_clock();
+            int64 start_time = clock.get_frame_time();

Instead of implementing this whole logic here, it should be decoupled in a new GtkScrolledWindow class. Make a AnimatedScrolledWindow class (in a new file) which does animations by default. ;-)
Comment 13 Petr Štětka 2017-11-24 16:15:20 UTC
Created attachment 364340 [details] [review]
performance-view: Merge views into scrolled window

Usage has a GtkStack with up/down transitions that communicate a
single page behavior anchored by the thumbnails in the sidebar.

Instead of using a GtkStack we can manually scroll a single view
and truly give a single page experience.
Comment 14 Petr Štětka 2017-11-24 16:15:31 UTC
Created attachment 364341 [details] [review]
performance-view: Add scrolling animation

When we click on the buttons at the sidebar it should scroll to view
position with animation.
Comment 15 Petr Štětka 2017-11-24 16:18:14 UTC
- adwaita-dark.css changes dropped.
- default transition time is 400
- Animating logic is decoupled to new AnimatedScrolledWindow

It is ok?
Comment 16 Felipe Borges 2017-11-28 13:30:43 UTC
Review of attachment 364341 [details] [review]:

Thank you for your patch! It works perfectly here, I just have some code legibility comments that you should address.

::: src/animated-scrolled-window.vala
@@ +7,3 @@
+        public void animated_scroll_vertically (int y)
+        {
+            const uint DURATION = 400;

constants can be class members instead (move it out of the method).

::: src/graph-stack-switcher.vala
@@ +36,3 @@
+            this.scrolled_window = scrolled_window;
+
+            scrolled_window.get_vadjustment().value_changed.connect(on_scroll_changed);

Introduce a "scroll_changed" signal in the AnimatedScrolledWindow object. This way the GraphStackSwitcher object doesn't need to be aware of any GtkAdjustment specific logic.

@@ +48,3 @@
+
+                button.clicked.connect(() => {
+                    var button_number = get_button_number(button);

In another commit (before this one), you could change the GraphSwitcherButton to be a GtkRadioButton.

With the GtkRadioButton you can benefit from its "grouping" capabilities to make sure that only one is active/toggled at the time.
Comment 17 Felipe Borges 2017-11-28 13:38:50 UTC
Created attachment 364564 [details] [review]
performance-view: Merge views into scrolled window

Usage has a GtkStack with up/down transitions that communicate a
single page behavior anchored by the thumbnails in the sidebar.

Instead of using a GtkStack we can manually scroll a single view
and truly give a single page experience.
Comment 18 Felipe Borges 2017-11-28 13:39:20 UTC
Review of attachment 364564 [details] [review]:

now rebased.
Comment 19 Petr Štětka 2018-01-15 15:23:32 UTC
Created attachment 366836 [details] [review]
graph-stack-switcher: Change graphSwitcherButton to GtkRadioButton

With GtkRadioButton we can benefit from its "grouping" capabilities
to make sure that only one is active/toggled at the time.
Comment 20 Petr Štětka 2018-01-15 15:23:48 UTC
Created attachment 366837 [details] [review]
performance-view: Merge views into scrolled window

Usage has a GtkStack with up/down transitions that communicate a
single page behavior anchored by the thumbnails in the sidebar.

Instead of using a GtkStack we can manually scroll a single view
and truly give a single page experience.
Comment 21 Petr Štětka 2018-01-15 15:23:56 UTC
Created attachment 366838 [details] [review]
performance-view: Add scrolling animation

When we click on the buttons at the sidebar it should scroll to view
position with animation.
Comment 22 Petr Štětka 2018-01-15 15:28:53 UTC
(In reply to Felipe Borges from comment #16)
> Review of attachment 364341 [details] [review] [review]:
> 
> Thank you for your patch! It works perfectly here, I just have some code
> legibility comments that you should address.

Thanks for review, I updated patches according to your suggestions.

> ::: src/animated-scrolled-window.vala
> @@ +7,3 @@
> +        public void animated_scroll_vertically (int y)
> +        {
> +            const uint DURATION = 400;
> 
> constants can be class members instead (move it out of the method).

Done.

> ::: src/graph-stack-switcher.vala
> @@ +36,3 @@
> +            this.scrolled_window = scrolled_window;
> +
> +           
> scrolled_window.get_vadjustment().value_changed.connect(on_scroll_changed);
> 
> Introduce a "scroll_changed" signal in the AnimatedScrolledWindow object.
> This way the GraphStackSwitcher object doesn't need to be aware of any
> GtkAdjustment specific logic.

Done. 

> 
> @@ +48,3 @@
> +
> +                button.clicked.connect(() => {
> +                    var button_number = get_button_number(button);
> 
> In another commit (before this one), you could change the
> GraphSwitcherButton to be a GtkRadioButton.
> 
> With the GtkRadioButton you can benefit from its "grouping" capabilities to
> make sure that only one is active/toggled at the time.

Done, attachment 366836 [details] [review].
Comment 23 Felipe Borges 2018-04-13 15:06:09 UTC
Thank you for your patches!

Attachment 366836 [details] pushed as f44dc4b - graph-stack-switcher: Change graphSwitcherButton to GtkRadioButton
Attachment 366837 [details] pushed as bb25a38 - performance-view: Merge views into scrolled window
Attachment 366838 [details] pushed as 3df591c - performance-view: Add scrolling animation