GNOME Bugzilla – Bug 686775
Move force shutdown to properties view
Last modified: 2016-03-31 14:02:31 UTC
As per the latest mockups[1] we should have a force shutdown button in the sidebar in the properties view. This is much easier to find than the weird placement of this in the global app menu. [1] https://raw.github.com/gnome-design-team/gnome-mockups/master/boxes/boxes-properties.png
This design may change if we decide to make it possible for the user to suspend running boxes, dunno if it's worth waiting for design input for that one.
Created attachment 227269 [details] [review] Rename machine to libvirt_machine We will later use the machine name for another use.
Created attachment 227270 [details] [review] Add machine local var This is resued twice and will be used more
Created attachment 227271 [details] [review] Add force shutdown button to properties view
Created attachment 227272 [details] [review] Remove force shutdown from app menu
Review of attachment 227269 [details] [review]: ACK but would be nice to say in the log that you are talking of a local variable (as opposed to the classes).
Review of attachment 227270 [details] [review]: ACK ::: src/properties.vala @@ -142,3 +142,4 @@ notebook.remove_page (-1); - if (App.app.current_item == null) + var machine = App.app.current_item as Machine; + nitpick: I'd remove this line
Review of attachment 227271 [details] [review]: Looks good otherwise. ::: src/properties.vala @@ -144,2 +145,3 @@ var machine = App.app.current_item as Machine; + shutdown_button.sensitive = machine != null && machine is LibvirtMachine; Shouldn't this be setting 'visible' prop instead? @@ -259,1 +262,5 @@ + shutdown_button = new Button.with_label (_("Force Shutdown")); + shutdown_button.clicked.connect ( () => { + var machine = App.app.current_item as LibvirtMachine; + if (machine != null) 1. I prefer explicit type checking instead of 'casting and then checking of null': if (App.app.current_item is LibvirtMachine) (App.app.current_item as LibvirtMachine).shutdown (); 2. Since the button can only be pressed for LibvirtMachine, do we need a check here?
Review of attachment 227270 [details] [review]: ::: src/properties.vala @@ -142,3 +142,4 @@ notebook.remove_page (-1); - if (App.app.current_item == null) + var machine = App.app.current_item as Machine; + Well, the next patch inserts something inbetween here.
Review of attachment 227271 [details] [review]: ::: src/properties.vala @@ -144,2 +145,3 @@ var machine = App.app.current_item as Machine; + shutdown_button.sensitive = machine != null && machine is LibvirtMachine; I don't think so, I think its nice that UIs don't relayout themselves base on the content to much, it means things are in the same place all the time and you can learn what features there are, even if they are not always available. @@ -259,1 +262,5 @@ + shutdown_button = new Button.with_label (_("Force Shutdown")); + shutdown_button.clicked.connect ( () => { + var machine = App.app.current_item as LibvirtMachine; + if (machine != null) 1. That is longer to read and less efficient at runtime. I'm not sure its better. 2. We don't, but better safe than sorry.
Comment on attachment 227270 [details] [review] Add machine local var Attachment 227270 [details] pushed as 660314e - Add machine local var
Review of attachment 227272 [details] [review]: ACK
Review of attachment 227271 [details] [review]: ::: src/properties.vala @@ -259,1 +262,5 @@ + shutdown_button = new Button.with_label (_("Force Shutdown")); + shutdown_button.clicked.connect ( () => { + var machine = App.app.current_item as LibvirtMachine; + if (machine != null) 1. More verbose and uglier, I agree but more explicit and therefore more readable. No strong feelings though.
Attachment 227271 [details] pushed as 91d5763 - Add force shutdown button to properties view Attachment 227272 [details] pushed as 7b1ef80 - Remove force shutdown from app menu
Review of attachment 227272 [details] [review]: ::: src/app.vala @@ -88,3 @@ - action_shutdown = new GLib.SimpleAction ("display.shutdown", null); - action_shutdown.activate.connect (() => { (current_item as LibvirtMachine).force_shutdown (); }); - application.add_action (action_shutdown); we could have left the action though, couldn't we?
Review of attachment 227272 [details] [review]: ::: src/app.vala @@ -88,3 @@ - action_shutdown = new GLib.SimpleAction ("display.shutdown", null); - action_shutdown.activate.connect (() => { (current_item as LibvirtMachine).force_shutdown (); }); - application.add_action (action_shutdown); I don't know if that is very useful. Its not accessed from anywhere and you can't really use it from another app as it picks whatever item happens to be current. What do you expect it to be used for?
Review of attachment 227272 [details] [review]: ::: src/app.vala @@ -88,3 @@ - action_shutdown = new GLib.SimpleAction ("display.shutdown", null); - action_shutdown.activate.connect (() => { (current_item as LibvirtMachine).force_shutdown (); }); - application.add_action (action_shutdown); I thought it was exposed on the bus, it could be useful for outside extensions & scripts