GNOME Bugzilla – Bug 677666
Fix recent regressions against live media
Last modified: 2016-03-31 14:02:13 UTC
Two patches to fix two regressions recently introduced in regards to live medias.
Created attachment 215885 [details] [review] wizard: Fix assumption that installer is Unattended
Created attachment 215886 [details] [review] wizard: Always skip setup page for live media Without this fix, we are presenting UI options to user that will be ignored completely.
Comment on attachment 215886 [details] [review] wizard: Always skip setup page for live media >From d34ebb380ab6b09f2d698147ead29a2faec65bc0 Mon Sep 17 00:00:00 2001 >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> >Date: Fri, 8 Jun 2012 04:05:29 +0300 >Subject: [PATCH] wizard: Always skip setup page for live media > >Without this fix, we are presenting UI options to user that will be >ignored completely. > >https://bugzilla.gnome.org/show_bug.cgi?id=677666 >--- > src/wizard.vala | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > >diff --git a/src/wizard.vala b/src/wizard.vala >index 34c1931..ccf6afe 100644 >--- a/src/wizard.vala >+++ b/src/wizard.vala >@@ -363,12 +363,9 @@ private class Boxes.Wizard: Boxes.UI { > skip_to = page - 1; > > if (install_media != null) { >- // go to last if skip review for live >- if (forwards && >- page == Boxes.WizardPage.SETUP && >- install_media.live && skip_review_for_live) >- // No setup or review required for live media >- skip_to = WizardPage.LAST; >+ if (forwards && page == Boxes.WizardPage.SETUP && install_media.live) >+ // No setup required for live media and also skip review if media passed from commandline >+ skip_to = skip_review_for_live ? WizardPage.LAST : WizardPage.REVIEW; Does the comment mean: // Skip review for live medias passed from the command line (skip_review_for_live is true) and go directly to the review page for live medias selected in the UI (skip_review_for_live is false) ? If yes, ACK, though it probably wouldn't hurt to improve a comment a bit, took me a while to understand what was going on ;)
Comment on attachment 215885 [details] [review] wizard: Fix assumption that installer is Unattended >From 4b4f5bb328f3c634b7ed3d313866a2ee036c2a4e Mon Sep 17 00:00:00 2001 >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> >Date: Fri, 8 Jun 2012 03:38:20 +0300 >Subject: [PATCH] wizard: Fix assumption that installer is Unattended > >https://bugzilla.gnome.org/show_bug.cgi?id=677666 >--- > src/wizard.vala | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > >diff --git a/src/wizard.vala b/src/wizard.vala >index 20a48b5..34c1931 100644 >--- a/src/wizard.vala >+++ b/src/wizard.vala >@@ -255,12 +255,14 @@ private class Boxes.Wizard: Boxes.UI { > return true; > > return_if_fail (install_media != null); >+ // Setup only needed for Unattended installers >+ if (!(install_media is UnattendedInstaller)) >+ return true; > Was this bug added in "Do not skip review page for unknown media" ? If so, setup() used to return after the foreach loop right below > foreach (var child in setup_vbox.get_children ()) > setup_vbox.remove (child); > >- var installer = install_media as UnattendedInstaller; >- installer.populate_setup_vbox (setup_vbox); >+ (install_media as UnattendedInstaller).populate_setup_vbox (setup_vbox); This turns half of the patch into noise... This bug just shows that explicit object casts are evil, and if (foo is Bar) even more so, but ACK with the question above answered (and addressed if needed). > setup_vbox.show_all (); > > return true; >-- >1.7.10.2
(In reply to comment #3) > (From update of attachment 215886 [details] [review]) > > >diff --git a/src/wizard.vala b/src/wizard.vala > >index 34c1931..ccf6afe 100644 > >--- a/src/wizard.vala > >+++ b/src/wizard.vala > >@@ -363,12 +363,9 @@ private class Boxes.Wizard: Boxes.UI { > > skip_to = page - 1; > > > > if (install_media != null) { > >- // go to last if skip review for live > >- if (forwards && > >- page == Boxes.WizardPage.SETUP && > >- install_media.live && skip_review_for_live) > >- // No setup or review required for live media > >- skip_to = WizardPage.LAST; > >+ if (forwards && page == Boxes.WizardPage.SETUP && install_media.live) > >+ // No setup required for live media and also skip review if media passed from commandline > >+ skip_to = skip_review_for_live ? WizardPage.LAST : WizardPage.REVIEW; > > Does the comment mean: > // Skip review for live medias passed from the command line > (skip_review_for_live is true) and go directly to the review page for live > medias selected in the UI (skip_review_for_live is false) > ? > > If yes, ACK, though it probably wouldn't hurt to improve a comment a bit, took > me a while to understand what was going on ;) Yes but in my eyes my explanation is easier to understand than what you wrote above. Sucks that these things are so subjective..
(In reply to comment #4) > (From update of attachment 215885 [details] [review]) > >From 4b4f5bb328f3c634b7ed3d313866a2ee036c2a4e Mon Sep 17 00:00:00 2001 > >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> > >Date: Fri, 8 Jun 2012 03:38:20 +0300 > >Subject: [PATCH] wizard: Fix assumption that installer is Unattended > > > > > > >https://bugzilla.gnome.org/show_bug.cgi?id=677666 > >--- > > src/wizard.vala | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > >diff --git a/src/wizard.vala b/src/wizard.vala > >index 20a48b5..34c1931 100644 > >--- a/src/wizard.vala > >+++ b/src/wizard.vala > >@@ -255,12 +255,14 @@ private class Boxes.Wizard: Boxes.UI { > > return true; > > > > return_if_fail (install_media != null); > >+ // Setup only needed for Unattended installers > >+ if (!(install_media is UnattendedInstaller)) > >+ return true; > > > > Was this bug added in "Do not skip review page for unknown media" ? Yes. >If so, > setup() used to return after the foreach loop right below > > > foreach (var child in setup_vbox.get_children ()) > > setup_vbox.remove (child); > > > >- var installer = install_media as UnattendedInstaller; > >- installer.populate_setup_vbox (setup_vbox); > >+ (install_media as UnattendedInstaller).populate_setup_vbox (setup_vbox); > > This turns half of the patch into noise... > > This bug just shows that explicit object casts are evil, and if (foo is Bar) > even more so, but ACK with the question above answered (and addressed if > needed). I'm sorry but I really didn't understand. How are 'foo is Bar' evil? We used to check for that in the dropped code and then we stopped checking if its the right type and started casting every object even if cast wasn't valid. There was warning on the console because of that and thats how I found out what/where the issue is. Having said that we should perhaps make populate_setup_vbox an empty virtual function in base InstallerMedia class and then we don't need any checks and casts.
(In reply to comment #5) > (In reply to comment #3) > > > > If yes, ACK, though it probably wouldn't hurt to improve a comment a bit, took > > me a while to understand what was going on ;) > > Yes but in my eyes my explanation is easier to understand than what you wrote > above. Sucks that these things are so subjective.. Depends where you come from, if you have no idea about how the code works, when skip_review_for_live is set, ... then my comment is probably much better. If you know that 'media passed from cmd line' == skip_review_for_live, then I can understand why you say your comment is better
(In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #3) > > > > > > > If yes, ACK, though it probably wouldn't hurt to improve a comment a bit, took > > > me a while to understand what was going on ;) > > > > Yes but in my eyes my explanation is easier to understand than what you wrote > > above. Sucks that these things are so subjective.. > > Depends where you come from, if you have no idea about how the code works, when > skip_review_for_live is set, ... then my comment is probably much better. > If you know that 'media passed from cmd line' == skip_review_for_live, then I > can understand why you say your comment is better In the comment I'm saying 'also skip review if media passed from commandline' and then actual code checks for 'skip_review_for_live' so I don't know what could be unclear here?
(In reply to comment #6) > I'm sorry but I really didn't understand. How are 'foo is Bar' evil? We used to > check for that in the dropped code and then we stopped checking if its the > right type and started casting every object even if cast wasn't valid. There > was warning on the console because of that and thats how I found out what/where > the issue is. > I didn't say that to imply your patch needs changes or that it was wrong, this was just a generic comment about abusing casts/runtime type checks with a language with enough OOP support, just ignore it. > Having said that we should perhaps make populate_setup_vbox an empty virtual > function in base InstallerMedia class and then we don't need any checks and > casts. Fits better with the language and is less error-prone indeed... I'm fine with your initial patch btw.
(In reply to comment #8) > > In the comment I'm saying 'also skip review if media passed from commandline' > and then actual code checks for 'skip_review_for_live' so I don't know what > could be unclear here? It's not immediatly obvious to me that skip_review_for_live is synonymous with skip_review_for_media_passed_from_command_line ;)
(In reply to comment #9) > (In reply to comment #6) > > > I'm sorry but I really didn't understand. How are 'foo is Bar' evil? We used to > > check for that in the dropped code and then we stopped checking if its the > > right type and started casting every object even if cast wasn't valid. There > > was warning on the console because of that and thats how I found out what/where > > the issue is. > > > > I didn't say that to imply your patch needs changes or that it was wrong, this > was just a generic comment about abusing casts/runtime type checks with a > language with enough OOP support, just ignore it. Ah ok. As I said, I just didn't understand what you meant. :) > > Having said that we should perhaps make populate_setup_vbox an empty virtual > > function in base InstallerMedia class and then we don't need any checks and > > casts. > > Fits better with the language and is less error-prone indeed... I'm fine with > your initial patch btw. Yeah, I can put this in a separate patch.. For now, I just added to my todo file..
(In reply to comment #10) > (In reply to comment #8) > > > > In the comment I'm saying 'also skip review if media passed from commandline' > > and then actual code checks for 'skip_review_for_live' so I don't know what > > could be unclear here? > > It's not immediatly obvious to me that skip_review_for_live is synonymous with > skip_review_for_media_passed_from_command_line ;) Looking at the short comment just above the line, shouldn't it be?
(In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #8) > > > > > > In the comment I'm saying 'also skip review if media passed from commandline' > > > and then actual code checks for 'skip_review_for_live' so I don't know what > > > could be unclear here? > > > > It's not immediatly obvious to me that skip_review_for_live is synonymous with > > skip_review_for_media_passed_from_command_line ;) > > Looking at the short comment just above the line, shouldn't it be? It wasn't for me at least... I didn't get at all what the 2nd part was for, and I even thought the changelog was wrong before I looked harder at the code. When I saw skip_review_for_live, I assumed this would be set for all live images for some reason.
Created attachment 216133 [details] [review] wizard: Always skip setup page for live media Without this fix, we are presenting UI options to user that will be ignored completely.
Review of attachment 216133 [details] [review]: ack
Attachment 215885 [details] pushed as 6af5eb9 - wizard: Fix assumption that installer is Unattended Attachment 216133 [details] pushed as 8b68c8b - wizard: Always skip setup page for live media