GNOME Bugzilla – Bug 747855
wizard loose url when going back
Last modified: 2016-09-20 08:16:03 UTC
I painstakenly entered a qemu+ssh:// url into the first page, and on the summary page I realized I had forgotten something, so I hit back. And the url was gone :-( Very frustration-inducing.
Created attachment 324257 [details] [review] wizard: Persist source URL on page change Navigating back and forth has the side effect of cleaning the source wizard, which leads to losing the source URL. In order to fix this restore URL after cleanup.
Review of attachment 324257 [details] [review]: ::: src/wizard.vala @@ +80,3 @@ + + wizard_source.uri = temp_uri; + This seems more like a work-around, we shouldn't be resetting the URL in case of going back in the wizard. I think we should only be resetting URL if we arrive at the source page with wizard_source.page != SourcePage.URL.
Created attachment 324501 [details] [review] wizard: Persist source URL on page change Navigating back and forth has the side effect of cleaning the source wizard, which leads to losing the source URL because of the cleanup. In order to fix this cleanup the wizard source only if the source page does not contain the URL.
Review of attachment 324501 [details] [review]: ::: src/wizard.vala @@ +207,3 @@ source = null; + if (wizard_source.page != SourcePage.URL) + wizard_source.cleanup (); this should be internal detail of WizardSource and hence WizardSource.cleanup() should be checking this. Also that way in future if WizardSource does need to cleanup some resources regardless of the page, the changes will be limited to WizardSource.
Created attachment 324561 [details] [review] wizard-source: Persist source URL on page change Navigating back and forth has the side effect of cleaning up the source wizard, which leads to losing the source URL because of the cleanup. In order to fix this remove the URL only if the source page does not contain the URL.
Review of attachment 324561 [details] [review]: Almost there! The commit log could be improved. Usually I'd just do this for you but if you are going to be working on Boxes for months, I'd like you to learn. Hope is that with more you contribute, the less I'll have to nitpick like this. :) * Shortlog isn't exactly correct any more since from the point of view of wizard-source, page is actually not changing and we are just not clearing up the URL on clean-up if page is URL. * "Navigating back and forth" * isn't correct, we do clean-up only on arriving to source page. * you need to mention you are talking about wizard (since the context isn't obvious from shortlog any more). * "source wizard" -> "wizard source". * "source page does not contain the URL" is not what you are checking for. "wizard source is not on URL page" would be a correct description.
Oh and you forgot to obsolete old patch again. Please do that.
Created attachment 324888 [details] [review] wizard-source: Keep uri on cleanup if page is URL Arriving on the source page has the side effect of cleaning the uri on the wizard source. In order to fix this do not erase the uri when arriving to source page and the wizard source page is URL.
Not using Review link for review since my only comments are on log. Just some nitpicks that I'll fix while pushing.. (In reply to Radu Stochitoiu from comment #8) > Created attachment 324888 [details] [review] [review] > wizard-source: Keep uri on cleanup if page is URL > > Arriving on the source page has the side effect of cleaning the uri on the * uri -> URI. Remember, this part is supposed to be normal English so abbreviations should be capitalized. * You missed/forgot this from previous review: "you need to mention you are talking about wizard (since the context isn't obvious from shortlog any more)". * Max 74 characters per line. > wizard source. In order to fix this do not erase the uri when arriving to > source > page and the wizard source page is URL. While I'm very pedantic about commit logs, goal is to help you become a good writer of both code and documentation (commit log is a kind of documentation and IMO the most important bit for developers) over time, and not to make me happy to get your code merged as soon as possible. Just wanted to let you know. :)
Created attachment 325018 [details] [review] wizard-source: Keep URI on cleanup if on URL page Navigating back to the source page in wizard, has the side effect of cleaning-up the wizard source. This means also clearing the URI, which is very unfriendly to user who could have painstakenly hand written the URI but made a small mistake while doing so, and wants to go back to simply fix his/her minor mistake. So let's not clear the UI as part of wizard source clean-up if source is on URL page.
Attachment 325018 [details] pushed as 07f2d91 - wizard-source: Keep URI on cleanup if on URL page