GNOME Bugzilla – Bug 680293
Properties modification improvements
Last modified: 2016-03-31 14:01:38 UTC
This is a series that fixes a number of bugs with properties page: - add an indicator of pending properties change requiring reboot - actually hard reboot VM to take changes into account - being able to stay on DISPLAY or PROPERTIES while display is disconnected with Machine:stay-on-display - do not lose pending changes when doing properties modification - misc spice improvements, using new "ready" display property
Created attachment 219285 [details] [review] Change Pair property into object Property
Created attachment 219286 [details] [review] Pass the Property in the changed callback So that the Property state can be modified.
Created attachment 219287 [details] [review] Remove unnecessary vexpand
Created attachment 219288 [details] [review] Inform if some property changes are pending Add an infobar to the properties page to inform if some changes are pending
Created attachment 219289 [details] [review] Do not lose pending changes Use the INACTIVE config as the current config to modified, to not lose any other pending changes
Created attachment 219290 [details] [review] Inform if the memory property has pending changes
Created attachment 219291 [details] [review] Add Machine:stay-on-display Learn to stay on display UI state after disconnection. This is a simplified version of https://bugzilla.gnome.org/show_bug.cgi?id=677101
Created attachment 219292 [details] [review] libvirt: use hard reboot/restart Make sure the qemu process is restarted to take changes into effect
Created attachment 219293 [details] [review] Ignore showing the same widget
Created attachment 219294 [details] [review] Factor out MachineActor.update_display() In a following patch, we will update the display widget while in Properties page. That new function take care of that. Note: I don't know what the Idle was used for, it doesn't seem to change anything. Note2: The animation duration seems to be longer now, so I divided it herer arbitrarily by 2.., I suspect this is related to the multiple size_alloc() events (and no, the Idle was not helping with this problem)
Created attachment 219295 [details] [review] spice: use SpiceDisplay:ready property Requires spice-gtk 0.12.101
Created attachment 219296 [details] [review] Show update display in Properties page Factor out the code to show the display in the right place function of current UI state.
Review of attachment 219287 [details] [review]: Why is it unnecessary? Is it causing issues?
Review of attachment 219288 [details] [review]: ::: src/properties.vala @@ +43,3 @@ + break; + } + } We don't need to iterate over the properties when we are arriving there through the 'changes-pending' signal, do we? @@ +50,3 @@ + foreach (var property in properties) { + message ("disconnecting %s", property.description); + SignalHandler.disconnect_by_func (property, (void*)update_infobar, this); Do we really need to do this? The list will be disposed when PageWidget is, so the signal handlers will get disconnected automatically no? @@ +114,3 @@ } + + update_infobar (); Why do we need to call it there? The object has just been created so there should be no pending changes? Isn't it enough to rely on the 'changes-pending' signal? @@ +145,3 @@ var page = new PageWidget (i, machine); notebook.append_page (page.widget, null); + notebook.set_data<PageWidget> (@"boxes-property-$i", page); This does not seem to be used?
(In reply to comment #5) > Created an attachment (id=219289) [details] [review] > Do not lose pending changes > > Use the INACTIVE config as the current config to modified, to not lose any > other pending changes Fwiw, see https://bugzilla.gnome.org/show_bug.cgi?id=678894#c68 : « Zeeshan Ali (Khattak) [reporter] [gnome-boxes developer] 2012-07-03 14:32:33 UTC (In reply to comment #65) > (From update of attachment 217851 [details] [review] [details]) > >- var config = domain.get_config (0); > >- VMConfigurator.post_install_setup (config); > >- domain.set_config (config); > >+ var config_xml = machine.domain_config.to_xml (); > >+ var post_install_config = new GVirConfig.Domain.from_xml (config_xml); > >+ VMConfigurator.post_install_setup (post_install_config); > >+ machine.domain.set_config (post_install_config); > > This part is ugy imo, I'd stick with the old code + a comment explaining why we > can't use machine.domain_config. But we can use it. Its ugly yes but from experience the lesser we make blocking calls in boxes the better so I'd say its worth the few lines of ugliness. How about I add a comment there explaining why we clone the config? »
Review of attachment 219288 [details] [review]: ::: src/properties.vala @@ +43,3 @@ + break; + } + } we need to know if there is still one or none of the properties pending, so yes we do need to iterate @@ +50,3 @@ + foreach (var property in properties) { + message ("disconnecting %s", property.description); + SignalHandler.disconnect_by_func (property, (void*)update_infobar, this); no, properties do out-live the PageWidget, so we need to explicitely disconnect handlers. @@ +114,3 @@ } + + update_infobar (); no, it's not enough. You can have a machine that has not been rebooted with pending changes., and you go to the properties page, and show the infobar. There has been no changes-pending notify signal during that time received by the PageWidget. @@ +145,3 @@ var page = new PageWidget (i, machine); notebook.append_page (page.widget, null); + notebook.set_data<PageWidget> (@"boxes-property-$i", page); It does keep the PageWidget around, along with the notebook, until it is replaced by new PageWidget.
Comment on attachment 219290 [details] [review] Inform if the memory property has pending changes >From 77bcfe519a4f5b22388d0aca58cbdbddfca7905a Mon Sep 17 00:00:00 2001 >From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@gmail.com> >Date: Thu, 19 Jul 2012 15:23:16 +0200 >Subject: [PATCH] Inform if the memory property has pending changes > >https://bugzilla.gnome.org/show_bug.cgi?id=680293 >--- > src/libvirt-machine.vala | 37 ++++++++++++++++++++++++++++++------- > 1 file changed, 30 insertions(+), 7 deletions(-) > >diff --git a/src/libvirt-machine.vala b/src/libvirt-machine.vala >index f8dabd2..639dc15 100644 >--- a/src/libvirt-machine.vala >+++ b/src/libvirt-machine.vala >@@ -467,17 +467,39 @@ public void force_shutdown (bool confirm = true) { > return net; > } > >+ private void update_ram_property (Boxes.Property property) { >+ try { >+ var config = domain.get_config (GVir.DomainXMLFlags.INACTIVE); I don't think adding lots of get_config calls is a good way forward, this has been found to sometimes block for hundreds of ms at times. >+ >+ // we uses KiB unit, convert to MiB >+ var actual = domain_config.memory / 1024; >+ var pending = config.memory / 1024; >+ >+ debug ("RAM actual: %llu, pending: %llu", actual, pending); Not important in this case, but maybe we can use g_format_size_full instead of doing the conversion by hand? >+ // somehow, there are rounded errors, so let's forget about 1Mb diff "rounding" >+ property.changes_pending = (actual - pending) > 1; // no need for abs() >+ Is this 'hack' needed too if you compare domain_config.memory and config.memory? >+ } catch (GLib.Error e) {} >+ } >+ > private void add_ram_property (ref List list) { > try { > var max_ram = connection.get_node_info ().memory; > >- add_size_property (ref list, >- _("RAM"), >- domain_config.memory, >- Osinfo.MEBIBYTES / Osinfo.KIBIBYTES, >- max_ram, >- Osinfo.MEBIBYTES / Osinfo.KIBIBYTES, >- on_ram_changed); >+ var property = add_size_property (ref list, >+ _("RAM"), >+ domain_config.memory, >+ Osinfo.MEBIBYTES / Osinfo.KIBIBYTES, >+ max_ram, >+ Osinfo.MEBIBYTES / Osinfo.KIBIBYTES, >+ on_ram_changed); >+ >+ this.notify["state"].connect (() => { >+ if (state == MachineState.STOPPED) >+ property.changes_pending = false; >+ }); >+ >+ update_ram_property (property); > } catch (GLib.Error error) {} > } > >@@ -500,6 +522,7 @@ private void on_ram_changed (Boxes.Property property, uint64 value) { > error.message); > } > ram_update_timeout = 0; >+ update_ram_property (property); > > return false; > }); >-- >1.7.10.4
Review of attachment 219292 [details] [review]: ::: src/libvirt-machine.vala @@ +543,3 @@ + state_id = this.notify["state"].connect (() => { + if (state == MachineState.STOPPED) { + start (); Isn't it enough to call domain.start(0) here? @@ +550,3 @@ try { + domain.shutdown (GVir.DomainShutdownFlags.ACPI_POWER_BTN | + GVir.DomainShutdownFlags.GUEST_AGENT); I think this will have no effect on domains with no ACPI nor guest agent, meaning that we will automatically restart the VM the next time the user shuts it down. Dunno if that's an issue we care about.
(In reply to comment #15) > (In reply to comment #5) > > Created an attachment (id=219289) [details] [review] [details] [review] > > Do not lose pending changes > > > > Use the INACTIVE config as the current config to modified, to not lose any > > other pending changes > > Fwiw, see https://bugzilla.gnome.org/show_bug.cgi?id=678894#c68 : > > « Zeeshan Ali (Khattak) [reporter] [gnome-boxes developer] 2012-07-03 14:32:33 > UTC > (In reply to comment #65) > > (From update of attachment 217851 [details] [review] [details] [details]) > > >- var config = domain.get_config (0); > > >- VMConfigurator.post_install_setup (config); > > >- domain.set_config (config); > > >+ var config_xml = machine.domain_config.to_xml (); > > >+ var post_install_config = new GVirConfig.Domain.from_xml (config_xml); > > >+ VMConfigurator.post_install_setup (post_install_config); > > >+ machine.domain.set_config (post_install_config); > > > > This part is ugy imo, I'd stick with the old code + a comment explaining why we > > can't use machine.domain_config. > > But we can use it. Its ugly yes but from experience the lesser we make blocking > calls in boxes the better so I'd say its worth the few lines of ugliness. How > about I add a comment there explaining why we clone the config? » The domain.get_config (GVir.DomainXMLFlags.INACTIVE) are called very rarely during init and modification of Properties.. if they really slow down Properties page (are we really talking about that?) we can put them in a thread.
Comment on attachment 219293 [details] [review] Ignore showing the same widget Is it causing issues?
(In reply to comment #17) > > > >+ private void update_ram_property (Boxes.Property property) { > >+ try { > >+ var config = domain.get_config (GVir.DomainXMLFlags.INACTIVE); > > I don't think adding lots of get_config calls is a good way forward, this has > been found to sometimes block for hundreds of ms at times. It's really not the same, doing dozen of get_config()/sec and more during DISPLAY time and doing it once in Properties page. > >+ // we uses KiB unit, convert to MiB > >+ var actual = domain_config.memory / 1024; > >+ var pending = config.memory / 1024; > >+ > >+ debug ("RAM actual: %llu, pending: %llu", actual, pending); > > Not important in this case, but maybe we can use g_format_size_full instead of > doing the conversion by hand? I don't see that as being more convenient here, since we go from KiB to MiB > >+ property.changes_pending = (actual - pending) > 1; // no need for abs() > >+ > > Is this 'hack' needed too if you compare domain_config.memory and > config.memory? Yes, it is.
(In reply to comment #18) > Review of attachment 219292 [details] [review]: > > ::: src/libvirt-machine.vala > @@ +543,3 @@ > + state_id = this.notify["state"].connect (() => { > + if (state == MachineState.STOPPED) { > + start (); > > Isn't it enough to call domain.start(0) here? I tried to simplify the domain.start() code in the start() method (handling of errors and various other state when needed) > > @@ +550,3 @@ > try { > + domain.shutdown (GVir.DomainShutdownFlags.ACPI_POWER_BTN | > + GVir.DomainShutdownFlags.GUEST_AGENT); > > I think this will have no effect on domains with no ACPI nor guest agent, > meaning that we will automatically restart the VM the next time the user shuts > it down. Dunno if that's an issue we care about. True, but I don't know what else we can do. Tbh, I am not sure having this reboot functionnality is really solid, time will tell I guess
Review of attachment 219294 [details] [review]: Not familiar at all with this code, Alex would be better to review this... As for the /2, I wouldn't necessarily worry too much about it since the plan was to make the animations faster anyway. No idea about the idle either. Patch looks reasonable but I'm very unfamiliar with clutter...
(In reply to comment #20) > (From update of attachment 219293 [details] [review]) > Is it causing issues? I think it is no longer needed, I can't hit the warning again.
Review of attachment 219291 [details] [review]: ::: src/libvirt-machine.vala @@ +15,3 @@ } + private bool _connect_display; This should be _display_connected if I understand the purpose of this variable correctly. @@ +28,3 @@ return; + _connect_display = true; I'd move that to the bottom of this function if nothing depends on this being set early, one less thing to change when/if we make connect_display() throw. @@ +34,1 @@ domain.resume (); Will we get a 'started' event in addition to a 'resumed' event when the domain is resumed? @@ +129,3 @@ + state = MachineState.RUNNING; + reconnect_display (); + }); This signal is no longer disconnected once the display has been reconnected, I think this is ok, but I prefer to check.
Review of attachment 219295 [details] [review]: ::: configure.ac @@ +50,3 @@ LIBVIRT_GCONFIG_MIN_VERSION=0.1.0 LIBXML2_MIN_VERSION=2.7.8 +SPICE_GTK_MIN_VERSION=0.12.101 It's better if we wait for a 0.13 release before this goes in, unless we are absolutely sure 0.13 will be out on August 6 (tarball dues for gnome 3.5.5) ::: src/spice-display.vala @@ +118,3 @@ + show (display.channel_id); + else + hide (display.channel_id); What happens in boxes when we hide this?
Review of attachment 219296 [details] [review]: Sorry, no clue about that one, I'll leave it up to Zeeshan ;) If that's not an option, let me know and I'll spend more time on it early next week.
(In reply to comment #25) > Review of attachment 219291 [details] [review]: > > ::: src/libvirt-machine.vala > @@ +15,3 @@ > } > > + private bool _connect_display; > > This should be _display_connected if I understand the purpose of this variable > correctly. It's not already connected, but it just mark that connect_display () has been called, to avoid reentering. I chose a variable that match the name of the function for this reason. > @@ +28,3 @@ > return; > > + _connect_display = true; > > I'd move that to the bottom of this function if nothing depends on this being > set early, one less thing to change when/if we make connect_display() throw. I prefer at the top for the reason that it block any further attempt to enter the function (recursively or not). > @@ +34,1 @@ > domain.resume (); > > Will we get a 'started' event in addition to a 'resumed' event when the domain > is resumed? I don't follow, why? > > @@ +129,3 @@ > + state = MachineState.RUNNING; > + reconnect_display (); > + }); > > This signal is no longer disconnected once the display has been reconnected, I > think this is ok, but I prefer to check. the domain::started signal? So if the machine is started, it will connect the display if it called connect_display () before and _connect_display is still set (disconnect_display() hasn't been called).
(In reply to comment #26) > Review of attachment 219295 [details] [review]: > > ::: configure.ac > @@ +50,3 @@ > LIBVIRT_GCONFIG_MIN_VERSION=0.1.0 > LIBXML2_MIN_VERSION=2.7.8 > +SPICE_GTK_MIN_VERSION=0.12.101 > > It's better if we wait for a 0.13 release before this goes in, unless we are > absolutely sure 0.13 will be out on August 6 (tarball dues for gnome 3.5.5) > > ::: src/spice-display.vala > @@ +118,3 @@ > + show (display.channel_id); > + else > + hide (display.channel_id); > > What happens in boxes when we hide this? We remove it from the display page, that's it atm. On ::show we place it where it is supposed to be depending on UIState with this seris.
Review of attachment 219288 [details] [review]: ::: src/properties.vala @@ +145,3 @@ var page = new PageWidget (i, machine); notebook.append_page (page.widget, null); + notebook.set_data<PageWidget> (@"boxes-property-$i", page); Hrm ok, this relies on some vala magic. A GtkNotebook subclass would be cleaner imo, but well...
(In reply to comment #31) > Review of attachment 219288 [details] [review]: > > ::: src/properties.vala > @@ +145,3 @@ > var page = new PageWidget (i, machine); > notebook.append_page (page.widget, null); > + notebook.set_data<PageWidget> (@"boxes-property-$i", page); > > Hrm ok, this relies on some vala magic. A GtkNotebook subclass would be cleaner > imo, but well... I would call it gobject magic, there is nothing specific to vala here, except that vala tries his best to use the right function to do object management (use set_data_full, ref/unref object)
(In reply to comment #21) > (In reply to comment #17) > > > > > >+ private void update_ram_property (Boxes.Property property) { > > >+ try { > > >+ var config = domain.get_config (GVir.DomainXMLFlags.INACTIVE); > > > > I don't think adding lots of get_config calls is a good way forward, this has > > been found to sometimes block for hundreds of ms at times. > > It's really not the same, doing dozen of get_config()/sec and more during > DISPLAY time and doing it once in Properties page. Now the use is well thought out, but if we have get_config calls here and there, then sooner or later it will be used again somewhere it should not be used. It could also interfere with some running animations, stat updating, ... I guess (?) and make them less smooth, that's why I'm mentioning it. > > > >+ // we uses KiB unit, convert to MiB > > >+ var actual = domain_config.memory / 1024; > > >+ var pending = config.memory / 1024; > > >+ > > >+ debug ("RAM actual: %llu, pending: %llu", actual, pending); > > > > Not important in this case, but maybe we can use g_format_size_full instead of > > doing the conversion by hand? > > I don't see that as being more convenient here, since we go from KiB to MiB Why MiB and not GiB ? ;) "12 GiB" is more readable than a unitless 12288. But that's not really important.
(In reply to comment #32) > (In reply to comment #31) > > Review of attachment 219288 [details] [review] [details]: > > > > ::: src/properties.vala > > @@ +145,3 @@ > > var page = new PageWidget (i, machine); > > notebook.append_page (page.widget, null); > > + notebook.set_data<PageWidget> (@"boxes-property-$i", page); > > > > Hrm ok, this relies on some vala magic. A GtkNotebook subclass would be cleaner > > imo, but well... > > I would call it gobject magic, there is nothing specific to vala here, except > that vala tries his best to use the right function to do object management (use > set_data_full, ref/unref object) Yes, it does a _ref behind the hood while g_object_set_data does not, so this is what I call vala magic.
(In reply to comment #29) > (In reply to comment #25) > > Review of attachment 219291 [details] [review] [details]: > > > > ::: src/libvirt-machine.vala > > @@ +15,3 @@ > > } > > > > + private bool _connect_display; > > > > This should be _display_connected if I understand the purpose of this variable > > correctly. > > It's not already connected, but it just mark that connect_display () has been > called, to avoid reentering. I chose a variable that match the name of the > function for this reason. _connecting_display then? It's not obvious if you meant to call it _connect_display or if the naming is slightly inaccurate. > > > @@ +28,3 @@ > > return; > > > > + _connect_display = true; > > > > I'd move that to the bottom of this function if nothing depends on this being > > set early, one less thing to change when/if we make connect_display() throw. > > I prefer at the top for the reason that it block any further attempt to enter > the function (recursively or not). Yeah, I agree since the variable is about "connection in progress" and not "display connected" > > > @@ +34,1 @@ > > domain.resume (); > > > > Will we get a 'started' event in addition to a 'resumed' event when the domain > > is resumed? > > I don't follow, why? > The old code was reconnecting the display on domain.resumed when the machine is paused, and on domain.started in other cases. Now we are only reconnecting the display on domain.started, so I'm asking if this is enough or if the .resumed handling has been lost.
(In reply to comment #30) > > What happens in boxes when we hide this? > > We remove it from the display page, that's it atm. On ::show we place it where > it is supposed to be depending on UIState with this seris. Does that mean we'll get a black screen if the box is fullscreen when we receive that signal?
Review of attachment 219290 [details] [review]: Failed to publish review: Undefined subroutine &extensions::splinter::lib::WSSplinter::ThrowCodeError called at /var/www/bugzilla.gnome.org/extensions/splinter/lib/WSSplinter.pm line 88. (seems I need a placeholder when switching the patch status, otherwise I get the error above if I don't write anything in the review box).
Review of attachment 219295 [details] [review]: ::: configure.ac @@ +50,3 @@ LIBVIRT_GCONFIG_MIN_VERSION=0.1.0 LIBXML2_MIN_VERSION=2.7.8 +SPICE_GTK_MIN_VERSION=0.12.101 There is a tarball for 0.12.101, no big changes expected for 0.13, but release should happen soon. We need testing, so I would prefer we actually depend on it for the next boxes release. ::: src/spice-display.vala @@ +118,3 @@ + show (display.channel_id); + else + hide (display.channel_id); The display widget is removed, but we don't change the current UI state (it stays on display page for example). The non-ready state should only happen during initial connection, and resolution switch. There is nothing to draw when non-ready, and spice-gtk doesn't provide a fallback atm. We may want to have the last display drawn in greyish with a little spinner during that time instead? But that can be address in a future patch, or in spice-gtk. For now this still better.
Attachment 219285 [details] pushed as b1ce37d - Change Pair property into object Property Attachment 219286 [details] pushed as e238156 - Pass the Property in the changed callback Attachment 219287 [details] pushed as 165c87c - Remove unnecessary vexpand Attachment 219288 [details] pushed as 727d262 - Inform if some property changes are pending Attachment 219289 [details] pushed as 6eb3b04 - Do not lose pending changes Attachment 219290 [details] pushed as bd477a2 - Inform if the memory property has pending changes Attachment 219291 [details] pushed as 3cdd3be - Add Machine:stay-on-display Attachment 219292 [details] pushed as ade9c1b - libvirt: use hard reboot/restart Attachment 219294 [details] pushed as 188f9e3 - Factor out MachineActor.update_display() Attachment 219295 [details] pushed as c7a26a5 - spice: use SpiceDisplay:ready property Attachment 219296 [details] pushed as f9d7073 - Show update display in Properties page
Why did you push all of this? Have of the patches were only tagged as reviewed, one as none, and there are still open questions in comment #35
(In reply to comment #38) > There is a tarball for 0.12.101, no big changes expected for 0.13, but release > should happen soon. We need testing, so I would prefer we actually depend on it > for the next boxes release. spice-gtk is an external dependency not following gnome release cycle, so Boxes can only depend on actual releases (not necessarily stable releases, development releases are fine too).
(In reply to comment #41) > (In reply to comment #38) > > There is a tarball for 0.12.101, no big changes expected for 0.13, but release > > should happen soon. We need testing, so I would prefer we actually depend on it > > for the next boxes release. > > spice-gtk is an external dependency not following gnome release cycle, so Boxes > can only depend on actual releases (not necessarily stable releases, > development releases are fine too). Good luck convincing elmarco about the diff. :)
(In reply to comment #42) > Good luck convincing elmarco about the diff. :) Attachment 219295 [details] has been pushed without being ACK'ed first, we still have the option of reverting it until spice-gtk sees an actual release.
(In reply to comment #43) > (In reply to comment #42) > > Good luck convincing elmarco about the diff. :) > > Attachment 219295 [details] has been pushed without being ACK'ed first, we still have the > option of reverting it until spice-gtk sees an actual release. FYI, maintainers do have the right to push patches w/o getting an ACK. Unless you can point out some regressions caused by this patch, I don't think we need to revert this just because we disagree with elmarco on the exact definition of a release.
Created attachment 219967 [details] [review] Reconnect when machine is RUNNING As pointed out by Christophe review, the commit 3cdd3bee2967eeca8ad57d8fda93db0da9bc51e6 removed the connection to display on resume. This patch fix it, as it was my original intent to factor out that logic in one place.
(In reply to comment #44) > (In reply to comment #43) > > (In reply to comment #42) > > > Good luck convincing elmarco about the diff. :) > > > > Attachment 219295 [details] [details] has been pushed without being ACK'ed first, we still have the > > option of reverting it until spice-gtk sees an actual release. > > FYI, maintainers do have the right to push patches w/o getting an ACK. I disagree with that. > Unless > you can point out some regressions caused by this patch, I don't think we need > to revert this just because we disagree with elmarco on the exact definition of > a release. not being able to do a proper Boxes 3.5 release next Monday sounds like a bad regression to me.
(In reply to comment #46) > (In reply to comment #44) > > (In reply to comment #43) > > > (In reply to comment #42) > > > > Good luck convincing elmarco about the diff. :) > > > > > > Attachment 219295 [details] [details] [details] has been pushed without being ACK'ed first, we still have the > > > option of reverting it until spice-gtk sees an actual release. > > > > FYI, maintainers do have the right to push patches w/o getting an ACK. > > I disagree with that. Yeah, I think all active contributors should have a consensus too. So if Christophe as good reasons to be opposed to it, and there is no schedule pressure, we should hold it. > > Unless > > you can point out some regressions caused by this patch, I don't think we need > > to revert this just because we disagree with elmarco on the exact definition of > > a release. > > not being able to do a proper Boxes 3.5 release next Monday sounds like a bad > regression to me. However, in this case, I am 100% convinced that naming a tarball 0.12.101 or 0.13-pre1 or whatever is really just convention and doesn't change anything. So please Christophe, accept once and for all that there is no difference except naming stuff. 0.13 will be released shortly after it has received some testing. And it's chicken-egg problem just to ask me to release now.
(In reply to comment #47) > (In reply to comment #46) > > (In reply to comment #44) > > > (In reply to comment #43) > > > > (In reply to comment #42) > > > > > Good luck convincing elmarco about the diff. :) > > > > > > > > Attachment 219295 [details] [details] [details] [details] has been pushed without being ACK'ed first, we still have the > > > > option of reverting it until spice-gtk sees an actual release. > > > > > > FYI, maintainers do have the right to push patches w/o getting an ACK. > > > > I disagree with that. > > Yeah, I think all active contributors should have a consensus too. So if > Christophe as good reasons to be opposed to it, and there is no schedule > pressure, we should hold it. I didn't mean to say otherwise. I've held many patches of mine for much longer than I wanted to.
Review of attachment 219967 [details] [review]: I don't get it. You seem to be moving reconnect_display call from 'Domain.started' signal to notify of the prop that the handler is setting. When machine is launched from within Boxes, connect_display gets called for it.
Review of attachment 219967 [details] [review]: Zeeshan: we need to wait until the machine is RUNNING, either resumed or started.
Review of attachment 219967 [details] [review]: Ah ok. Perhaps the log message needs a bit of improvement then?
(In reply to comment #47) > However, in this case, I am 100% convinced that naming a tarball 0.12.101 or > 0.13-pre1 or whatever is really just convention and doesn't change anything. So > please Christophe, accept once and for all that there is no difference except > naming stuff. 0.13 will be released shortly after it has received some testing. > And it's chicken-egg problem just to ask me to release now. 0.13-pre1 would be a very bad release number... Versioning is one issue, but the core issue with the current scheme is that you don't want to make development releases, but you still want spice-gtk users to depend on git snapshots for an undefinite amount of time. Just add a git tag, write some quick release notes about spice-gtk unstable release 0.12.101, and we'll have an unstable release that can be used as an external GNOME dependency. Even better, if spice-gtk starts following GNOME release schedule, we can switch spice-gtk git in jhbuild instead of depending on release tarballs.