After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 696399 - gnome-boxes can not custmoize Memory and Maximum Disk Size
gnome-boxes can not custmoize Memory and Maximum Disk Size
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: wizard
3.7.x
Other Linux
: Normal normal
: 3.22
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-03-22 16:00 UTC by sangu
Modified: 2016-03-31 13:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Make UI.previous_ui_state public readable (774 bytes, patch)
2013-03-25 15:51 UTC, Zeeshan Ali
committed Details | Review
libvirt-machine-props: Allow editing system props before creation (2.76 KB, patch)
2013-03-25 15:51 UTC, Zeeshan Ali
reviewed Details | Review
libvirt-machine-props: Allow editing system props before creation (2.29 KB, patch)
2013-03-25 21:34 UTC, Zeeshan Ali
committed Details | Review

Description sangu 2013-03-22 16:00:47 UTC
gnome-boxes can not custmoize Memory and Maximum Disk Size
1. select a file
Fedora 18 liveCD
2. Click customize...
3. Click memory bar or Maximum Disk Size bar
bar doesn't move

gnome-boxes-3.7.92-1.fc19.x86_64
$ gnome-boxes

(gnome-boxes:3409): Boxes-CRITICAL **: boxes_spice_display_construct: assertion `(_tmp0_ != 0) || (_tmp1_ != 0)' failed

(gnome-boxes:3409): Boxes-CRITICAL **: boxes_spice_display_construct: assertion `(_tmp0_ != 0) || (_tmp1_ != 0)' failed

(gnome-boxes:3409): Boxes-CRITICAL **: boxes_spice_display_construct: assertion `(_tmp0_ != 0) || (_tmp1_ != 0)' failed
Comment 1 Zeeshan Ali 2013-03-22 16:18:19 UTC
Yikes, that must be a regression from commit 59dbcc5 "Disallow changing of system props during install".
Comment 2 Zeeshan Ali 2013-03-25 15:51:06 UTC
Created attachment 239780 [details] [review]
Make UI.previous_ui_state public readable
Comment 3 Zeeshan Ali 2013-03-25 15:51:10 UTC
Created attachment 239781 [details] [review]
libvirt-machine-props: Allow editing system props before creation

While we shouldn't allow these props to be modified during
installation/live session, we want users to be able to modify them
before they hit 'create' in the wizard.
Comment 4 Christophe Fergeau 2013-03-25 19:26:16 UTC
Comment on attachment 239781 [details] [review]
libvirt-machine-props: Allow editing system props before creation

>From 51a572677ee49f8d3a9feaa999f1fbf3ce9b0f12 Mon Sep 17 00:00:00 2001
>From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
>Date: Mon, 25 Mar 2013 17:48:34 +0200
>Subject: [PATCH] libvirt-machine-props: Allow editing system props before
> creation
>
>While we shouldn't allow these props to be modified during
>installation/live session, we want users to be able to modify them
>before they hit 'create' in the wizard.
>
>https://bugzilla.gnome.org/show_bug.cgi?id=696399
>---
> data/disk.img                       | Bin 1474560 -> 1474560 bytes
> src/libvirt-machine-properties.vala |  10 ++++++----
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/data/disk.img b/data/disk.img
>index f7eb640e41c1eb85e5eb96ea0278a3e8c20b8c8b..6b9761ddecea8e4fdaea97cb256afb75a0ac4e33 100644
>GIT binary patch
>delta 90
>zcmV~$sS(0J06@{<3dfRzB%pEl-vI*2#&0Jm7V$kQg=8jeP!sRvoN`XzTNqzcABSta
>nKOY}tAxl}wS~gP1R(7(Ng9JHBDV5aH$Vpo1q?hx|<@f#r&V?Eg
>
>delta 90
>zcmXBByA^^!006-!2%a4HhlqfTe9O6*I`9ZKw;9{8CIV-2PC2K~9G7to)8B?1q>xgM
>eQb{c*338T3qO{V<MXqv_yFBD6FX_JwzxNJAg&0Ht
>

Does not belong here

>diff --git a/src/libvirt-machine-properties.vala b/src/libvirt-machine-properties.vala
>index 8c486e8..7465f39 100644
>--- a/src/libvirt-machine-properties.vala
>+++ b/src/libvirt-machine-properties.vala
>@@ -408,8 +408,9 @@ private async void mark_recommended_resources (SizeProperty? ram_property, SizeP
>                                               max_ram * Osinfo.KIBIBYTES,
>                                               64 * Osinfo.MEBIBYTES,
>                                               FormatSizeFlags.IEC_UNITS);
>-            if (VMConfigurator.is_install_config (machine.domain_config) ||
>-                VMConfigurator.is_live_config (machine.domain_config))
>+            if ((VMConfigurator.is_install_config (machine.domain_config) ||
>+                 VMConfigurator.is_live_config (machine.domain_config)) &&
>+                App.app.previous_ui_state != Boxes.UIState.WIZARD)

Not a big fan of checking ui_state here and there, even less a big fan of exposing publicly previous_ui_state. Maybe we could have a Boxes.UIState.CUSTOMIZE to make the difference between the 2 here? Or set a boolean in App from select_item to remember where we came from?
Comment 5 Zeeshan Ali 2013-03-25 21:31:46 UTC
(In reply to comment #4)
> (From update of attachment 239781 [details] [review])
> >From 51a572677ee49f8d3a9feaa999f1fbf3ce9b0f12 Mon Sep 17 00:00:00 2001
> >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
> >Date: Mon, 25 Mar 2013 17:48:34 +0200
> >Subject: [PATCH] libvirt-machine-props: Allow editing system props before
> > creation
> >
> >While we shouldn't allow these props to be modified during
> >installation/live session, we want users to be able to modify them
> >before they hit 'create' in the wizard.
> >
> >https://bugzilla.gnome.org/show_bug.cgi?id=696399
> >---
> > data/disk.img                       | Bin 1474560 -> 1474560 bytes
> > src/libvirt-machine-properties.vala |  10 ++++++----
> > 2 files changed, 6 insertions(+), 4 deletions(-)
> >
> >diff --git a/data/disk.img b/data/disk.img
> >index f7eb640e41c1eb85e5eb96ea0278a3e8c20b8c8b..6b9761ddecea8e4fdaea97cb256afb75a0ac4e33 100644
> >GIT binary patch
> >delta 90
> >zcmV~$sS(0J06@{<3dfRzB%pEl-vI*2#&0Jm7V$kQg=8jeP!sRvoN`XzTNqzcABSta
> >nKOY}tAxl}wS~gP1R(7(Ng9JHBDV5aH$Vpo1q?hx|<@f#r&V?Eg
> >
> >delta 90
> >zcmXBByA^^!006-!2%a4HhlqfTe9O6*I`9ZKw;9{8CIV-2PC2K~9G7to)8B?1q>xgM
> >eQb{c*338T3qO{V<MXqv_yFBD6FX_JwzxNJAg&0Ht
> >
> 
> Does not belong here

Yikes. I'll remove..

> >diff --git a/src/libvirt-machine-properties.vala b/src/libvirt-machine-properties.vala
> >index 8c486e8..7465f39 100644
> >--- a/src/libvirt-machine-properties.vala
> >+++ b/src/libvirt-machine-properties.vala
> >@@ -408,8 +408,9 @@ private async void mark_recommended_resources (SizeProperty? ram_property, SizeP
> >                                               max_ram * Osinfo.KIBIBYTES,
> >                                               64 * Osinfo.MEBIBYTES,
> >                                               FormatSizeFlags.IEC_UNITS);
> >-            if (VMConfigurator.is_install_config (machine.domain_config) ||
> >-                VMConfigurator.is_live_config (machine.domain_config))
> >+            if ((VMConfigurator.is_install_config (machine.domain_config) ||
> >+                 VMConfigurator.is_live_config (machine.domain_config)) &&
> >+                App.app.previous_ui_state != Boxes.UIState.WIZARD)
> 
> Not a big fan of checking ui_state here and there, even less a big fan of
> exposing publicly previous_ui_state. Maybe we could have a
> Boxes.UIState.CUSTOMIZE to make the difference between the 2 here?

Introducing a new UI state isn't something I would like to do just before rolling out first stable release of the cycle.

> Or set a
> boolean in App from select_item to remember where we came from?

I don't see how that is different. The info being exposed is practically the same.
Comment 6 Zeeshan Ali 2013-03-25 21:34:42 UTC
Created attachment 239834 [details] [review]
libvirt-machine-props: Allow editing system props before creation

v2: Remove bogus changes.
Comment 7 Zeeshan Ali 2013-03-26 00:44:50 UTC
Attachment 239780 [details] pushed as 6ff0bd8 - Make UI.previous_ui_state public readable
Attachment 239834 [details] pushed as 6611634 - libvirt-machine-props: Allow editing system props before creation
Comment 8 Christophe Fergeau 2013-03-26 09:48:59 UTC
(In reply to comment #5)
> > 
> > Not a big fan of checking ui_state here and there, even less a big fan of
> > exposing publicly previous_ui_state. Maybe we could have a
> > Boxes.UIState.CUSTOMIZE to make the difference between the 2 here?
> 
> Introducing a new UI state isn't something I would like to do just before
> rolling out first stable release of the cycle.

Yup, fair enough, that was my feeling too, does not hurt to suggest it ;)

> 
> > Or set a
> > boolean in App from select_item to remember where we came from?
> 
> I don't see how that is different. The info being exposed is practically the
> same.

Yup, though in one case you are exposing the inner implementation of the UI state machine to the outside world, which I rather avoid. By wrapping this in an intermediate class, you keep the LibvirtMachineProperties class decoupled from the UI code. Also, by only providing a way to differentiate between properties/customize, you also prevent other code using previous_ui_state for other reasons, which is a good thing from a class containment point of view.