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 686328 - Fix Continue not enabled when doing non-express install of XP
Fix Continue not enabled when doing non-express install of XP
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2012-10-17 19:38 UTC by Marc-Andre Lureau
Modified: 2016-03-31 14:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix Continue not enabled when doing non-express install of XP (1.67 KB, patch)
2012-10-17 19:38 UTC, Marc-Andre Lureau
reviewed Details | Review
Fix Continue not enabled when doing non-express install of XP (4.01 KB, patch)
2012-10-24 19:11 UTC, Marc-Andre Lureau
committed Details | Review

Description Marc-Andre Lureau 2012-10-17 19:38:01 UTC
Although I added a new property ready to solve the issue, I wonder if
it would make sense to have the switch at higher level than
UnattendedInstaller class instead... imho, it is at the wrong place.
Comment 1 Marc-Andre Lureau 2012-10-17 19:38:03 UTC
Created attachment 226682 [details] [review]
Fix Continue not enabled when doing non-express install of XP
Comment 2 Zeeshan Ali 2012-10-17 21:27:05 UTC
Review of attachment 226682 [details] [review]:

::: src/unattended-installer.vala
@@ +14,3 @@
     }
 
+    public virtual bool ready { get { return username != ""; } }

Since I had to name the property user_data_for_vm_creation_available to satisfy Christophe, I'd prefer some consistency and naming this very related property a bit verbose as well.

@@ -16,3 @@
     public override bool user_data_for_vm_creation_available {
         get {
-            return !express_toggle.active || username != "";

why not just do this at the top in both implementations?

if (!express_toggle.active)
   return true;
Comment 3 Marc-Andre Lureau 2012-10-24 09:54:19 UTC
Review of attachment 226682 [details] [review]:

::: src/unattended-installer.vala
@@ +14,3 @@
     }
 
+    public virtual bool ready { get { return username != ""; } }

right, I find the current naming not so elegant, but that is not the point.

The best thing I can propose instead is to have a method we can override and call it's parent to do an AND on conditions. (afaik, we can't do that with properties). The property could still exists to return that method value.

@@ -16,3 @@
     public override bool user_data_for_vm_creation_available {
         get {
-            return !express_toggle.active || username != "";

It should be done in one place. And it shouldn't be inside unattended installer class (where it's a bit too late). So for now, I moved it to base class, but instead it should go out imho.
Comment 4 Marc-Andre Lureau 2012-10-24 19:11:01 UTC
Created attachment 227191 [details] [review]
Fix Continue not enabled when doing non-express install of XP

Although I added a new property ready to solve the issue, I wonder if
it would make sense to have the switch at higher level than
UnattendedInstaller class instead... imho, it is at the wrong place.
Comment 5 Zeeshan Ali 2012-10-24 21:31:48 UTC
Review of attachment 227191 [details] [review]:

You want to update the log details. Looks good otherwise.
Comment 6 Marc-Andre Lureau 2012-10-26 10:08:08 UTC
Attachment 227191 [details] pushed as 530d269 - Fix Continue not enabled when doing non-express install of XP