GNOME Bugzilla – Bug 684219
ISO selection is not working properly
Last modified: 2016-03-31 14:02:19 UTC
After select an ISO from Wizard, Boxes never can select another ISO from File Selector. Steps to reproduce: 1 - Select an ISO from Wizard 2 - Go back to the Wizard selection screen 3 - Select an ISO from File Selector I did a small test trying to search a revision where this behavior is okay, I didn't find.
One way to solve this specific bug is diff --git a/src/wizard-source.vala b/src/wizard-source.vala index 57d140d..3fecd06 100644 --- a/src/wizard-source.vala +++ b/src/wizard-source.vala @@ -301,6 +301,8 @@ private void launch_file_selection_dialog () { dialog.filter.add_mime_type ("application/x-cd-image"); if (dialog.run () == Gtk.ResponseType.ACCEPT) { uri = dialog.get_uri (); + // clean install_media as this may be set already when going back in the wizard + install_media = null; url_entry.activate (); } but then you get the same bug when typing a spice:// URI by hand instead of picking an ISO in the file selector. Doing this helps: @@ -67,6 +67,8 @@ public override void get_preferred_height (out int minimum_height, out int natur main_vbox.grab_focus (); break; case SourcePage.URL: + warning("Source Page URL"); + install_media = null; url_entry.changed (); url_entry.grab_focus (); break; but then clicking on 'customize' is buggy, it still shows the properties of the first VM, not of the spice:// URI (which is not working, tracked in another bug). This is caused by vm_creator not being reset when going backwards in the wizard, but if I set it to null when going backward, then I get an extra empty 'Configure' step. Finally, 'source' is not reset either when going backwards or exiting the wizard, which means we have the opposite bug: - start creating a new box - enter a spice:// uri - go back/quit the wizard - try to click on an ISO from the list -> the spice:// uri keeps showing up I guess we could go for the easy fix right now (first one), and fix the spice:// URI case later as this is trickier, and quite untested currently (typing the URI is very slow as it seems to trigger the search code as I understand it).
Created attachment 224534 [details] [review] Reset install_media when it has become stale When clicking on an ISO from the ISO list, we already know 'install_media' so we can set it immediatly. When we select a media from the filechooser, we don't know the corresponding install_media immediatly, so we have to guess it later on. However, these 2 different situations conflict when the user first selects a media from the ISO list in the wizard, then goes back in the wizard and selects a file from the file chooser. The old media is then reused instead of the new one being used. This commit sets install_media to null when a file is selected in the file chooser so that it's guess later on instead of being reused. I chose not to clear install_media when navigating through the wizard in order to be able to reuse the media when going back in the wizard, and then forward without changing anything. An alternate fix that would likely work is to always reset it to null, and to reset it every time the URI change.
Created attachment 224535 [details] [review] Fix typo (cutomize)
Review of attachment 224534 [details] [review]: Looks ok to me, safe and simple
Review of attachment 224535 [details] [review]: ok
Attachment 224534 [details] pushed as 775dfd3 - Reset install_media when it has become stale Attachment 224535 [details] pushed as b3eb1aa - Fix typo (cutomize)
(In reply to comment #1) > Finally, 'source' is not reset either when going backwards or exiting the > wizard, which means we have the opposite bug: > - start creating a new box > - enter a spice:// uri > - go back/quit the wizard > - try to click on an ISO from the list > -> the spice:// uri keeps showing up > Reopening as this part of the bug hasn't been fixed.
Created attachment 226540 [details] [review] wizard: Reset install_media when it has become stale For details, please read log of commit 775dfd3f.
(In reply to comment #8) > > For details, please read log of commit 775dfd3f. No I won't. And this commit does something more than what is explained/described in the commit you refer to, this needs to be explained.
(In reply to comment #9) > (In reply to comment #8) > > > > For details, please read log of commit 775dfd3f. > > No I won't. Suit yourself. >And this commit does something more than what is > explained/described in the commit you refer to, this needs to be explained. For the love of whatever, this is a one line change that does exactly what the summary line says and the other commit in question has a log that explains the context more in detail (if anyone is interested to read). They both also have links to this bug. Feel free to change the commit log to suit your satisfaction levels, I have fixed the bug!
(In reply to comment #10) > >And this commit does something more than what is > > explained/described in the commit you refer to, this needs to be explained. > > For the love of whatever, this is a one line change that does exactly what the > summary line says and the other commit in question has a log that explains the > context more in detail (if anyone is interested to read). They both also have > links to this bug. The other commit does not describe the bug your change fixes, and bugzilla does not yet work when people are offline. So this needs to be in the commit log. > Feel free to change the commit log to suit your satisfaction levels, I have > fixed the bug! A good commit log is part of a proper bug fix.
(In reply to comment #10) > the other commit in question has a log that explains the > context more in detail (if anyone is interested to read). It also says "I chose not to clear install_media when navigating through the wizard in order to be able to reuse the media when going back in the wizard, and then forward without changing anything.", isn't it what your patch does? Explaining why this is good would also be interesting I guess
Review of attachment 226540 [details] [review]: ::: src/wizard-source.vala @@ +224,3 @@ install_media = media; uri = media.device_file; + install_media = null; Did you intentionally keep 'install_media = media;' ?
(In reply to comment #13) > Review of attachment 226540 [details] [review]: > > ::: src/wizard-source.vala > @@ +224,3 @@ > install_media = media; > uri = media.device_file; > + install_media = null; > > Did you intentionally keep 'install_media = media;' ? No and now that i had another look, the patch is wrong and will trigger a redundant detection of media. I'll submit another patch..
Created attachment 226663 [details] [review] wizard: Unset collection source on main page Instead of unsetting collection source just before preparing for a URI, we now do it each time user goes to main page of wizard source. With former approach, we don't unset collection source if user first enters URI manually, changes his/her mind at 'Review', goes back to 'Source' and chooses a ready media on the main page.
Review of attachment 226663 [details] [review]: have you verified you don't break command line usage?
so what if the user just wanted to correct port? he has to type it all again?
(In reply to comment #17) > so what if the user just wanted to correct port? he has to type it all again? No, we are not unsetting the URI itself but only the collection source (if it already exists), that shall be recreated if URI was kept/entered again. (In reply to comment #16) > Review of attachment 226663 [details] [review]: > > have you verified you don't break command line usage? Although not irrelevant afaict, I just tested and I didn't see any breakage there.
Review of attachment 226663 [details] [review]: ack, thanks for checking
Attachment 226663 [details] pushed as 00294f2 - wizard: Unset collection source on main page