GNOME Bugzilla – Bug 753172
crash when going back from create from url view
Last modified: 2016-09-20 08:15:55 UTC
I wanted to create ovirt pool so went into create from url view, entered ovirt:// Boxes told me that ovirt is unsupported (misconfiguration in jhbuild or a bug? ) so I clicked back and crash. 3.17.4.11-3ed5-dirty (jhbuild created today)
Created attachment 308655 [details] gdb backtrace
Installation of libgovirt and rebuilding helped. Ovirt:// protocol is not unsupported anymore nevertheless still not working. But that's another story tracked in 752966.
(In reply to vladimir benes from comment #0) > I wanted to create ovirt pool so went into create from url view, entered > ovirt:// Boxes told me that ovirt is unsupported (misconfiguration in > jhbuild or a bug? ) so I clicked back and crash. > > 3.17.4.11-3ed5-dirty (jhbuild created today) This is really strange cause here, Boxes doesn't enable the 'Continue' button if ovirt support is disabled at configure time and we don't show any errors in this case either.
NEEDINFO cause I might be misunderstanding this bug. If you can easily reproduce, maybe I can see it on your laptop during GUADEC?
Created attachment 324305 [details] [review] wizard: Fix crash when going back from enter url view When an erroneous URL (such as an empty or unsupported one) is entered in the source view, the back button is wrongfully made sensitive. This is due to a page change action that is canceled mid-way, when the URL validity is actually checked. In order to fix this, validate the URL before attempting to change the page.
Review of attachment 324305 [details] [review]: * Thanks for the patch, at least now I understand what the bug is about. :) I don't think the solution is correct though since sensitivity of 'Back' button at Source page should not depend on URL being valid or not. I think what happens is that we try to go to PREPARATION page on activation of the URL field and since that fails, we are back to SOURCE page but we don't update the sensitivity of 'Back' button. Since there is no page before SOURCE page, Boxes ends up crashing. This issue likely is caused by the fact that we used to have an INTRODUCTION page before SOURCE page but we failed to update all code accordingly when we removed that page. The correction solution would either be to not set sensitivity of 'Back' button before URL validation on PREPARATION page or just ensure that we disable the 'Back' button on arriving to SOURCE page. I'll prefer the latter approach. * As per commit digest guidelines: in the summary line/shortlog "Summarize the change itself in the shortlog, not the bug or effects/benefits (that goes in description)." https://wiki.gnome.org/Git/CommitMessages In this case the change was to valide URL before switching page. Doesn't matter anymore for this case but just for future reference. :) ::: src/wizard.vala @@ +194,3 @@ wizard_source.activated.connect(() => { + try { + prepare_for_location(wizard_source.uri, true); Space before (. @@ +196,3 @@ + prepare_for_location(wizard_source.uri, true); + } + catch(GLib.Error error) { catch goes on the same line as } before it. @@ +198,3 @@ + catch(GLib.Error error) { + window.notificationbar.display_error (error.message); + return; Empy line before return.
Created attachment 324505 [details] [review] wizard: Return to SOURCE page when erroneously changed into PREPARATION When an erroneous URL (such as empty or unsupported one) is entered in the source view, the page is wrongfully changed in PREPARATION thus making the back button sensitive. In order to fix this, change page back to SOURCE if validation of URL fails.
Review of attachment 324505 [details] [review]: * Seems you ignored what I said in my previous review of what needs to be done here. You can always disagree but cause I can be completely wrong but you have to first comment and show how I'm wrong or convince me how your approach is better. :) Looking into the code, I think the best approach is probably to update the back_button.sensitive after a successful page change. * Shortlog is too long. It's ideally supposed to be < 50 chars and maximum 74 chars. * You forgot to mark the previous version of the patch, obsolete. -e option of `git-bz attach` makes it very easy to do that while uploading patches.
(In reply to Zeeshan Ali (Khattak) from comment #6) > The correction solution would either be to not set sensitivity of 'Back' > button before URL validation on PREPARATION page or just ensure that we > disable the 'Back' button on arriving to SOURCE page. I'll prefer the latter > approach. I think the problem is that the page is changed into PREPARATION even if the URL is broken and it doesn't go back to SOURCE. I think the back_button sensitivity is logically set correct as (page != WizardPage.SOURCE). Am I wrong?
(In reply to Radu Stochitoiu from comment #9) > (In reply to Zeeshan Ali (Khattak) from comment #6) > > The correction solution would either be to not set sensitivity of 'Back' > > button before URL validation on PREPARATION page or just ensure that we > > disable the 'Back' button on arriving to SOURCE page. I'll prefer the latter > > approach. > > I think the problem is that the page is changed into PREPARATION even if the > URL is broken and it doesn't go back to SOURCE. I think the back_button > sensitivity is logically set correct as (page != WizardPage.SOURCE). > Am I wrong? Yes. :) If you read the 'page' property setter more closely, you'll find that: 1. back button sensitivity is updated based on the new value (that is not yet set to be the page value yet) at the beginning and hence will happen even if page setting was unsuccessful. 2. Page is not actually being set since the setter returns prematurely, before property is actually set much later: _page = value; which is wizard.vala#134 in git master.
Created attachment 324891 [details] [review] wizard: Modify back button sensitivity The back button sensitivity is based on the new value that may not become the page value due to some errors or early returns. In order to fix this, make the back button sensitive to the value of the modified page.
Review of attachment 324891 [details] [review]: Shortlog is not only vague but also sounds wrong. You are not modifying button sensitivity but rather moving it to later, when the page change has been successful. ::: src/wizard.vala @@ +130,3 @@ return; + back_button.sensitive = (value != WizardPage.SOURCE); Since the point is to only update sensitivity after the page change, let's actually do this after updating _page below. I know it doesn't make any difference currently but if we keep it this way, it won't be clear to developers that this needs to be done after page change and they could easily add a failable step between these two lines and we have this bug all over again.
Created attachment 325994 [details] [review] wizard: Make back button sensitive after page change The back button sensitivity is based on a temporary value of the Wizard page. In order to fix this, move the back button sensitivity change after the page change is confirmed.
Review of attachment 325994 [details] [review]: Seems while being a away for a bit, you forgot about rules for shortlong and description lines. :) Summary should be 50 chars ideally and description lines never beyond 74 characters. "The back button sensitivity is based on a temporary value of the Wizard page." That makes it sound like a generic fact rather than the problem description. How about "Back button sensitivity is currently updated before page change is successful, making the button sensitive on the 'source' page for after an unsuccessful switch to 'preparation' page." "In order to fix this," part is pretty redundant. If you feel it's not, just "Fix this by" would be good enough, no need to sound super official. :)
Created attachment 327896 [details] [review] wizard: back bttn gets sensitive after page change Back button sensitivity is currently updated before page change is successful, making the button sensitive on the 'source' page after an unsuccessful switch to 'preparation' page. Fix this by setting the back button sensitive after the page change is successful.
Created attachment 327897 [details] [review] wizard: back button sensitive after page change Back button sensitivity is currently updated before page change is successful, making the button sensitive on the 'source' page after an unsuccessful switch to 'preparation' page. Fix this by setting the back button sensitivity after the page change is successful.
Review of attachment 327897 [details] [review]: ::: src/wizard.vala @@ -6,3 @@ SETUP, REVIEW, - Something went wrong cause I'm sure this one line of change, won't do any good to anyone. :) Had the same issues with patches submitted by Visarion before and it turned out that he was (unknowingly) modifying the patches outside of git afterwards. Please ensure you do not do that. Just use `git-bz attach -e` command to upload patches. Otherwise, generate the patches through git-fromat-patch and upload them as is, without even opening them in any editor that might be changing things in the patch. You can be 200% sure that git-format-patch does it's job correctly and if you need to modify or review the patch before uploading, please do so using git (or tools around it, like tig).
Created attachment 328452 [details] [review] wizard: back button sensitive after page change Back button sensitivity is currently updated before page change is successful, making the button sensitive on the 'source' page after an unsuccessful switch to 'preparation' page. Fix this by setting the back button sensitivity after the page change is successful.