GNOME Bugzilla – Bug 734486
Allow running multiple boxes, each in its own window
Last modified: 2016-03-31 13:22:07 UTC
Its about implementing this mockup: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/boxes/wires/multi-window.png This will require quite a bit of changes in code layout, for which we have a separate bug: bug#732098.
Created attachment 283351 [details] [review] searchbar: Ensure window is set before connecting to it Until this patch, window was null when trying to connect to it.
Created attachment 283352 [details] [review] app: Replace AppWindow singleton by List<Boxes.AppWindow> This is needed to make multiple windows possible.
Created attachment 283353 [details] [review] app-window: Closing window removes it from collection Also, use 'this.window' to refer to Topbar's AppWindow in a closure to avoid a Vala bug relative to the AppWindow while destroying the closure. This is needed to make multiple windows possible.
Created attachment 283354 [details] [review] collection-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283355 [details] [review] display-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283356 [details] [review] app-window: Present window when machine selected This presents a machine's window if the machine is selected and is already running in a window, otherwise it creates a new window for the machine. This is needed to make multiple windows possible.
Created attachment 283357 [details] [review] app: Populate new main window's collection This is needed to make multiple windows possible.
Created attachment 283358 [details] [review] selectionbar: Add 'Open in new windows' button This is needed to make multiple windows possible.
Created attachment 283359 [details] [review] Add MachineMenu Implements the machine menu shown in this mockup: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ boxes/wires/multi-window.png This is needed to make multiple windows possible.
Created attachment 283360 [details] [review] display-toolbar: Replace properties button by MachineMenu This is needed to make multiple windows possible.
Created attachment 283361 [details] [review] colection-view: Show MachineMenu on right-click This is needed to make multiple windows possible.
Review of attachment 283351 [details] [review]: I think its a good change but I don't think justification in log is correct. I don't think it was always 'null' at least, maybe it could be 'null' but I don't know if even that is possible right now.
Review of attachment 283352 [details] [review]: I'd write "Replace main_window by list of windows". And you are not replacing a sigleton but a property. ::: src/app.vala @@ +259,2 @@ public bool quit_app () { + foreach (var window in windows) { no need for braces.
Review of attachment 283353 [details] [review]: You also want to mention the diff with existing code in the details. ::: src/app-window.vala @@ +306,3 @@ [GtkCallback] private bool on_delete_event () { + return_if_fail ((current_item == null) || (current_item is Machine)); inner brackets are redundant IMO and makes it less readable. ::: src/app.vala @@ +502,3 @@ } + + public bool remove_app_window (AppWindow window) { _app_ is pretty redundant in the name due to context and argument type. @@ +503,3 @@ + + public bool remove_app_window (AppWindow window) { + var windows_nb = windows.length (); what does nb stands for? coding style is not to use known or nonobvious abbreviations. @@ +510,3 @@ + window.hide (); + windows.remove (window); + remove_window (window); oh I see that you are already using 'remove_window'. Since both take AppWindow argument, adding '_app_' in the name of this function helps avoid the conflict but OTOH its confusing still what exactly is the diff between these two functions. ::: src/machine.vala @@ +505,3 @@ debug("Could not delete screenshot: %s", e.message); } + These changes seem to be about deletion of window on deletion of machine rather than about removal of window from collection on deletion so I think they need to be in separate patch. ::: src/topbar.vala @@ +103,2 @@ window.notify["selection-mode"].connect (() => { + page = this.window.selection_mode ? * Please always add a FIXME comment above work arounds for bugs with a URL to the bug. * This is unrelated to this patch so you know what to do. :)
Created attachment 283448 [details] [review] searchbar: Ensure window is set before connecting to it Until this patch, window was in an uncertain state when trying to connect to it.
Created attachment 283449 [details] [review] app: Replace AppWindow singleton by List<Boxes.AppWindow> This is needed to make multiple windows possible.
Created attachment 283450 [details] [review] app-window: Closing window removes it from collection Also, use 'this.window' to refer to Topbar's AppWindow in a closure to avoid a Vala bug relative to the AppWindow while destroying the closure. This is needed to make multiple windows possible.
Created attachment 283451 [details] [review] machine: Close window on machine deletion If a machine haves a window and the machine is deleted, the window will be closed. This is needed to make multiple windows possible.
Created attachment 283452 [details] [review] collection-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283453 [details] [review] display-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283454 [details] [review] app-window: Present window when machine selected This presents a machine's window if the machine is selected and is already running in a window, otherwise it creates a new window for the machine. This is needed to make multiple windows possible.
Created attachment 283455 [details] [review] app: Populate new main window's collection This is needed to make multiple windows possible.
Created attachment 283456 [details] [review] selectionbar: Add 'Open in new windows' button This is needed to make multiple windows possible.
Created attachment 283457 [details] [review] Add MachineMenu Implements the machine menu shown in this mockup: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ boxes/wires/multi-window.png This is needed to make multiple windows possible.
Created attachment 283458 [details] [review] display-toolbar: Replace properties button by MachineMenu This is needed to make multiple windows possible.
Created attachment 283460 [details] [review] colection-view: Show MachineMenu on right-click This is needed to make multiple windows possible.
Created attachment 283461 [details] [review] searchbar: Ensure window is set before connecting to it Until this patch, window was in an uncertain state when trying to connect to it.
Created attachment 283462 [details] [review] app: Replace main_window by list of windows This is needed to make multiple windows possible.
Created attachment 283463 [details] [review] app-window: Closing window removes it from collection Also, use 'this.window' to refer to Topbar's AppWindow in a closure to avoid a Vala bug relative to the AppWindow while destroying the closure. This is needed to make multiple windows possible.
Created attachment 283464 [details] [review] machine: Close window on machine deletion If a machine haves a window and the machine is deleted, the window will be closed. This is needed to make multiple windows possible.
Created attachment 283465 [details] [review] collection-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283466 [details] [review] display-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283467 [details] [review] app-window: Present window when machine selected This presents a machine's window if the machine is selected and is already running in a window, otherwise it creates a new window for the machine. This is needed to make multiple windows possible.
Created attachment 283468 [details] [review] app: Populate new main window's collection This is needed to make multiple windows possible.
Created attachment 283469 [details] [review] selectionbar: Add 'Open in new windows' button This is needed to make multiple windows possible.
Created attachment 283470 [details] [review] Add MachineMenu Implements the machine menu shown in this mockup: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ boxes/wires/multi-window.png This is needed to make multiple windows possible.
Created attachment 283471 [details] [review] display-toolbar: Replace properties button by MachineMenu This is needed to make multiple windows possible.
Created attachment 283472 [details] [review] colection-view: Show MachineMenu on right-click This is needed to make multiple windows possible.
Review of attachment 283461 [details] [review]: "Until this patch, window was in an uncertain state when trying to connect to it." As I asked previously, how so?
Review of attachment 283462 [details] [review]: ack
Review of attachment 283463 [details] [review]: almost there. :) ::: src/machine.vala @@ +78,3 @@ + else { + window.destroy (); + window = null; I thought it would be obvious but I was wrong. These changes also qualify for my comment for rest of the changes in this module: "These changes seem to be about deletion of window on deletion of machine rather than about removal of window from collection on deletion so I think they need to be in separate patch." ::: src/topbar.vala @@ +104,3 @@ + // FIXME: Usage of 'this' is a work around to avoid double + // deletion of the window when destroying the closure. + // It is either caused by a Vala bug or an ownership problem. if you can't find out the issue right now, please file a separate bug for this. Also (I think i mentioned last time), this change doesn't belong in this patch even if this is the patch that realizes the issue.
Review of attachment 283464 [details] [review]: typo: haves. Looks fine otherwise.
Review of attachment 283465 [details] [review]: ack
Review of attachment 283466 [details] [review]: ack
Review of attachment 283467 [details] [review]: I'd use the term 'activation' and 'activate' instead of 'selection' and 'select' to avoid confusion with selection as in selection mode. Also I'd just replace "if the machine is selected and " with " on activation if machine " ::: src/app-window.vala @@ +226,1 @@ + if (machine.window != this) { shouldn't we also check if its non-null?
Review of attachment 283468 [details] [review]: ack
Review of attachment 283469 [details] [review]: Good otherwise. ::: data/ui/selectionbar.ui @@ +59,3 @@ + <property name="valign">center</property> + <property name="use-underline">True</property> + <property name="label" translatable="yes">_Open in new window(s)</property> Since we set this from code, either maybe we shouldn't set it here or use one of the strings we use in the code. Translators could do with one less string to translate. :) ::: src/app.vala @@ +323,3 @@ + } + + public void open_in_new_window (Machine machine) { Lets keep it private until/unless we need it public. ::: src/selectionbar.vala @@ +162,3 @@ + + open_btn.sensitive = items > 0; + // This goes with the "Click on items to select them" string and is about selection of items (boxes) I tend to prefix comments for translators as "Translators: ". Without that, this is confusing since this button is not in the same place as "Click on items.." string at all. Actually I don't think we need the part before "and" here. Actually I'd just write: "Translators: This is a button to open box(es) in new window(s)".
Review of attachment 283470 [details] [review]: Firstly, I'll want to separate out this menu from mult-windows since you already provide a means to launch item in new windows. Secondly, I'm not sure this approach is what we want to take. I'd think we should: 1. Define a context menu completely in UI file(s). 2. On right click, we check if there is any item under the cursor (I believe we already have code for that since currently item is automatically selected when going to selection mode through right click). 3. If there is item under cursor, we show the context menu instead of going to selection mode. You'll need code to find out coordinates of the item. We might have deleted that code at some point but git remembers everything. :) ::: src/util.vala @@ +278,3 @@ } + public Variant object_to_variant (Object object) { Let put addition of general utils in a separate patch.
Review of attachment 283471 [details] [review]: Looks fine, apart from "This is needed to make multiple windows possible." is not true and just like previous patch, I think we should put it in separate bug which would be about adding of menu.
Review of attachment 283472 [details] [review]: same as last patch. ::: src/collection-view.vala @@ +45,3 @@ notify["ui-state"].connect (ui_state_changed); + + icon_view = get_generic_view () as Gtk.IconView; I think there is other users of 'get_generic_view () as Gtk.IconView;' so I'd put addition of icon_view in a separate (before this one) patch and update existing uses. @@ +322,3 @@ + + machine_menu.machine = machine; + machine_menu.set_pointing_to (rect); ah good, you already got code to do this. I'm a bit confused now why you need to convert items to variant and viceversa if you can find out the item under cursor anyway.
Review of attachment 283470 [details] [review]: 1. I didn't achieve to do this and have actions with the machine as a parameter, unfortunately, do you know how to do it properly? 2. Unfortunately it's not as simple as it looks like, as the right-click event is phagocytosed by the view to create the selection_mode_request signal, which gives no information. 3. Good to know, it will certainly be needed to make the implementation cleaner!
Created attachment 283493 [details] [review] searchbar: Ensure window is set before connecting to it Until this patch, window was in an uncertain state when trying to connect to it, because on_app_ready() was called before setup_ui().
Created attachment 283494 [details] [review] app: Replace main_window by list of windows This is needed to make multiple windows possible.
Created attachment 283495 [details] [review] app-window: Closing window removes it from collection Also, use 'this.window' to refer to Topbar's AppWindow in a closure to avoid a Vala bug relative to the AppWindow while destroying the closure. This is needed to make multiple windows possible.
Created attachment 283496 [details] [review] machine: Close window on machine deletion/not running If a machine have a window and the machine is deleted or stops running, the window will be closed. This is needed to make multiple windows possible.
Created attachment 283497 [details] [review] collection-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283498 [details] [review] display-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283499 [details] [review] app-window: Present window when machine activated This presents a machine's window if the machine is activated and is already running in a window, otherwise it creates a new window for the machine. This is needed to make multiple windows possible.
Created attachment 283500 [details] [review] app: Populate new main window's collection This is needed to make multiple windows possible.
Created attachment 283501 [details] [review] selectionbar: Add 'Open in new windows' button This is needed to make multiple windows possible.
Review of attachment 283493 [details] [review]: ack
Review of attachment 283494 [details] [review]: ack
Review of attachment 283495 [details] [review]: ::: src/app.vala @@ +511,3 @@ + base.remove_window (window); + + bool removed = initial_windows_count != windows.length (); * i'd declare/define it here just before its first use below. * Use 'var' ::: src/topbar.vala @@ +104,3 @@ + // FIXME: Usage of 'this' is a work around to avoid double + // deletion of the window when destroying the closure. + // It is either caused by a Vala bug or an ownership problem. You missed my comment from last review on this line.
Review of attachment 283495 [details] [review]: ::: src/app.vala @@ +511,3 @@ + base.remove_window (window); + + bool removed = initial_windows_count != windows.length (); In fact, you don't need the variable just do: return (condition);
Review of attachment 283496 [details] [review]: Good. One nitpick though: "not running" -> "stopping" in shortlog.
Review of attachment 283497 [details] [review]: ack
Review of attachment 283498 [details] [review]: ack
Review of attachment 283499 [details] [review]: ack apart from one nitpick. ::: src/app-window.vala @@ +226,1 @@ + if (machine.window != this && machine.window != null) { probobly won't matter in practice but better haven the null check before comparison.
Review of attachment 283500 [details] [review]: still ack
Review of attachment 283501 [details] [review]: you did as I asked so sorry that still changes are still needed. Not a lot though. :) ::: src/selectionbar.vala @@ +163,3 @@ + open_btn.sensitive = items > 0; + // Translators: This is a button to open box(es) in new window(s) + open_btn.label = ngettext ("_Open in %d new window", "_Open in %d new windows", items).printf (items); Just realized this: We dont' want to show number of boxes for singular case. Also since its a button, we want to keep its label short. So how about just a singlular shorter form like "Open in new window"? In which case we can just set it from UI file once. Sorry for not thinking this thoroughly before.
Created attachment 283542 [details] [review] app-window: Closing window removes it from collection Also, use 'this.window' to refer to Topbar's AppWindow in a closure to avoid a Vala bug relative to the AppWindow while destroying the closure. This is needed to make multiple windows possible.
Created attachment 283543 [details] [review] machine: Close window on machine deletion/stopping If a machine have a window and the machine is deleted or stops running, the window will be closed. This is needed to make multiple windows possible.
Created attachment 283544 [details] [review] collection-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283545 [details] [review] display-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283546 [details] [review] app-window: Present window when machine activated This presents a machine's window if the machine is activated and is already running in a window, otherwise it creates a new window for the machine. This is needed to make multiple windows possible.
Created attachment 283548 [details] [review] app: Populate new main window's collection This is needed to make multiple windows possible.
Created attachment 283549 [details] [review] selectionbar: Add 'Open in new windows' button This is needed to make multiple windows possible.
Created attachment 283555 [details] [review] app-window: Closing window removes it from collection Also, use 'this.window' to refer to Topbar's AppWindow in a closure to avoid a Vala bug relative to the AppWindow while destroying the closure. This is needed to make multiple windows possible.
Created attachment 283556 [details] [review] machine: Close window on machine deletion/stopping If a machine have a window and the machine is deleted or stops running, the window will be closed. This is needed to make multiple windows possible.
Created attachment 283557 [details] [review] collection-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283558 [details] [review] display-toolbar: Show back button only on main window This forbids non-main windows to access the collection view. This is needed to make multiple windows possible.
Created attachment 283559 [details] [review] app-window: Present window when machine activated This presents a machine's window if the machine is activated and is already running in a window, otherwise it creates a new window for the machine. This is needed to make multiple windows possible.
Created attachment 283560 [details] [review] app: Populate new main window's collection This is needed to make multiple windows possible.
Created attachment 283561 [details] [review] selectionbar: Add 'Open in new windows' button This is needed to make multiple windows possible.
Review of attachment 283555 [details] [review]: Good otherwise. Your log will obviously also need some change. ::: src/topbar.vala @@ +103,3 @@ window.notify["selection-mode"].connect (() => { + // FIXME: Usage of 'this' is a work around for https://bugzilla.gnome.org/show_bug.cgi?id=734877 + page = this.window.selection_mode ? I think I should repeat myself so you don't miss it again :) "..this change doesn't belong in this patch even if this is the patch that realizes the issue."
Review of attachment 283555 [details] [review]: oops wrong status.
Review of attachment 283556 [details] [review]: ack. Although when i tested, I had the following: 1. launched a VM in new window 2. Click Properties 3. Change RAM 4. Go back 5. Got notification about shutdown needed and chose 'yes' 6. Since the VM has booting issues, shutdown request wasn't entertained so 5. got notification for force shutdown. 7. Clicked force shutdown 8. Although VM was shutdown, the window stayed around (all grey) and got a notification about 'failed to connect'. 9. Went back to the main window and was able to start the VM fine from collection view but it started in that window. I'm not sure this is the patch thats responsible to make sure that doesn't happen but likely that it is.
Review of attachment 283557 [details] [review]: ack
Review of attachment 283558 [details] [review]: ack
Review of attachment 283559 [details] [review]: ack
Review of attachment 283560 [details] [review]: still ack
Review of attachment 283561 [details] [review]: Good otherwise. ::: src/selectionbar.vala @@ +163,3 @@ + open_btn.sensitive = items > 0; + // Translators: This is a button to open box(es) in new window(s) + if (items ==0) nitpick: space before 0
Review of attachment 283556 [details] [review]: Infact I can reproduce this with proper shutdown from within guest too so pretty sure its this patch that needs fixing.
Review of attachment 283556 [details] [review]: And now I got some crashes too while testing this: (gnome-boxes:16984): Gtk-CRITICAL **: gtk_widget_get_window: assertion 'GTK_IS_WIDGET (widget)' failed (gnome-boxes:16984): GLib-GObject-WARNING **: invalid (NULL) pointer instance (gnome-boxes:16984): GLib-GObject-CRITICAL **: g_signal_handler_disconnect: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed Segmentation fault (core dumped) I think this code needs some more testing and fixing.
Pushing all but the patch to close window on machine deletion/stopping. I'm hopeful you'll fix that early next week. :) Attachment 283493 [details] pushed as 3a4f0b2 - searchbar: Ensure window is set before connecting to it Attachment 283494 [details] pushed as 18c44dc - app: Replace main_window by list of windows Attachment 283555 [details] pushed as df7751b - app-window: Closing window removes it from collection Attachment 283557 [details] pushed as c7ad053 - collection-toolbar: Show back button only on main window Attachment 283558 [details] pushed as c390167 - display-toolbar: Show back button only on main window Attachment 283559 [details] pushed as b2c41b0 - app-window: Present window when machine activated Attachment 283560 [details] pushed as 1f95ac9 - app: Populate new main window's collection Attachment 283561 [details] pushed as b0e2fc3 - selectionbar: Add 'Open in new windows' button
I cant close the window regularly anymore since this commit: commit df7751b4e6f631d2f6d1375706845651f9e51d8d Author: Adrien Plazas <kekun.plazas@laposte.net> Date: Sun Aug 3 16:16:02 2014 +0200 app-window: Closing window removes it from collection This is needed to make multiple windows possible. https://bugzilla.gnome.org/show_bug.cgi?id=734486
(In reply to comment #96) > I cant close the window regularly anymore since this commit: > > commit df7751b4e6f631d2f6d1375706845651f9e51d8d > Author: Adrien Plazas <kekun.plazas@laposte.net> > Date: Sun Aug 3 16:16:02 2014 +0200 > > app-window: Closing window removes it from collection > > This is needed to make multiple windows possible. > > https://bugzilla.gnome.org/show_bug.cgi?id=734486 Thanks for bisecting. I'm able to close the window fine so I think you'll have to debug this on your own unless Kekun can also reproduce.
Review of attachment 283556 [details] [review]: I dont know if its the best solution that the machine closes the window where it belongs. Can't we tell the AppWindow that its a secondary one and check on change of the ui_state property for invalid states for secondary windows? This way we dont have to worry what causes the VM to go away, we just let the window disappear if it changes to COLLECTION state and show the main window, which has the COLLECTION state. Just my humble opinion, I'm not really in the code so I can't tell whats possible or not.
(In reply to comment #98) > Review of attachment 283556 [details] [review]: > > I dont know if its the best solution that the machine closes the window where > it belongs. Can't we tell the AppWindow that its a secondary one and check on > change of the ui_state property for invalid states for secondary windows? This > way we dont have to worry what causes the VM to go away, we just let the window > disappear if it changes to COLLECTION state and show the main window, which has > the COLLECTION state. Makes sense actually. Since we already have multi-window support (which this bug is about), lets move this issue into another bug (bug#735045) and resolve this one.