GNOME Bugzilla – Bug 762015
roomList: Default arrow icon and change-sensitive apply button
Last modified: 2016-02-16 13:19:12 UTC
There should be an icon on the roomlist header suggesting there is a menu. In addition, on the properties dialog under the roomlist header, the apply button should only be sensitive when a change was made. To fix, these errors the default icon on the roomlist header is now a down-arrow and all the fields on the properties dialog are watched to enable the apply button when a change was made.
Created attachment 321091 [details] [review] roomList: Default arrow icon and change-sensitive apply button
Review of attachment 321091 [details] [review]: Please split this into two commits - adding the icon to the room list header and changing the sensitivity of the apply button are completely unrelated changes. ::: data/resources/room-list-header.ui @@ +60,3 @@ </child> <child> + <object class="GtkArrow"> GtkArrow is deprecated - just use an image with an icon-name of 'pan-down-symbolic' @@ +71,1 @@ <object class="GtkBox"> This isn't used anymore (the "none" page), so please remove it ::: src/connections.js @@ +142,3 @@ + + _paramsChanged: function() { + let savedParams = this._account.dup_parameters_vardict().deep_unpack(); ConnectionDetails are used both for changing account properties and for account creation (in the joinDialog) - in the latter case, account is null, so this will throw an error. Given that those values shouldn't change between calls, how about saving those as properties? You can set them to empty string in _init(), then fill in the real values in _populateFromAccount() @@ +146,3 @@ + savedParams[p] = savedParams[p].deep_unpack(); + + let savedName = this._account.display_name; This isn't quite right - the 'name' field is optional, but we always set a display name anyway (falling back to 'server'). So the saved name could be 'irc.gnome.org' and the unchanged text in the nameEntry ''. @@ +147,3 @@ + + let savedName = this._account.display_name; + let savedServer = savedParams.server || ''; That's not quite correct either. The server field is used to enter both server and port, but we only include the port if it is non-standard. So again, the saved server could be 'irc.gnome.org', but the unchanged text in the serverEntry 'irc.gnome.org:6665'.
Created attachment 321288 [details] [review] roomList: Adding default arrow down icon There should be an icon on the roomlist header suggesting there is a menu. To fix this error the default icon on the roomlist header is now a down-arrow.
Review of attachment 321288 [details] [review]: The icon part looks good to me now, but the patch still has the button sensitivity bits (with the same issues pointed out before).
Created attachment 321300 [details] [review] roomList: Adding default arrow down icon There should be an icon on the roomlist header suggesting there is a menu. To fix this error the default icon on the roomlist header is now a down-arrow.
Created attachment 321304 [details] [review] roomList: Adding default arrow down icon There should be an icon on the roomlist header suggesting there is a menu. To fix this error the default icon on the roomlist header is now a down-arrow.
Created attachment 321306 [details] [review] roomList: Adding default arrow down icon There should be an icon on the roomlist header suggesting there is a menu. To fix this error the default icon on the roomlist header is now a down-arrow.
Created attachment 321310 [details] [review] editConnection: Adding change-sensitive apply button On the properties dialog under the roomlist header, the apply button should only be sensitive when a change was made. To fix this error, all the fields on the properties dialog are watched to enable the apply button when a change was made.
Created attachment 321312 [details] [review] connectionDetails: Adding change-sensitive apply button On the properties dialog under the roomlist header, the apply button should only be sensitive when a change was made. To fix this error, all the fields on the properties dialog are watched to enable the apply button when a change was made.
Review of attachment 321306 [details] [review]: Code looks good, commit message could be improved a bit: roomList: Add default arrow icon to headers Since commit 6ef2e5737, the header popovers are used for connection management, so the headers are now always sensitive. However that's not obvious to users, so add an arrow icon to indicate that there's a menu.
Created attachment 321322 [details] [review] roomList: Add default arrow icon to headers Since commit 6ef2e5737, the header popovers are used for connection management, so the headers are now always sensitive. However that's not obvious to users, so add an arrow icon to indicate that there's a menu.
Review of attachment 321312 [details] [review]: There's a bug in _paramsChanged(), otherwise style nits. ::: src/connections.js @@ +60,3 @@ + this._savedServer = ''; + this._savedNick = ''; + this._savedRealname = ''; That's not quite correct - the value for nick is not the empty string, but the user name. How about moving this into reset(): this._savedName = ''; this._savedServer = ''; this._savedNick = GLib.get_user_name(); this._savedRealname = ''; this._nameEntry.text = this._savedName; this._serverEntry.text = this._savedServer; this._nickEntry.text = this._savedNick; this._realnameEntry.text = this._savedRealname; By setting the entries from the saved values, you make sure that the two always match up. @@ +144,3 @@ + this._savedServer = server; + this._savedNick = nick; + this._savedRealname = realname; Similar to the suggestion for reset(), I'd replace the local variables with the properties, then set the entries at the end: this._savedServer = params.server || ''; let port = params.port || 6667; this._savedNick = params.account || ''; this._savedRealname = params.fullname || ''; if (port != 6667) this._savedServer += ':%d'.format(port); if (this._savedServer != account.display_name) this._savedName = account.display_name; this._serverEntry.text = this._savedServer; this._nickEntry.text = this._savedNick; this._realnameEntry.text = this._savedRealname; this._nameEntry.text = this._savedName; @@ +149,3 @@ get can_confirm() { return this._serverEntry.get_text_length() > 0 && + this._nickEntry.get_text_length() > 0 && this._paramsChanged(); Style nit: When a condition is split over several lines, operands of the same precedence should be aligned: return serverEntryLength > 0 && nickEntryLenght > 0 && this._paramsChanged(); Also not sure it's worth splitting out paramsChanged into a function - (foo && bar && (baz || quz)) is a bit ugly, but you could use a local variable: let paramsChanged = nameText != savedName || serverText != savedServer || ... @@ +153,3 @@ + + _paramsChanged: function() { + return this._nameEntry.text != this._savedName || this._serverEntry.text != this._savedServer Missing || at the end - to avoid those mistakes, I prefer writing large conditions as return condition1 || condition2 || condition3 || condition4;
Review of attachment 321322 [details] [review]: LGTM
Created attachment 321325 [details] [review] connectionDetails: Adding change-sensitive apply button On the properties dialog under the roomlist header, the apply button should only be sensitive when a change was made. To fix this error, all the fields on the properties dialog are watched to enable the apply button when a change was made.
Comment on attachment 321322 [details] [review] roomList: Add default arrow icon to headers Attachment 321322 [details] pushed as 1e9aee7 - roomList: Add default arrow icon to headers
Review of attachment 321325 [details] [review]: Code looks good now, thanks! For the commit message: - subject should use active form ("Add" instead of "Adding" - though the button isn't actually added, you just change when it's sensitive; maybe "connectionDetails: Consider changes for "Apply" sensitivity"?) - in the body it would be good to mention the reason for the change (also a nit: it's not really an error fix - it makes sense to make the button insensitive when there are no changes to save, but nothing breaks when the existing settings are saved again) ::: src/connections.js @@ +104,2 @@ reset: function() { + Odd newline
Created attachment 321332 [details] [review] connectionDetails: Consider changes for "Apply" sensitivity On the properties dialog under the roomlist header, make sense if the apply button only be sensitive only when a change was made. This usability enhancement makes all the fields on the properties dialog watched for when a change was made the apply button can be enabled.
Attachment 321332 [details] pushed as 978e713 - connectionDetails: Consider changes for "Apply" sensitivity Thanks! I fixed up some minor grammar mistakes in the commit message and pushed.