GNOME Bugzilla – Bug 688473
Clean up ui_state_changed
Last modified: 2016-03-31 13:57:13 UTC
I really dislike how tangled our control flow is due to the various ui_state_changed handlers spread all over the place. This series tries to clean this up by grouping related code in the same class, and making the ui_state_changed implementations smaller and have more "local" behaviour.
Created attachment 229145 [details] [review] Properties: Avoid connecting to stats multiple times populate() can now be called multiple times, via the refresh_properties signal. This moves the stats_updated call to ui_state_changed so that it only happens once.
Created attachment 229146 [details] [review] Simplify status tracking Instead of signal connections we just bind properties. This also centralizes the status handling to App.vala.
Created attachment 229147 [details] [review] App: minor ui_state_changed cleanup Move the set_main_ui_state() call out of the switch as its really only depending on != DISPLAY, and duplicating it in all cases is not very nice.
Created attachment 229148 [details] [review] App.ui_state_changed: move searchbar hiding out of switch
Created attachment 229149 [details] [review] Move most App.current_item.actor stuff to App from CollectionView Its really confusion how this actor is modified from multiple files. current_item is a member of App, lets manage it from there. This just copied the code over and minimally modifies it to work. Further commits will clean up the code.
Created attachment 229150 [details] [review] Simplify actor fading in ui_state_changed implementation These are all of the form if in the right state, fade in, otherwise fade out. No need to use the switches for this.
Created attachment 229151 [details] [review] CollectionView.ui_state_changed remove switch No need for the big switch anymore.
Created attachment 229152 [details] [review] Add allocate_actor_noanim herlper
Created attachment 229153 [details] [review] App: Move shared code into position_item_actor_at_icon helper
Created attachment 229154 [details] [review] Cleanup ui_state_changed implementations Now that many of these are pretty small we can simplify them a bit. For instance, most of them don't really need full switches on the state as they just have one or two cases that are handled, which is an excellent usecase for "if".
Review of attachment 229145 [details] [review]: Looks good otherwise. ::: src/properties.vala @@ -304,2 +298,4 @@ switch (ui_state) { case UIState.PROPERTIES: + var libvirt_machine = App.app.current_item as LibvirtMachine; + if (libvirt_machine != null) { don't remember what was the agreement last we discussed this but I prefer more explicit: if (App.app.current_item is LibvirtMachine) { var libvirt_machine = App.app.current_item as LibvirtMachine; .... } I know is more verbose but not so much as it makes things more clear/readable.
Review of attachment 229146 [details] [review]: Looks good
Review of attachment 229147 [details] [review]: ACK
Review of attachment 229148 [details] [review]: ACK ::: src/app.vala @@ +560,3 @@ set_main_ui_state (); + if (ui_state != UIState.COLLECTION) this means that we do this now also for DISPLAY state. I guess that intentional but you might want to mention this in the commit log.
Review of attachment 229149 [details] [review]: ACK, though App seems to be getting very bloated. Hopefully we can clean it later.
Review of attachment 229150 [details] [review]: ACK
Review of attachment 229151 [details] [review]: ACK
Review of attachment 229152 [details] [review]: herlper -> helper in the commit log Looks good otherwise. ::: src/app.vala @@ +771,3 @@ + allocate_actor_noanim (actor, x, y, + Machine.SCREENSHOT_WIDTH, + Machine.SCREENSHOT_WIDTH * 2); I guess its a c&p mistake and last 2 arguments were supposed to be: x + Machine.SCREENSHOT_WIDTH, y + Machine.SCREENSHOT_HEIGHT * 2 ? ::: src/util-app.vala @@ +100,3 @@ + // This can be used to ensure a specific initial allocation for a previously unallocated actor + public void allocate_actor_noanim (Clutter.Actor actor, dont see the need to shorten "no_animation" in the name but no strong feelings. @@ -100,1 +100,5 @@ + // This can be used to ensure a specific initial allocation for a previously unallocated actor + public void allocate_actor_noanim (Clutter.Actor actor, + float x, float y, + float w, float h) { w -> width, h -> height
Review of attachment 229153 [details] [review]: Looks like nice refactoing otherwise.. ::: src/app.vala @@ +554,3 @@ + // We temporarily set the duration to 0 because we don't want to animate + // fixed_x/y, but rather immidiately set it to the target value and then + // animate actor.allocation which is set based on these. where do we 'animate actor.allocation' ?
Review of attachment 229154 [details] [review]: what did switches ever do to you. :)
Review of attachment 229153 [details] [review]: ::: src/app.vala @@ +554,3 @@ + // We temporarily set the duration to 0 because we don't want to animate + // fixed_x/y, but rather immidiately set it to the target value and then + // animate actor.allocation which is set based on these. During the layout phase the thing affecting size and position (like min_width, fixed_x/y, etc) will be used to calculate a final allocation of the actor which will be applied to the actor, just like say GtkWidget.size_allocate(). It is this rectangle we want to interpolate from the initial state to the destination state, not the individual input components of they layout system. The later will not result in a correct animation.
Created attachment 229347 [details] [review] App.ui_state_changed: move searchbar hiding out of switch This also sets searchbar state for the DISPLAY case, which should not cause any change in behaviour.
Created attachment 229348 [details] [review] Move most App.current_item.actor stuff to App from CollectionView Its really confusion how this actor is modified from multiple files. current_item is a member of App, lets manage it from there. This just copied the code over and minimally modifies it to work. Further commits will clean up the code.
Created attachment 229349 [details] [review] Simplify actor fading in ui_state_changed implementation These are all of the form if in the right state, fade in, otherwise fade out. No need to use the switches for this.
Created attachment 229350 [details] [review] CollectionView.ui_state_changed remove switch No need for the big switch anymore.
Created attachment 229351 [details] [review] Add allocate_actor_noanim herlper
Created attachment 229352 [details] [review] App: Move shared code into position_item_actor_at_icon helper
Created attachment 229353 [details] [review] Cleanup ui_state_changed implementations Now that many of these are pretty small we can simplify them a bit. For instance, most of them don't really need full switches on the state as they just have one or two cases that are handled, which is an excellent usecase for "if".
I updated all the patches wrt to the reviews except comment 11. I don't think the verboser version is more readable. I'm perfectly willing to change it, but i don't want to do so on a "random" choice based on who is reviewing. Seems we need to either: a) not really care which way it is or b) agree in the group which way we want and add it to the styleguide
Teuf also agreed (on irc) that the more explicit version is better. Thats at least 50% for the explict version, so I think we should always use that even if elmarco also agrees with me, since it seems to be the least likely to surprise people. I'll fix this patch and come up with a CodingStyle.txt patch.
Attachment 229145 [details] pushed as eb9eed6 - Properties: Avoid connecting to stats multiple times Attachment 229146 [details] pushed as 1d003ef - Simplify status tracking Attachment 229147 [details] pushed as 180a3f1 - App: minor ui_state_changed cleanup Attachment 229347 [details] pushed as f5c802f - App.ui_state_changed: move searchbar hiding out of switch Attachment 229348 [details] pushed as 102a826 - Move most App.current_item.actor stuff to App from CollectionView Attachment 229349 [details] pushed as 6281117 - Simplify actor fading in ui_state_changed implementation Attachment 229350 [details] pushed as b53aa59 - CollectionView.ui_state_changed remove switch Attachment 229351 [details] pushed as ef50d47 - Add allocate_actor_noanim herlper Attachment 229352 [details] pushed as 804c502 - App: Move shared code into position_item_actor_at_icon helper
Ugh, when i commited this i accidentally squashed the last 2 patches together. Sorry about that.