GNOME Bugzilla – Bug 700470
setup -> summary -> setup -> summary transition broken
Last modified: 2016-03-31 13:22:07 UTC
When doing that, something fails and it goes out of the wizard.
NB: doing an express install
Working on this..
Created attachment 244884 [details] [review] installer: Don't emit 'user_wants_to_create' when disabling toggle While its a good idea to signal the wizard that user wants to setup the VM already and for wizard to automatically move to next page when user toggle's off the express install but we must ensure we don't do this when we toggle it off ourselves in case of errors from installer setup. Without this patch, we end up launching the VM if installer setup fails for some reason.
Created attachment 244885 [details] [review] wizard: Unref VMCreator instance on cancellation
Created attachment 244886 [details] [review] vm-creator: Make sure to clean up installer media
Created attachment 244887 [details] [review] Don't clean up on error to setup unattended install The cleaned-up files still might be needed if user goes back in wizard and still chooses to select unattended install. We allow user to do that because he/she might manage to somehow work around the problem by choosing different setup parameters.
Created attachment 244888 [details] [review] installer: Delay clean-up of temp. unattended files Clean-up the unattended files in the method meant for cleanups, clean_up. Now that we ensure that clean_up is always called, this patch is not expected to cause unattended files to leak. This combined with last few patches, fixes the issue of setup->review->setup->review wizard transition, while keeping express installation selected, resulting in an error.
These patches are on top of patch in bug#700477. I'll rebase if needed.
Review of attachment 244884 [details] [review]: It would probably be better to never move to the next page when changing that switch, I've found that behaviour quite weird during my testing, and this would hopefully avoid the need for that ugly patch.
Review of attachment 244884 [details] [review]: I disagree. This behavior although hard on us is one of those simple small things that you could do to make your user really happy. However, I'd defer to UI designers here.
Review of attachment 244884 [details] [review]: While this might be nice if you know exactly what the switch does, moving to the next page as soon as it's changed makes things much harder to undo if you clicked on it by mistake, if you just wanted to see what happens when using it, ...
Review of attachment 244884 [details] [review]: OK so jimmac agrees with you so I'll revert the patch that introduce this change instead.
Created attachment 244927 [details] [review] Don't jump to next page on express install untoggled While I thought this is a good idea, teuf and jimmac agree that this is weird and unexpected behavior. Moreover, it complicates the code and requires hacks to not mistrigger this. Without this patch, we end up launching the VM if installer setup fails for some reason.
Review of attachment 244887 [details] [review]: I'm not sold on that one, as prepare_for_installation() is able to recreates the files that are being cleaned up, calling clean_up() here should not be an issue.
Review of attachment 244888 [details] [review]: There's a big difference between these unattended_files/secondary_unattended_files and the rest of the data cleaned up by clean_up(). The unattended_files variables hold files that are needed to generate the actual unattended installation floppy/cd, so they are no longer needed once the unattended installation media have been successfully created/have started to be used. On the other hand, clean_up() currently takes care of deleting the unattended installation media that need to be alive for the whole duration of the install. If the current cleanup in prepare_for_installation is causing issues when going backward/forward in the wizard, then I'd prefer that we have a 2nd cleanup method that is called when the installation starts rather than when the installation ends, as the UnattendedFile classes may hold open some mounts, ... If having the cleanup in prepare_for_installation does not cause issues, I'd rather it stays there so that we cleanup things as soon as we on longer need them.
Review of attachment 244927 [details] [review]: eia
Review of attachment 244888 [details] [review]: You got a point, especially with open mounts. I'll look into this..
Created attachment 244989 [details] [review] installer: Introducing clean_up_preparation_cache method Add a separate method for cleaning up any cache created by 'prepare' method that need to be kept around until VM is created and launched. This is kept separate from 'clean_up' method as that is meant to clean data that is needed until VM is completely installed. To ensure clean_up_preparation_cache is always called, we also call it from 'clean_up' method. This fixes the issue of setup->review->setup->review wizard transition, while keeping express installation selected, resulting in an error.
Review of attachment 244989 [details] [review]: Can you add more details to the end of the log about what was causing the error? Looks good otherwise, thanks.
Review of attachment 244927 [details] [review]: Actually, NACK, this breaks with Windows which support unattended installs. This commit needs to revert part of 0461f032, removing the line is not enough.
Created attachment 245038 [details] [review] Don't jump to next page on express install untoggled v2: Ensure sensitivity of 'Continue' button is still updated on toggling express install.
Attachment 244885 [details] pushed as 92948bb - wizard: Unref VMCreator instance on cancellation Attachment 244886 [details] pushed as 4270fca - vm-creator: Make sure to clean up installer media Attachment 244989 [details] pushed as 533c916 - installer: Introducing clean_up_preparation_cache method Attachment 245038 [details] pushed as 9d37899 - Don't jump to next page on express install untoggled