GNOME Bugzilla – Bug 734675
Take needs-internet flag for express installation script into account
Last modified: 2016-03-31 13:22:07 UTC
Patches are appended. I tested the following scenarios: - Fedora express installation ignores internet connection - Debian express installation only works with internet connection In the latter case I tested specifically: - express_toggle is only sensitive and active when internet connection is present - applies when internet connection is initially present and then toggled at every time - applies when internet connection is initially unpresent and then toggled at every time - Latest user defined value of express_toggle.active is not overwritten when internet connection is toggled twice
Created attachment 283216 [details] [review] unattended-setup-box: Detect internet connection if needed This is needed to obey the needs-internet flag from libosinfo for the express installation.
Created attachment 283217 [details] [review] unattended-setup-box: Show message when internet connection is needed
Created attachment 283218 [details] [review] unattended-installer: Obey needs-internet from scripts
Review of attachment 283216 [details] [review]: ::: src/unattended-setup-box.vala @@ +88,3 @@ string? product_key_format; + public UnattendedSetupBox (bool live, string? product_key_format, bool needs_internet = false) { There is only one caller of UnttendedSetupBox and it will pass needs_internet directly from script so no need for default value. @@ +106,3 @@ } + private void setup_express_toggle(bool live, bool needs_internet) { space before ( @@ +116,3 @@ + var network_monitor = NetworkMonitor.get_default (); + update_express_toggle(network_monitor.get_network_available ()); + network_monitor.network_changed.connect ((available) => { Unless you take care of it in later patches (that I haven't yet looked at), we need to ensure we remove this connection once we are out of setup step or wizard. @@ +117,3 @@ + update_express_toggle(network_monitor.get_network_available ()); + network_monitor.network_changed.connect ((available) => { + update_express_toggle(network_monitor.get_network_available ()); space before ( @@ +124,3 @@ + private bool express_toggle_was_active = false; + + private void update_express_toggle(bool network_available) { space missing before ( @@ +136,3 @@ + express_toggle.active = false; + } + } I don't think this needs to be so complicated. When network is not available, just make the toggle inactive and insensitive and when network is available, just make it sensitive. if (network_available) express_toggle.sensitive = false; else { express_toggle.sensitive = false; express_toggle.active = false; }
Review of attachment 283216 [details] [review]: oh and you are not 'obeying' anything, you are taking needs-internet param of *Osinfo.InstallScript* into account.
Review of attachment 283217 [details] [review]: Sure you can remove 'connection' from shortlog to make it shorter w/o sacrificing any clarity. :) ::: data/ui/unattended-setup-box.ui @@ +24,3 @@ </child> <child> + <object class="GtkLabel" id="internet_label"> I think we should display it like we do no_kvm warning: GtkInfoBar. We'd want to use 'info' message type and dialog-information icon rather than 'warning' and dialog-warning though. @@ +25,3 @@ <child> + <object class="GtkLabel" id="internet_label"> + <property name="label" translatable="yes">Please make sure you have an internet connection in order to activate express installation for this media.</property> Connection is not just needed for enabling express installation but during the installation. Also 'for this media' is pretty redundant as its not telling anything new to user. How about simply "Express installation of OS_NAME requires internet connection."? ::: src/unattended-setup-box.vala @@ +130,3 @@ if (!express_toggle.sensitive) { express_toggle.active = express_toggle_was_active; + internet_label.visible = false; This should be always visible if internet connection is required by script. This is so that user knows that internet connection is needed through out the whole installation process.
Review of attachment 283216 [details] [review]: ::: src/unattended-setup-box.vala @@ +88,3 @@ string? product_key_format; + public UnattendedSetupBox (bool live, string? product_key_format, bool needs_internet = false) { This caller is not yet aware of this parameter. The intention was to introduce it with a default value and pass the parameter later. The second thing is that the API is simpler because of the possibility to ignore this parameter. @@ +106,3 @@ } + private void setup_express_toggle(bool live, bool needs_internet) { Yeah, sorry. This got back in my habits somehow... @@ +116,3 @@ + var network_monitor = NetworkMonitor.get_default (); + update_express_toggle(network_monitor.get_network_available ()); + network_monitor.network_changed.connect ((available) => { I dont. Very valid point. @@ +136,3 @@ + express_toggle.active = false; + } + } I disagree. When I toggle the state of the internet connection twice I would expect the button to be sensitive if it was sensitive before. It would be very stupid if toggling the WLAN switch on my laptop results in the express_toggle be sensitive but disabled wouldnt it?
Review of attachment 283217 [details] [review]: Ok, just for clarification, we want: (1) A label that hints on the necessity of the internet connection (2) A GtkInfoBar with a message when the connection is missing initially/goes missing
Review of attachment 283218 [details] [review]: Same comment about 'obeying'. Good otherwise.
Review of attachment 283216 [details] [review]: ::: src/unattended-setup-box.vala @@ +88,3 @@ string? product_key_format; + public UnattendedSetupBox (bool live, string? product_key_format, bool needs_internet = false) { You can make the caller aware then. :) Just pass 'false' in this patch. Regarding the secondary thing, no the API is just more complex with default values if there is only one caller and its not very likely there is going to be more callers. @@ +136,3 @@ + express_toggle.active = false; + } + } Yeah but how often would it be that your internet connect goes up and down while you are at setup page? If your internet connection is so flaky, your installation is very likely to fail anyway even if you get to choose express installation here. There is no need to complicate the code for such corner cases, especially since its not so terrible if user has to just make one more click if this case is realized.
Review of attachment 283217 [details] [review]: Ok, just for clarification, we want: (1) A label that hints on the necessity of the internet connection (2) A GtkInfoBar with a message when the connection is missing initially/goes missing No! We only want infobar and its always displayed if internet connection is required.
Created attachment 283487 [details] [review] unattended-setup-box: Detect internet connection if needed This is needed to take the needs-internet flag from libosinfo into account for the express installation.
Created attachment 283488 [details] [review] unattended-setup-box: Show hint when internet connection needed
Created attachment 283489 [details] [review] unattended-installer: Take needs-internet into account
Created attachment 283490 [details] [review] unattended-setup-box: Add missing private keywords
Created attachment 283510 [details] [review] unattended-setup-box: Show hint when internet connection needed
Created attachment 283511 [details] [review] unattended-installer: Take needs-internet into account
Created attachment 283512 [details] [review] unattended-setup-box: Add missing private keywords
Review of attachment 283487 [details] [review]: As pointed out in last review: "needs-internet flag from libosinfo" -> "needs-internet param of Osinfo.InstallScript" Almost there. ::: src/unattended-setup-box.vala @@ +88,3 @@ string? product_key_format; + private NetworkMonitor? network_monitor; No need to store the singleton but rather store the signal id. @@ +111,3 @@ + public void clean_up () { + if (network_monitor != null) + network_monitor.network_changed.disconnect (update_express_toggle); I thought 'disconnect' takes an signal handler id you get from 'connect'. We'd want to use that in here anyway. Also you want to set it to '0' after disconnecting signal.
Review of attachment 283510 [details] [review]: * s/when/if/ for shorter shortlog * You didn't remove 'connection' from shortlog Good otherwise. ::: data/ui/unattended-setup-box.ui @@ +30,3 @@ + <object class="GtkLabel" id="needs_internet_label"> + <property name="visible">True</property> + <property name="label" translatable="yes">Express installation of this OS requires an internet connection.</property> well if we can't mention the OS, not much point in "of this OS" part here.
Review of attachment 283487 [details] [review]: ::: src/unattended-setup-box.vala @@ +111,3 @@ + public void clean_up () { + if (network_monitor != null) + network_monitor.network_changed.disconnect (update_express_toggle); https://wiki.gnome.org/Projects/Vala/SignalsAndCallbacks#Disconnecting_Signals I find that clearer than storing the handler. So two options: 1. Go your way, store the handler and disconnect the signal if the handler id is non-zero 2. Store needs_internet as member and disconnect the signal via NetworkMonitor.get_default ().network_changed.disconnect (update_express_toggle); if needs_internet
Review of attachment 283510 [details] [review]: ::: data/ui/unattended-setup-box.ui @@ +30,3 @@ + <object class="GtkLabel" id="needs_internet_label"> + <property name="visible">True</property> + <property name="label" translatable="yes">Express installation of this OS requires an internet connection.</property> well it points out that this is only valid for this OS and not for all of them. Alternative: insert name of the OS in the label. I'd pledge for the latter even though its more work. (Need to pass the os name to the constructor of UnattendedSetupBox and set the label appropriately.)
Review of attachment 283511 [details] [review]: A sentence in details section would be nice to tell whats 'needs-internet' ?
Review of attachment 283512 [details] [review]: ack
Review of attachment 283487 [details] [review]: ::: src/unattended-setup-box.vala @@ +111,3 @@ + public void clean_up () { + if (network_monitor != null) + network_monitor.network_changed.disconnect (update_express_toggle); unless we don't get an error/warning if handler is not connected, we don't need to keep anything and can simply disconnect. Otherwise, we go for option#1.
Review of attachment 283510 [details] [review]: ::: data/ui/unattended-setup-box.ui @@ +30,3 @@ + <object class="GtkLabel" id="needs_internet_label"> + <property name="visible">True</property> + <property name="label" translatable="yes">Express installation of this OS requires an internet connection.</property> fair enough. :) Go for latter. I'd just not set label in here then as that would mean translators having to translate a string that is very unlikely going to be shown since we are going to set from code.
Created attachment 283530 [details] [review] unattended-setup-box: Detect internet connection if needed This is needed to take the needs-internet param from Osinfo.InstallScript into account for the express installation.
Created attachment 283531 [details] [review] unattended-setup-box: Show hint if internet needed
Created attachment 283532 [details] [review] unattended-installer: Take needs-internet into account The needs-internet param from Osinfo.InstallScript signals that an express installation needs an internet connection in order to run without user intervention. https://bugzilla.gnome.org/show_bug.cgi?id=734675 Conflicts: src/unattended-installer.vala
Created attachment 283533 [details] [review] unattended-setup-box: Add missing private keywords
Review of attachment 283533 [details] [review]: patch rebased without merge, no difference to previous version
Review of attachment 283530 [details] [review]: * s/connection// in shortlog, unless it already fits 50 chars? * "param from" -> "param of" good otherwise.
Review of attachment 283531 [details] [review]: good otherwise. ::: src/unattended-setup-box.vala @@ +98,2 @@ setup_express_toggle (live, needs_internet); + var msg = _("Express installation of %s requires an internet connection.").replace ("%s", os_name); use str.printf rather than replace. :)
Review of attachment 283532 [details] [review]: ack
Review of attachment 283533 [details] [review]: Yeah, just move this at beginning of other patches since its not required by any patch and so you won't have to reupload.
Created attachment 283539 [details] [review] unattended-setup-box: Detect internet if needed This is needed to take the needs-internet param of Osinfo.InstallScript into account for the express installation.
Created attachment 283540 [details] [review] unattended-setup-box: Show hint if internet needed
Created attachment 283541 [details] [review] unattended-installer: Take needs-internet into account The needs-internet param from Osinfo.InstallScript signals that an express installation needs an internet connection in order to run without user intervention. https://bugzilla.gnome.org/show_bug.cgi?id=734675 Conflicts: src/unattended-installer.vala
Review of attachment 283539 [details] [review]: ACK
Review of attachment 283540 [details] [review]: ACK
Review of attachment 283541 [details] [review]: ACK
Created attachment 283551 [details] [review] unattended-setup-box: Add missing private keywords
Created attachment 283552 [details] [review] unattended-setup-box: Detect internet if needed This is needed to take the needs-internet param of Osinfo.InstallScript into account for the express installation.
Created attachment 283553 [details] [review] unattended-setup-box: Show hint if internet needed
Created attachment 283554 [details] [review] unattended-installer: Take needs-internet into account The needs-internet param from Osinfo.InstallScript signals that an express installation needs an internet connection in order to run without user intervention. https://bugzilla.gnome.org/show_bug.cgi?id=734675 Conflicts: src/unattended-installer.vala
Pushed all but when I said I want an infobar like that we show for non-kvm, I actually meant it should look like that. :) i-e on the top and stretched across the wizard. Attachment 283551 [details] pushed as efd53ea - unattended-setup-box: Add missing private keywords Attachment 283552 [details] pushed as 61bd553 - unattended-setup-box: Detect internet if needed Attachment 283553 [details] pushed as 9e54be6 - unattended-setup-box: Show hint if internet needed Attachment 283554 [details] pushed as a52d29b - unattended-installer: Take needs-internet into account
(In reply to comment #46) > Pushed all but when I said I want an infobar like that we show for non-kvm, I > actually meant it should look like that. :) i-e on the top and stretched across > the wizard. IIRC, in the end we didn't like that. So closing this now.