GNOME Bugzilla – Bug 686328
Fix Continue not enabled when doing non-express install of XP
Last modified: 2016-03-31 14:01:00 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.
Created attachment 226682 [details] [review] Fix Continue not enabled when doing non-express install of XP
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;
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.
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.
Review of attachment 227191 [details] [review]: You want to update the log details. Looks good otherwise.
Attachment 227191 [details] pushed as 530d269 - Fix Continue not enabled when doing non-express install of XP