GNOME Bugzilla – Bug 745691
Appropriate actions in popup
Last modified: 2016-09-20 08:16:03 UTC
As discussed between me, Alberto Ruiz and Allan Day, we should remove: * Pause * Delete from the actions popup when shown in display mode (VM in foreground) and rather provide a more appropriate action: * Restart for libvirt VMs only. This will first try to restart the box gracefully and on failure, it will show the usual "Do you want to force shutdown".
I'm guessing a libvirt VM is qualified as such because of the API used to manage the hypervisor on the host machine (libvirt). Which other such APIs are used by gnome-boxes and how can you switch between them ?
(In reply to Visarion Alexandru from comment #1) > I'm guessing a libvirt VM is qualified as such because of the API used to > manage the hypervisor on the host machine (libvirt). Which other such APIs > are used by gnome-boxes and how can you switch between them ? Don't really understand what you mean but I think you might have misunderstood the bug. I'll attach screenshots of the popover in two different contexts that should clear it up.
Created attachment 324180 [details] Screenshot of popover menu in overview
Created attachment 324181 [details] Screenshot of popover menu in display mode
I read in your bug report that the fix must be made only for libvirt VMs. I was asking if there are other types of VMs and how could I install one. Also, the popover menu in display mode doesn't have both "Pause" and "Delete", only "Pause". So basically it narrows down to replacing "Pause" with "Restart"?
I think I answered my first question. The remote machines are the ones which shouldn't be affected by this fix, right?
There is also oVirt machines but i don't think we support any management operations (deletion, pause etc) on them.
Created attachment 324362 [details] [review] patch v1 *removed "Pause" and "Delete" from the popover menu in overview mode and added "Restart", for the machines that are vurrently running *removed "Pause" from the popover menu in display mode I made two changes because I wasn't sure which was the desired behaviour (my last question regarding this remained unanswered). Which should I keep?
small edit :*removed "Pause" from the popover menu in display mode and added "Restart"
(In reply to Visarion Alexandru from comment #8) > Created attachment 324362 [details] [review] [review] > patch v1 The patch is not in correct format. You need to commit changes locally and then either use `git-format-patch` and upload manually or make use of git-bz tool to upload the patches. > *removed "Pause" and "Delete" from the popover menu in overview mode and > added "Restart", for the machines that are vurrently running * In overview mode, these two make sense so we don't want to remove them. * While deletion of Pause and Delete might be considered the same logical change, additions of Restart is not and hence should be added in a separate patch. "Each logical change goes into a separate commmit/patch" is the rule to follow here. > *removed "Pause" from the popover menu in display mode That's good. > I made two changes because I wasn't sure which was the desired behaviour (my > last question regarding this remained unanswered). Which is? I thought you answered one of your questions yourself and then I answered the other.
Created attachment 324560 [details] [review] Replace "Pause" with "Restart" in the popup action, display mode
We both answered the same question. I guess I should have better structured replies xD. I think it's OK now.
Review of attachment 324560 [details] [review]: From previous comment: "* While deletion of Pause and Delete might be considered the same logical change, additions of Restart is not and hence should be added in a separate patch. "Each logical change goes into a separate commmit/patch" is the rule to follow here." ::: src/actions-popover.vala @@ +55,3 @@ } // Pause This comment will need to be moved into the if block below. @@ +57,2 @@ // Pause + if (window.ui_state != UIState.DISPLAY) { we already have a block for this below, re-use that instead. @@ +61,3 @@ + action.set_enabled (machine.can_save); + } + if(window.ui_state == UIState.DISPLAY && machine is LibvirtMachine) { * or you could simply add an 'else if' block to the 'if' below and have only one condition in it. * We've been trying to avoid this 'machine is LibvirtMachine' pattern so I'd prefer addition of a boolean 'can_restart' (perhaps virtual) property in Machine instead. @@ +64,3 @@ + section.append (_("Restart"), "box.restart"); + var action = action_group.lookup_action ("restart") as GLib.SimpleAction; + } wrongly indented. @@ +106,3 @@ + private void restart_activated() { + var machine = window.current_item as Machine; + machine.restart(); no need for machine variable here. (window.current_item as Machine).restart() would do.
Created attachment 324632 [details] [review] Remove display mode popup action "Pause"
Created attachment 324664 [details] [review] Remove display mode popup action "Pause" *removed trailing whitespaces
Created attachment 324668 [details] [review] Add display mode popup action "Restart"
Review of attachment 324664 [details] [review]: Patch itself is good but in shortlog * Space after ':' * "inappropriate" is redundant. It's more a justification of the change, which you can keep for description part. * "popup action" part is also redundant, prefix makes this part of the context clear enough for a shortlog. * doesn't say the important bit: "from display mode". and description calls the action inappropriate but doesn't explain why it's inappropriate. Also the wording makes it sound like option is inappropriate in general but is just being removed from display mode.
Review of attachment 324668 [details] [review]: Some of the points on shortlog in previous patch, also apply here so I wont repeat them. Description should be describing the change in English, not translating the code/implementation details into English. Summarize the problem/rationale and the fix. ::: src/actions-popover.vala @@ +109,3 @@ + private void restart_activated () { + (window.current_item as Machine).restart(); space before (. ::: src/machine.vala @@ +17,2 @@ public virtual bool can_save { get { return false; } } + public virtual bool can_restart { get { return false; } } Addition of this property (and it's override in subclass) is a separate change so you should put that in a separate patch.
Created attachment 324838 [details] [review] Remove Pause from the popover-menu, in display-mode I didn't want to speculate on the reasons of the change, so I just said it is because of the result of the discussion you had with the other developers
Created attachment 324839 [details] [review] Add virtual property "can_restart"
Created attachment 324840 [details] [review] Add "Restart" in the display-mode's popover-menu
Review of attachment 324838 [details] [review]: * We are not removing "Pause" for replacing it with "Restart", they are both independent changes as I said before. * "as discussed between Zeeshan Ali,Alberto Ruiz and Allan Day" part isn't needed. Bug URL is enough. ::: src/actions-popover.vala @@ -53,1 @@ action.set_enabled (machine.is_running); I don't get it. What's the change here?
Review of attachment 324839 [details] [review]: ::: src/libvirt-machine.vala @@ +32,3 @@ public override bool suspend_at_exit { get { return connection == App.app.default_connection && !run_in_bg; } } public override bool can_save { get { return !saving && state != MachineState.SAVED; } } + public override bool can_restart { get { return state == MachineState.RUNNING; } } If you look at restart(), you'll see that we can also handle MachineState.SAVED. ::: src/machine.vala @@ +17,2 @@ public virtual bool can_save { get { return false; } } + public virtual bool can_restart { get { return false; } } Actually since restart() is abstract, it's best this associated property should be too. Then when restart() is actually implemented in RemoteMachine and OvirtMachine subclasses, the value of can_restart property is also updated at the same time in those classes and in base case we don't assume anything. This will obviously mean you need to implement this property in each subclass but that's fine.
Review of attachment 324839 [details] [review]: Apart from that, some nitpicks about commit log as usual: * Shortlog isn't saying the most important bit (what property?) and hence way to vague. * Description lines are max 74 chars so you have a lot more space in each line than you're using. * Please always add an empty line before a new paragraph (Remember it's normal English), although I don't think the second sentence needs to be a new para (i-e no need to break it on next line).
Review of attachment 324840 [details] [review]: As I said, "restart" is not a replacement for "pause", these are different operations and have nothing to do with each other. I'm marking as commit_now cause I could fix it while pushing but since you'll be update the other patches anyway, you could fix the log at the same time.
(In reply to Zeeshan Ali (Khattak) from comment #22) > Review of attachment 324838 [details] [review] [review]: > ::: src/actions-popover.vala > @@ -53,1 @@ > action.set_enabled (machine.is_running); > > I don't get it. What's the change here? I may be mistaken, but it doesn't appear to be any change. These are only the code lines just above a removal of lines. Either that or it is because I configured my IDE to automatically remove trailling whitespaces and convert tabs into spaces.
Created attachment 325173 [details] [review] Remove display mode popup action "Pause" I just relocated the code for the "Pause" action, so that it's only available when not in display mode. * fixed log
Created attachment 325174 [details] [review] Add abstract property "can restart" Patch 2/3
Created attachment 325175 [details] [review] Add "Restart" in the display-mode's popover-menu * fixed log Inter-dependent with the first "Remove Pause from display mode" patch. Patch 3/3
(In reply to Visarion Alexandru from comment #26) > (In reply to Zeeshan Ali (Khattak) from comment #22) > > Review of attachment 324838 [details] [review] [review] [review]: > > > ::: src/actions-popover.vala > > @@ -53,1 @@ > > action.set_enabled (machine.is_running); > > > > I don't get it. What's the change here? > > I may be mistaken, but it doesn't appear to be any change. These are only > the code lines just above a removal of lines. Either that or it is because I > configured my IDE to automatically remove trailling whitespaces and convert > tabs into spaces. Don't worry, it seems like a bug in Splinter: https://bugzilla.gnome.org/show_bug.cgi?id=764652 .
Let me try to workaround that annoying bz bug: (In reply to Visarion Alexandru from comment #27) > Created attachment 325173 [details] [review] [review] > Remove display mode popup action "Pause" > > I just relocated the code for the "Pause" action, so that it's only > available when not in display mode. > * fixed log > @@ -, +, @@ > src/actions-popover.vala | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > --- a/src/actions-popover.vala > +++ a/src/actions-popover.vala > @@ -53,12 +53,11 @@ public void update_for_item (CollectionItem item) { > action.set_enabled (machine.is_running); > } > - // Pause > - section.append (_("Pause"), "box.pause"); > - var action = action_group.lookup_action ("pause") as GLib.SimpleAction; > - action.set_enabled (machine.can_save); > - Why remove this empty line? > if (window.ui_state != UIState.DISPLAY) { > + // Pause > + section.append (_("Pause"), "box.pause"); > + var action = action_group.lookup_action ("pause") as GLib.SimpleAction; > + action.set_enabled (machine.can_save); Empty line here would be nice. (In reply to Visarion Alexandru from comment #28) > Created attachment 325174 [details] [review] [review] > Add abstract property "can restart" Name of property is can_restart and since it has no spaces in the name, you could just omit "". > > Patch 2/3 This information isn't useful (it's only on mailing-list) and would have been nice to have the commit log description in here instead since that changed. "Make it true for a libvirt machine which is in running state or in saved state." * is implied in the second para. * "Set to true by classname" would be better way of putting it IMO. > @@ -, +, @@ > src/libvirt-machine.vala | 1 + > src/machine.vala | 1 + > src/ovirt-machine.vala | 2 ++ > src/remote-machine.vala | 2 ++ > 4 files changed, 6 insertions(+) > --- a/src/libvirt-machine.vala > +++ a/src/libvirt-machine.vala > @@ -31,6 +31,7 @@ > public override bool suspend_at_exit { get { return connection == App.app.default_connection && !run_in_bg; } } > public override bool can_save { get { return !saving && state != MachineState.SAVED; } } > + public override bool can_restart { get { return state == MachineState.RUNNING || state == MachineState.SAVED; } } 'override' is redundant for abstract properties. > --- a/src/ovirt-machine.vala > +++ a/src/ovirt-machine.vala > @@ -6,6 +6,8 @@ > private Ovirt.Vm vm; > private Ovirt.Proxy proxy; > + public override bool can_restart { get { return false; } } * No 'override' needed here either. * public properties are listed before private and please keep an empty line between them. > --- a/src/remote-machine.vala > +++ a/src/remote-machine.vala > @@ -3,6 +3,8 @@ > private class Boxes.RemoteMachine: Boxes.Machine, Boxes.IPropertiesProvider { > + public override bool can_restart { get { return false; } } Same here.
(In reply to Visarion Alexandru from comment #29) > Created attachment 325175 [details] [review] [review] > Add "Restart" in the display-mode's popover-menu > > * fixed log > > Inter-dependent with the first "Remove Pause from display mode" patch. > Patch 3/3 Same comment about keeping the log around if changed. > @@ -, +, @@ > src/actions-popover.vala | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > --- a/src/actions-popover.vala > +++ a/src/actions-popover.vala > @@ -7,7 +7,8 @@ > {"pause", pause_activated}, > {"force_shutdown", force_shutdown_activated}, > {"delete", delete_activated}, > - {"properties", properties_activated} > + {"properties", properties_activated}, > + {"restart", restart_activated} > }; > private AppWindow window; > @@ -62,6 +63,9 @@ public void update_for_item (CollectionItem item) { > section.append (_("Delete"), "box.delete"); > action = action_group.lookup_action ("delete") as GLib.SimpleAction; > action.set_enabled (machine.can_delete); > + } else if (machine.can_restart) { > + section.append (_("Restart"), "box.restart"); > + var action = action_group.lookup_action ("restart") as GLib.SimpleAction; > } > menu.append_section (null, section); > @@ -103,6 +107,10 @@ private void force_shutdown_activated () { > machine.force_shutdown (); > } Won't hurt to have an empty line here. :) > + private void restart_activated () { > + (window.current_item as Machine).restart (); > + } > + > private void delete_activated () { > window.set_state (UIState.COLLECTION); Looks good otherwise.
Created attachment 325633 [details] [review] Remove display mode popup action "Pause" * empty line added I remove an empty line in this patch and in the previous because, when I relocate the code, two consecutive empty lines are left. (although it is weird that the remaining empty line can be seen when clicking on "details" but can't be seen when clicking on the attachment)
Created attachment 325634 [details] [review] Add abstract property can restart. Add an abstract property which symbolises the property of a machine to be able to be restarted. Set to true by class name. Make the can_restart property false for all the machine types that can not be restarted (all, except libvirt).
As I told you, I couldn't follow your advice of removing the keyword "override", because vala wouldn't let me.
Created attachment 325636 [details] [review] Add "Restart" in display mode, Add a Restart action in the display mode popover-menu. Again, some issues with empty lines. There is an empty line where you pointed, actually (just before the newly added restart_activated method? ).
(In reply to Visarion Alexandru from comment #36) > Created attachment 325636 [details] [review] [review] > Add "Restart" in display mode, > > Add a Restart action in the display mode popover-menu. > > > Again, some issues with empty lines. There is an empty line where you > pointed, actually (just before the newly added restart_activated method? ). Same for this patch/bug, if you are uploading patches manually, please do not edit the patches outside git (i-e after git-format-patch, you should only be uploading them, as is).
Created attachment 326171 [details] [review] actions-popover: Remove "Pause" from display mode Remove "Pause" action from the popover menu, in display mode.
Created attachment 326172 [details] [review] machine: Add can_restart abstract property Add an abstract property which symbolises the property of a machine to be able to be restarted. Set to true by class name. Make the can_restart property false for all the machine types that can not be restarted (all, except libvirt).
Created attachment 326173 [details] [review] actions-popover: Add "Restart" in display mode Add a Restart action in the display mode popover-menu.
Review of attachment 326171 [details] [review]: ACK
Review of attachment 326172 [details] [review]: Whey I suggested "Set to true by class name", I expected you to replace 'class name' by the appropriate name of the class and not just copy&paste as is. Please keep in mind that point of these reviews is not to satisfy me to get the patches in, but to ensure quality and to guide you to think critically about the code so in future, your patches do not need a lot of changes before they can be merged. So please think about each comment in the review and if something doesn't make sense to you, please discuss. Also I said it's made redundant by second para so you either want this sentence or the second para, but not both. I'd just keep the second para and remove this. ::: src/remote-machine.vala @@ +4,2 @@ private class Boxes.RemoteMachine: Boxes.Machine, Boxes.IPropertiesProvider { nitpick: No need for this empty line here.
Review of attachment 326173 [details] [review]: Looks good.
Created attachment 326380 [details] [review] machine: Add can_restart abstract property Add an abstract property which symbolises the property of a machine to be able to be restarted. Make the can_restart property false for all the machine types that can not be restarted (all, except libvirt).
Review of attachment 326380 [details] [review]: ::: src/remote-machine.vala @@ +4,2 @@ private class Boxes.RemoteMachine: Boxes.Machine, Boxes.IPropertiesProvider { You didn't remove this line but it's ok, i'll do that when pushing.
Review of attachment 326173 [details] [review]: ::: src/actions-popover.vala @@ +109,3 @@ } + private void restart_activated () { You are not hooking it up. Not only this means you did not test this patch but also that you are not paying attention to vala warnings.
Created attachment 326404 [details] [review] actions-popover: Remove "Pause" from display mode Remove "Pause" action from the popover menu, in display mode.
Created attachment 326405 [details] [review] machine: Add can_restart abstract property Add an abstract property which symbolises the property of a machine to be able to be restarted. It's currently false for all the machine types except libvirt.
Created attachment 326406 [details] [review] actions-popover: Add "Restart" in display mode Add a Restart action in the display mode popover-menu.
Review of attachment 326380 [details] [review]: ::: src/remote-machine.vala @@ +4,2 @@ private class Boxes.RemoteMachine: Boxes.Machine, Boxes.IPropertiesProvider { I thought you were talking about the new line I was inserting after "public override bool can_restart { get { return false; } }". Should I put it back ?
Review of attachment 326173 [details] [review]: ::: src/actions-popover.vala @@ +109,3 @@ } + private void restart_activated () { I usually care a lot about warnings, because I know how important they are. Sorry for this mistake. The patch was working, only that the "Restart" button would have appeared when running machines that had the "can_restart" property and wouldn't have appeared otherwise. I now see that the promoted behaviour is for a button to be enabled when running a machine that has a certain property and to be disabled otherwise.
Created attachment 326420 [details] [review] machine: Add can_restart abstract property *fixed empty lines I couldn't make the previous patch obsolete, as it was made by Zeeshan
Created attachment 326421 [details] [review] actions-popover: Add "Restart" in display mode *fixed warning and behaviour issue Same problem about making previous patch obsolete
Attachment 326404 [details] pushed as e0f5ba8 - actions-popover: Remove "Pause" from display mode Attachment 326420 [details] pushed as 664ca02 - machine: Add can_restart abstract property Attachment 326421 [details] pushed as 2e6bf4c - actions-popover: Add "Restart" in display mode