GNOME Bugzilla – Bug 677274
Use layout managers to lay out our widgets
Last modified: 2016-03-31 13:55:51 UTC
Boxes is really a pretty traditional UI, with some swooshy animations added. As such it makes a lot more sense to lay it out like a proper UI with the new LayoutManagers. Additionally, the new implicit animations API makes a lot of sense for many of the animations we use. This drops a lot of scattered constraints and and magic numbers making the code easier to understand, and allows us in the future to do things like automatically getting the minimum window size right.
Created attachment 215413 [details] [review] Update to clutter 1.10
Created attachment 215414 [details] [review] Add static global App.app
Created attachment 215415 [details] [review] Add Revealer actor This is an actor that lets you slide in/out a child, clipping the child to the revealer. This is useful for instance to implement sliding in sidebars or notifications.
Created attachment 215416 [details] [review] Add Util.fade_actor helper This makes it easy to fade in/out actors, taking care to correctly hide the actor when completely invibile and making it non-reactive while fading.
Created attachment 215417 [details] [review] Remove use of deprecated actor methods
Created attachment 215418 [details] [review] Use LayoutManagers and implicit animations This rewrites the whole layout and animation setup to use Clutter 1.10 style LayoutManager APIs and implicit animations. It also removes the use of deprecated APIs like ClutterState, etc. This leads to less magic numbers and a more robust layout, as well as niceties like being able to automatically set a minimum size for the window (although this needs more work).
Created attachment 215419 [details] [review] Remove unused pin/unpin utils
Note: This needs latest clutter and clutter-gtk.
Do I clutter from git? I can't click/activate an item in the collection.
(In reply to comment #9) > Do I clutter from git? Yes you need clutter/clutter-gtk from git 15:57 <@alex> Author: Alexander Larsson <alexl@redhat.com> 2012-05-25 15:16:11 15:57 <@alex> that for clutter-gtk 15:57 <@alex> Author: Emmanuele Bassi <ebassi@linux.intel.com> 2012-05-30 15:26:58 15:57 <@alex> that for clutter 1
Created attachment 215626 [details] [review] Enable use-layout-mode This requires latest clutter-gtk and automatically sets up a proper minimum size for the Boxes window.
Created attachment 215628 [details] [review] Linewrap some potentially long labels This helps with e.g. bug 672456
Review of attachment 215413 [details] [review]: ack
Review of attachment 215414 [details] [review]: ack
Review of attachment 215414 [details] [review]: note though that we used to pass App around. So perhaps we should change the rest of the code to use the global..
Review of attachment 215415 [details] [review]: ack
Review of attachment 215416 [details] [review]: ack
Review of attachment 215417 [details] [review]: ack
Review of attachment 215626 [details] [review]: I run clutter/clutter-gtk from git, but that doesn't seem to affect Boxes usage and window size.
marc: Really? Do you have http://git.gnome.org/browse/clutter-gtk/commit/?id=c98d0593d6d382028637dd3dcf285016f84fad72
(In reply to comment #20) > marc: Really? oops, I didn't test with the boxes commit correctly. So indeed, it does resize the window, which isn't all great in window mode. The wizard is very tall here (but again, if it's not resized, then content is not layed out properly) I thought the designers had a rule along: "an application should never resize a window by itself" (if not, I wish they had)
Review of attachment 215418 [details] [review]: ack (there is a few code-style nitpicks here and there, but I don't have an eagle-eye to spot them) ::: src/properties.vala @@ +194,3 @@ + + screenshot_placeholder = new Gtk.Alignment (0.0f, 0.0f, 0.0f, 0.0f); + screenshot_placeholder.set_size_request (180, 130); Yeah, that makes sense. though after this patch the screenshot is still not placed correctly when the windows is resized. That can be addressed later.
Review of attachment 215419 [details] [review]: nice to get rid of that hacks :)
Review of attachment 215626 [details] [review]: not really sure this is what we want for the reason mentioned above: - can resize the window to weird dimensions without user intervention - can force very large window size (when the wizard is fully populated for example)
Review of attachment 215626 [details] [review]: it also causes an extra annoying glitch when the window is shown (in default fullscreen for instance)
Review of attachment 215628 [details] [review]: ack
Review of attachment 215418 [details] [review]: ::: src/properties.vala @@ +194,3 @@ + + screenshot_placeholder = new Gtk.Alignment (0.0f, 0.0f, 0.0f, 0.0f); + screenshot_placeholder.set_size_request (180, 130); Yeah, we need to keep being attached to the size_allocate handler.
Review of attachment 215626 [details] [review]: Yeah, there are still a bunch of layout issues here, but this is things that are normally handled by e.g. gtk+ apps, I don't see anything making it conceptually harder to fix for Clutter code. We just need to make sure that things lay out properly, wrapping and/or ellipsizing, or scrolls as needed. I.E. if we don't want some UI to force the window larger then we need to lay it out such that that part doesn't make the window larger.
I pushed everything but the use-layout-mode patch.
Created attachment 215862 [details] [review] Set names on clutter actors for easier debugging
Created attachment 215863 [details] [review] Remove unused code
Created attachment 215864 [details] [review] Track screenshot position as window size changes This handles the position of the screenshot thumbnail changing. It requires latest clutter version to work. Also, it animates when changing the position, but we can't fix this until we get per-meta animation properties in clutter.
Created attachment 215865 [details] [review] Use static App.app rather than passing around app This is really a global object, use it as such.
Created attachment 215866 [details] [review] Fix warnings if canceling opening vm The timeout was triggering warnings if you clicked back fast enough.
Created attachment 215867 [details] [review] Use transitions-complete signal instead of timeout This is much nicer. Also remove comment about this.
Created attachment 215868 [details] [review] Animate going out of display mode Zoom VM into icon when exiting display mode. Also properly handle the case where going in/out of collection-view/display before animations are fully done.
Created attachment 215869 [details] [review] Use fade_actor in selectionbar This makes the code cleaner, and fixes an issue where sometimes the selection bar would be hidden on animation completion.
Review of attachment 215862 [details] [review]: Looks simple and straight-forward, ACK!
Review of attachment 215863 [details] [review]: ACK otherwise. ::: src/machine.vala @@ -270,1 +269,1 @@ ~MachineActor() { No need to keep the empty destructor.
Review of attachment 215864 [details] [review]: Looks good.
Created attachment 215874 [details] [review] Put the dark background inside clutter We make most main actors have a transparent background, then we add a clutter actor at the bottom of the stage with the dark background. This means we see background everywhere, even e.g. under the sidebar, so that we can e.g. properly scroll it in and have a shadow cast by it.
Review of attachment 215865 [details] [review]: Looks right if it works.
Review of attachment 215866 [details] [review]: ACK!
Review of attachment 215867 [details] [review]: ACK
Review of attachment 215868 [details] [review]: Looks good, assuming you tested it, ACK!
Review of attachment 215869 [details] [review]: You don't exaggerate about 'cleaner' part. :) ACK!
Review of attachment 215874 [details] [review]: Looks good.
Attachment 215862 [details] pushed as 6cf938f - Set names on clutter actors for easier debugging Attachment 215863 [details] pushed as 5080592 - Remove unused code Attachment 215864 [details] pushed as 2a1c9e0 - Track screenshot position as window size changes Attachment 215865 [details] pushed as ab8f334 - Use static App.app rather than passing around app Attachment 215866 [details] pushed as 3b04c11 - Fix warnings if canceling opening vm Attachment 215867 [details] pushed as bc94e4d - Use transitions-complete signal instead of timeout Attachment 215868 [details] pushed as 1ef4810 - Animate going out of display mode Attachment 215869 [details] pushed as 628273a - Use fade_actor in selectionbar Attachment 215874 [details] pushed as 78fdb4e - Put the dark background inside clutter
I noticed 2 bugs since we started this bug: - sometime animation of item thumbnail from collection <-> display doesn't appear (it seems to be item specific, as other items still work when it happens) - when in display state, clicking on global Boxes menu "New" will end up in broken UI state
The first happens once you've once showed the properties, its on my todo list. Will also add the second to TODO list.
Created attachment 216799 [details] [review] Don't show wrong sidebar in collection view We don't show any sidebar in collection view, so there is no need to change the sidebar page. This fixes it temporarily showing the wrong thing when cancelling a new box operation.
Created attachment 216800 [details] [review] Fix "new box" when in DISPLAY mode If you create a new box via the app menu when in display mode we need to remove the current_item to ensure that its not displayed when in WIZARD mode and that its not later zoomed out when we go back to the collection mode.
Review of attachment 216799 [details] [review]: ack
Review of attachment 216800 [details] [review]: ack ::: src/collection-view.vala @@ +80,3 @@ + case UIState.WIZARD: + if (current_item != null) + actor_remove (current_item.actor); doesn't compile as is, but looks reasonable and works when fixed.
(In reply to comment #55) > Created an attachment (id=216800) [details] [review] > Fix "new box" when in DISPLAY mode Btw, it is still buggy from PROPERTIES -> WIZARD
Ugh, i rebased it, the original was based on the patchset in bug 678456, but i didn't test the rebase. That patchset also has a fix for the "sometime animation of item thumbnail from collection <-> display doesn'tappear" bug.
Attachment 216799 [details] pushed as 6a6f3c7 - Don't show wrong sidebar in collection view Attachment 216800 [details] pushed as d3840e1 - Fix "new box" when in DISPLAY mode
I'm closing this as its a bit sprawling. The remaining patch is handled in bug 678960.
> Btw, it is still buggy from PROPERTIES -> WIZARD => bug 679103