GNOME Bugzilla – Bug 593840
Add errorhandling to the runDialog
Last modified: 2009-09-08 18:36:12 UTC
The attached patch passes the errors from running commands to the user instead of just logging them using log().
Created attachment 142265 [details] [review] The patch
Created attachment 142266 [details] [review] The correct patch Sorry attached the wrong patch, this is the correct one.
Created attachment 142365 [details] [review] Better patch Reworked patch, resizes the box instead of adding a new one.
Generally looks good, some small details, and I'm not very happy about continuing to do manual positioning (runDialog was written before we had Big.Box in the tree to do real layout.) More comments about that below. - let box = new Big.Box({ background_color: BOX_BACKGROUND_COLOR, + this._box = new Big.Box({ background_color: BOX_BACKGROUND_COLOR, corner_radius: 4, reactive: false, width: BOX_WIDTH, height: BOX_HEIGHT }); Should reindent the properties and the closing brace to keep everything lined up. this._entry.connect('activate', Lang.bind(this, function (o, e) { this._run(o.get_text()); - this.close(); - return false; + if(!this._commandError) { + this.close(); + return false; + } + return true; })); the activate signal actually doesn't have a return value. So, should be: if (!this._commandError) return; Note space after 'if'. + this._errorMessage = new Clutter.Text({ color: BOX_TEXT_COLOR, + font_name: '18px Sans Bold', + editable: false, + activatable: false, + singleLineMode: false, + text: '', + width: BOX_WIDTH - 12, + height: BOX_HEIGHT - 12 }); Parameters all need to be lined up. default fields - editable, activatable, singleLineMode, text don't need to be set. Manual sizing here is really yuck and long errors overflow (probably worse in non-English languages). Suggest getting rid of BOX_HEIGHT and fixed positioning entirely, and having: ------------------------------------------------------------ | Box1, horz | Box2, vert | | | | | | | | | | | +----------------------+ | | Box3 |Please enter a command| | | vert |<Entry> | | | +----------------------+ | | | | | Box4H | | | icon |err | | | |Box5V | | | | +----------------------| | | | | | | | | | | | | | ------------------------------------------------------------ Box1 - size of the screen, centers dialog horizontally Box2 - introduced to center the dialog vertically Box3 is the dialog itself, and contains three children Box4 is a horizontal box with the icon Box5 centers the errorr icon vertically Not as complex as it sounds or as drawing the ascii art was. You may still want to set a fixed with on Box so that the entry isn't too short and so that err wraps rather than mkaing the dialog hugely wide. Box4 should be shown/hidden depending on whether you have an error or not. new RegExp(/.+\((.+)\)/); Isn't new Regexp() redundant with // ? I'd give an example in a comment of what the error was before you parsed out the part between the (). + log(errorStr); Don't really see a reason to log this in addition.
(In reply to comment #4) > Generally looks good, some small details, and I'm not very happy about > continuing to do manual positioning (runDialog was written before we had > Big.Box in the tree to do real layout.) More comments about that below. > > - let box = new Big.Box({ background_color: BOX_BACKGROUND_COLOR, > + this._box = new Big.Box({ background_color: BOX_BACKGROUND_COLOR, > corner_radius: 4, > reactive: false, > width: BOX_WIDTH, > height: BOX_HEIGHT > }); > > Should reindent the properties and the closing brace to keep everything lined > up. OK. > > this._entry.connect('activate', Lang.bind(this, function (o, e) { > this._run(o.get_text()); > - this.close(); > - return false; > + if(!this._commandError) { > + this.close(); > + return false; > + } > + return true; > })); > > the activate signal actually doesn't have a return value. So, should be: > > if (!this._commandError) > return; OK, got rid of the the return value. > Note space after 'if'. Fixed. > + this._errorMessage = new Clutter.Text({ color: BOX_TEXT_COLOR, > + font_name: '18px Sans Bold', > + editable: false, > + activatable: false, > + singleLineMode: false, > + text: '', > + width: BOX_WIDTH - 12, > + height: BOX_HEIGHT - 12 }); > > Parameters all need to be lined up. default fields - editable, activatable, > singleLineMode, text don't need to be set. Yeah removed them. > Manual sizing here is really yuck and long errors overflow (probably worse in > non-English languages). Suggest getting rid of BOX_HEIGHT and fixed positioning > entirely, and having: > > ------------------------------------------------------------ > | Box1, horz | Box2, vert | | > | | | | > | | | | > | +----------------------+ | > | Box3 |Please enter a command| | > | vert |<Entry> | | > | +----------------------+ | > | | | | Box4H | > | | icon |err | | > | |Box5V | | | > | +----------------------| | > | | | | > | | | | > | | | | > ------------------------------------------------------------ > > Box1 - size of the screen, centers dialog horizontally > Box2 - introduced to center the dialog vertically > Box3 is the dialog itself, and contains three children > Box4 is a horizontal box with the icon > Box5 centers the errorr icon vertically > > Not as complex as it sounds or as drawing the ascii art was. OK, done that. > You may still want to set a fixed with on Box so that the entry isn't too short > and so that err wraps rather than mkaing the dialog hugely wide. Well I tried that but with a fixed with the error does not wrap but overflow the box ... any idea why? > Box4 should be shown/hidden depending on whether you have an error or not. > new RegExp(/.+\((.+)\)/); > > Isn't new Regexp() redundant with // ? Yeah it is, but with "new Regexp()" it is more obvious, but ok removed that. > I'd give an example in a comment of what the error was before you parsed out > the part between the (). OK, added. > + log(errorStr); > > Don't really see a reason to log this in addition. Yeah removed that.
Created attachment 142527 [details] [review] Errorhandling patch with new layout model (no magic numbers)
Created attachment 142558 [details] [review] Fxied patch OK, I did not set line_wrap to true before that's why wrapping was not working, the attached patch has this fixed so we now have a fixed with and the error message wraps nicely.
Review of attachment 142558 [details] [review]: Almost perfect - just a few indentation tweaks and a UI tweak or two and it will be good to commit. ::: js/ui/runDialog.js @@ -70,1 +72,3 @@ - this._group.add_actor(boxGroup); + width: global.screen_width, + height: global.screen_height + }); The parameters not on the first line and the closing } are not quite aligned right. @@ -75,3 +79,3 @@ - width: BOX_WIDTH, - height: BOX_HEIGHT - }); + y_align: Big.BoxAlignment.CENTER + }); + More alignment problems, this is a consistent (if minor) problem through the patch @@ -93,3 +108,3 @@ - height: BOX_HEIGHT - 12 }); - // TODO: Implement relative positioning using Tidy. - this._entry.set_position(6, 30); + dialogBox.append(this._entry, Big.BoxPackFlags.EXPAND); + + this._errorBox = new Big.Box({ orientation: Big.BoxOrientation.HORIZONTAL }); It looks better if you add a little padding above the error box - padding_top: DIALOG_PADDING (6px) looks about right to me. @@ -96,0 +119,3 @@ + }); + + this._errorBox.append(iconBox, Big.BoxPackFlags.EXPAND); I don't think you want to expand the iconBox - it looks a bit funny when the icon gets more padding around it as there is more available space - using '0' for flags here would be more appropriate. @@ -100,2 +139,3 @@ - this.close(); - return false; + if(!this._commandError) { + this.close(); + } Missing space after 'if', {} not needed
Created attachment 142686 [details] [review] New patch Fixed alignment and style problems, added padding to the error dialog and made the iconbox not expanding.
Review of attachment 142686 [details] [review]: Still a few small alignment problems, marked below, but I can just fix them up and push your patch with those fixes. ::: js/ui/runDialog.js @@ -69,2 +71,3 @@ - (global.screen_height - BOX_HEIGHT) / 2); - this._group.add_actor(boxGroup); + y_align: Big.BoxAlignment.CENTER, + width: global.screen_width, + height: global.screen_height }); Hmm, we are very inconsistent on whether the }) goes on the next line or not. I thought we usually did the other, but looking through the code it seems to be more common this way. And it does make getting the alignment right easier :-) @@ -86,2 +98,2 @@ this._entry = new Clutter.Text({ color: BOX_TEXT_COLOR, - font_name: '20px Sans Bold', + font_name: '20px Sans Bold', One position off @@ -93,3 +105,3 @@ - height: BOX_HEIGHT - 12 }); - // TODO: Implement relative positioning using Tidy. - this._entry.set_position(6, 30); + + this._errorBox = new Big.Box({ orientation: Big.BoxOrientation.HORIZONTAL, + padding_top: DIALOG_PADDING }); second line is off by one position @@ -96,0 +123,3 @@ + + this._errorMessage = new Clutter.Text({ color: BOX_TEXT_COLOR, + font_name: '18px Sans Bold', One posiiton off
(In reply to comment #10) > Review of attachment 142686 [details] [review]: > > Still a few small alignment problems, marked below, but I can just fix them up > and push your patch with those fixes. > > ::: js/ui/runDialog.js > @@ -69,2 +71,3 @@ > - (global.screen_height - BOX_HEIGHT) / 2); > - this._group.add_actor(boxGroup); > + y_align: Big.BoxAlignment.CENTER, > + width: global.screen_width, > + height: global.screen_height }); > > Hmm, we are very inconsistent on whether the }) goes on the next line or not. I > thought we usually did the other, but looking through the code it seems to be > more common this way. And it does make getting the alignment right easier :-) Yeah I compared it with the rest of the code and decided to stick to that. > @@ -86,2 +98,2 @@ > this._entry = new Clutter.Text({ color: BOX_TEXT_COLOR, > - font_name: '20px Sans Bold', > + font_name: '20px Sans Bold', > > One position off > > @@ -93,3 +105,3 @@ > - height: BOX_HEIGHT - 12 }); > - // TODO: Implement relative positioning using Tidy. > - this._entry.set_position(6, 30); > + > + this._errorBox = new Big.Box({ orientation: > Big.BoxOrientation.HORIZONTAL, > + padding_top: DIALOG_PADDING }); > > second line is off by one position > > @@ -96,0 +123,3 @@ > + > + this._errorMessage = new Clutter.Text({ color: BOX_TEXT_COLOR, > + font_name: '18px Sans Bold', > > One posiiton off Fixed those, and removed trailing whitespace, I hope did not miss anything.
Created attachment 142715 [details] [review] New Patch
Patch pushed with a couple of small fixes discussed on IRC.