GNOME Bugzilla – Bug 785801
UI: transition model disconnected from the sidebar switcher
Last modified: 2018-04-13 15:06:25 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.
> 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?
(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.
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.
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.
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.
This updated version also updates the buttons at the sidebar, which were missing in the initial implementation.
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.
Review of attachment 359714 [details] [review]: It's looks good. Based on your patch, I did next patch which adds an animation.
(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?
Created attachment 360787 [details] screencast with animation
(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..
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. ;-)
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.
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.
- adwaita-dark.css changes dropped. - default transition time is 400 - Animating logic is decoupled to new AnimatedScrolledWindow It is ok?
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.
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.
Review of attachment 364564 [details] [review]: now rebased.
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.
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.
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.
(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].
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