GNOME Bugzilla – Bug 686348
Check URI before enabling 'Continue'
Last modified: 2016-03-31 13:59:31 UTC
The 'Continue' button is enabled as soon as you enter a character on the URI text entry.
I don't see how to verify it though. I mean you can check that its a valid uri, but that would be true even for useless uris like "http://a" or whatever.
Created attachment 288843 [details] [review] Changes to enable Continue button correctly Not foolproof yet, submitting for review. Will be adding screen shots explaining different scenarios.
Created attachment 288844 [details] docx/odt files with screenshots for my previous patch. Screenshots show at what places the Continue button is enabled. They also show where it's failing.
Review of attachment 288843 [details] [review]: First off, the shortlog should mention what the change is rather than its effect (which you describe in description, along with details of the change). Secondly, you are not far off (considering its your first patches) since you provided some details in the commit log. Instead of saying its not foolproof, it would be more helpful to describe what you think is missing so others can more easily help you out. Last but not the least, try not to bundle all changes in one patch (even if its to solve one bug). Having the patches nicely divided into logical changes, will also make it easy for you to come up with good commit messages. You might want to go through existing bugs to see whats expected of Boxes patch contributors: https://bugzilla.gnome.org/show_bug.cgi?id=710306 and its a particularly good idea to go through checklist here: https://wiki.gnome.org/Git/CommitMessages P.S. Don't feel bad about making mistakes, just try your best not to keep repeating them too many times. :) ::: src/wizard.vala @@ +173,2 @@ case Boxes.SourcePage.URL: + //next_button.sensitive = wizard_source.uri.length != 0; why this?
(In reply to comment #3) > Created an attachment (id=288844) [details] > docx/odt files with screenshots for my previous patch. > > Screenshots show at what places the Continue button is enabled. They also show > where it's failing. In future, please inline such descriptions. For pictures, you can upload them first and then refer to them as attachment#XYZ so bz turns it into links for you.
(In reply to comment #5) > (In reply to comment #3) > > Created an attachment (id=288844) [details] [details] > > docx/odt files with screenshots for my previous patch. > > > > Screenshots show at what places the Continue button is enabled. They also show > > where it's failing. > > In future, please inline such descriptions. For pictures, you can upload them > first and then refer to them as attachment#XYZ so bz turns it into links for > you. Forgot to mention the rationale. :) This is so that people can make inline replies to everything easily.
Created attachment 289899 [details] [review] wizard-source: Update url page to diplay messages for (in)correct URI Adding a new function update_url_page_for_error to display messages. Correct markup with title, icon_name can be added once functionality is accepted to be correct, using update_url_page itself.
Created attachment 289900 [details] [review] wizard: add timers to be called when url entry is changed reset_timeout (): Timeout after 1 second to call wizard_source_update_next. If reset url entry changes before the timeout happens, reset the timeout. timed_wizard_source_update_next (): if Continue button is already activated, terminate the timer (till next url entry change occurs)
Created attachment 289902 [details] [review] wizard: class fields added to support reset_timeout
Created attachment 289904 [details] [review] wizard: Change the sensitivity of Continue button for various cases Update url page with relavent messages, when sensitivity of Continue button is changed. Replaced throw phrases with calls to update url page. Correctness of enabling continue button at places where changes have been made, has to be verified. Adding screenshots
Created attachment 289906 [details] Single box creation using location URI Continue is active because boxes seems to assume path as default path as current. Location is the URI string entered. wizard.vala : var file = location.contains ("://")? File.new_for_uri (location) : File.new_for_path (location); I think this should be changed.
Created attachment 289907 [details] Screenshot showing local (file://) URI to either ISO or disk image Continue is enabled when file:// uri is entered.
Created attachment 289908 [details] Screenshot for HTTP(s) URI When HTTP(S) URL to media, Continue is not enabled, as download is required.
Created attachment 289909 [details] Screenshot for Spice URI Continue button enabled for remote login URI: SPICE
Review of attachment 289899 [details] [review]: Short log is a bit long (ideal is 50 chars) and incorrect. You are only adding the function to display *error* messages, not actually displaying in this patch. How about simply "wizard-source: Add update_url_page_for_error()"? > ", icon_name can be added once functionality is accepted to be correct," * Please start a new sentence with a '.' and capitalization. * Can be added or will be added? * Its not very clear what "icon-name" is and what it will do. ::: src/wizard-source.vala @@ +216,3 @@ } + + public void update_url_page_for_error(string errText) { * How is this different than update_url_page? It should at least use some special markup to differentiate error test from normal text to justify its existance. * Coding style requires variable and parameter names in all short letters with '_' rather than CamelCase.
Review of attachment 289900 [details] [review]: I believe this patch is in the right direction. Some comments still though: * Short log could be shorter and it doesn't tell about the important part of the patch. Timeout is more a detail, the important bit is checking of URL. How about "wizard: Keep checking URL" (this would be more correct assuming you act on my inline comments). ::: src/wizard.vala @@ +194,3 @@ } + private void reset_timeout () { * We prefer descriptive names. How about adding adding "url_entry_" before "timeout" in this function and timeout_id field? * Isn't this more a 'setup' function rather than 'reset'? I'd name it 'setup_url_entry_tiemout'. @@ +195,3 @@ + private void reset_timeout () { + if(timeout_id != 0){ * timeout_id field/property is never declared before use. * I'd rather take this field bein non-zero as indication that timeout is already setup and therefore simply to return if thats the case. * No need for "{" for single statement blocks. @@ +201,3 @@ + } + + private bool timed_wizard_source_update_next () { I'd name it "on_url_entry_timeout". @@ +202,3 @@ + + private bool timed_wizard_source_update_next () { + wizard_source_update_next(); coding style: space before '('. @@ +203,3 @@ + private bool timed_wizard_source_update_next () { + wizard_source_update_next(); + if(next_button.sensitive == true) * coding style: space before '('. * Check the return value of wizard_source_update_next() instead here. If it doesn't return one already, please change that first in a parent patch to this one. @@ +204,3 @@ + wizard_source_update_next(); + if(next_button.sensitive == true) + return false; // false terminates timer and that means timeout_id is not set to 0 even though there is no timeout anymore. @@ +211,2 @@ construct { + timeout_id = 0; Vala takes care of this for you. @@ +214,3 @@ wizard_source.notify["page"].connect(wizard_source_update_next); wizard_source.notify["selected"].connect(wizard_source_update_next); + wizard_source.url_entry.changed.connect (reset_timeout); Boxes could also change the text in URL entry so i don't think change of text alone is a good anchor for setup of timeout. Also timeout is not removed if user hits 'Cancel' or 'back' in the wizard before entering valid URL. I suggest you hook the setup and removal of this signal handler to entry gaining and loosing focus. There is "focus-in-event” and "focus-out-event” signals on GtkWidget for this.
Review of attachment 289902 [details] [review]: I don't think this belongs in a separate patch as its not really an independent change, especially since fields are private. ::: src/wizard.vala @@ +57,3 @@ private Cancellable? review_cancellable; private bool skip_review_for_live; + private uint timeout_id; As pointed out in another review: too generic name. @@ +59,3 @@ + private uint timeout_id; + + //being used at many places Comment isn't helpful. @@ +61,3 @@ + //being used at many places + private string text = _("Please enter desktop or collection URI"); + private string icon = "preferences-desktop-remote-desktop"; * too generic names. * The 'text' string is already being used in wizard so this patch should update the usage to use this field. * Please use 'const' for constants and declare them above all other fields/properties.
Review of attachment 289904 [details] [review]: The very generic shortlog referring to 'various cases' is a good indication that this is multiple changes put in the same commit so you gotta divide this further, one patch per case. Also shortlog is way too long. You can omit 'the' for example to keep it shorter. :) The description also mentioned different changes so thats another indication that this patch should be divided further. "Correctness of enabling continue button at places where changes have been made, has to be verified." * As I said before, write commit logs assuming that patch could be pushed as is, so sentences like these that are meant only/mainly for reviewer's eyes should not be part of the log. You can inform others of untested status of your patch by adding a comment in review link. * Please add a blank line betweeen paragraphs. I'll review the changes themselves once you have divided the patch.
commit: e4080c518d83c251322548adba5d4164a5e3ed55 Date: Mon Feb 16 17:29:58 2015 +0000 wizard: Only enable 'Continue' for valid URIs We currently enable the 'Continue' button as soon as URL entry in non-empty. Lets not enable it until we have a validated URI in the entry. commit: c1056e122c16b0f9795d63f2e8bc984d7b67213c Date: Mon Feb 16 17:27:52 2015 +0000 wizard: More thorough validation of local URIs Check if the file is of a supported extension and has correct file type.