GNOME Bugzilla – Bug 761290
roomList: shortcut to switch to channels with unread messages
Last modified: 2016-02-17 21:49:37 UTC
It would be convenient to have a way of quickly going through your unread messages. In slack they do the following [1]: Alt+Up: Previous room Alt+Down: Next room Alt+Shift+Up: Previous room with unread messages Alt+Shift+Down: Next room with unread messages We already have Alt+Up and Alt+Down for previous/next room, so perhaps adding Alt+Shift+Up / Alt+Shift+Down could be useful. Since we also have Ctrl+PgDown and Ctrl+PgUp we could similarly have Ctrl+Shift+PgDown and Ctrl+Shift+PgUp ? [1]: https://get.slack.help/hc/en-us/articles/201374536-Slack-keyboard-shortcuts Could also be interesting to think about having an "activities overview" but that's beyond this enhancement request..
Created attachment 321067 [details] [review] roomList: Switch to channels with unread messages Added the functionality to switch to channels that have pending/unread messages. The Alt+Shift+Down and Ctrl+Shift+PageDown combinations can be used to switch to the next room that has pending/unread messages, while the Alt+Shift+Up and Ctrl+Shift+PageUp combinations can be used to switch to the previous room with pending/unread messages.
Review of attachment 321067 [details] [review]: ::: src/roomList.js @@ +360,3 @@ + action.connect('activate', Lang.bind(this, + function() { + this._nextPending(); Please avoid the code duplication, in particular as the function will need to be trickier than what you have now. @@ +477,3 @@ + + _nextPending: function(){ + let current = this.widget.get_selected_row(); Please make sure you are on master - you are definitively missing this[0] from a week ago (the short summary is: replace 'this.widget' with 'this') [0] https://git.gnome.org/browse/polari/commit/?id=40ce0dfde4d46 @@ +484,3 @@ + let count = this._roomManager.roomCount; + + for(let i = current.get_index(); i < count; i++){ This is not correct. We now use the room list headers for account management, so we include all accounts regardless of whether there are joined rooms or not. This is implemented by inserting a placeholder row for every account, so the number of rows in the list is *not* the number of rooms. There are _rowToRoomIndex() and _roomToRowIndex() functions to translate between row- and room indices and a _getRoomRowAtIndex() helper function to get the row corresponding to a room index (i.e. _getRoomRowAtIndex(0) for the first room row). You might want to take a look at _moveSelection() for inspiration. It might even be possible to extend _moveSelection() to _moveSelectionFull() or something: _moveSelectionFull(direction, testFunc) which moves the selection in the specified direction until testFunc() returns true. _moveSelection() could then be written as: _moveSelection(direction) { return this._moveSelectionFull(direction, () => { return true; }); } @@ +490,3 @@ + } + + if(!row.get_style_context().has_class('inactive')){ I don't like this too much. IMHO it is an implementation detail of RoomRow that rooms with activity are styled differently from rooms without activity, and how that's done. It could just as well be (row.get_style_context().has_class('has-activity')) without making any less sense. That's why I suggested a read-only property in RoomRow which indicates the state without exposing how that's represented internally: get hasPending() { return this.get_style_context().has_class('inactive'); // or: return this._room.channel.dup_pending_messages().length > 0; } The in the RoomList, you can use it like any other property: if (!row.hasPending) { this.select_row(); } (Note that 'hasPending' is used as a property and not as a function - see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get for details)
Review of attachment 321067 [details] [review]: ::: src/application.js @@ +113,3 @@ + accels: ['<Alt><Shift>Down', '<Primary><Shift>Page_Down']}, + { name: 'previous-pending-room', + accels: ['<Alt><Shift>Up', '<Primary><Shift>Page_Up']} Oh, and something else: Those shortcuts should be added to the shortcuts window (data/resources/help-overlay.ui) - those are really useful, we should tell users about them!
Created attachment 321247 [details] [review] roomList: Switch to channels with unread/pending messages Added the functionality to switch to channels that have pending/unread messages. The Alt+Shift+Down and Ctrl+Shift+PageDown combinations can be used to switch to the next room that has pending/unread messages, while the Alt+Shift+Up and Ctrl+Shift+PageUp combinations can be used to switch to the previous room with pending/unread messages. Also, the keyboard shortcuts have been updated.
Review of attachment 321247 [details] [review]: ::: data/resources/help-overlay.ui @@ +76,3 @@ <object class="GtkShortcutsShortcut"> <property name="visible">True</property> + <property name="title" translatable="yes" context="shortcut window">Next Room with Pending Messages</property> Not sure "pending messages" is the best term to expose to users. Maybe "unread" is better? @@ +77,3 @@ <property name="visible">True</property> + <property name="title" translatable="yes" context="shortcut window">Next Room with Pending Messages</property> + <property name="accelerator"><Primary><Shift>Page_Down <Alt><Shift>Down</property> Some actions have alternative shortcuts that are used by other IRC clients to help users transition, but we don't expose those in the shortcuts window. See the "Show userlist" shortcuts and "Next/Previous Room". ::: src/roomList.js @@ +63,3 @@ + get hasPending() { + return !this.get_style_context().has_class('inactive'); No tabs! @@ +404,3 @@ + function() { + this._moveSelectionFull(Gtk.DirectionType.DOWN, + this._isPendingRow); Dto. @@ +410,3 @@ + function() { + this._moveSelectionFull(Gtk.DirectionType.UP, + this._isPendingRow); Dto. @@ +444,2 @@ _moveSelection: function(direction) { + this._moveSelectionFull(direction, (index) => {return true;}); Style nit: space after opening braces and before closing ones @@ +447,3 @@ + + _moveSelectionFull: function(direction, testFunction){ + let current = this.get_selected_row(); Tabs again @@ +454,3 @@ let index = this._rowToRoomIndex(current.get_index()); + + do{ Missing space @@ +458,3 @@ + current = this._getRoomRowAtIndex(index); + }while(!testFunction(current) && index < this._roomManager.roomCount + && index >= 0); - gtk_list_box_get_row_at_index() returns NULL when the index is out of bounds, so you don't need to add the checks again (also note that if that wasn't the case, the checks would miss the first iteration) - testFunc() should be to check if a row fulfills a certain condition, we shouldn't expect it to test for the existence of the row parameter (note how your callback in _moveSelection() gets that wrong) - space after brace @@ +460,3 @@ + && index >= 0); + + let row = current; Nit: The current code uses 'current' to refer to the currently selected row and 'row' for the row to be selected. Technically you don't need two variables, but if you keep them, they should make sense :-) @@ +462,3 @@ + let row = current; + + if(row){ Style nit: no braces around one-liners (otherwise: missing space before opening brace), space after if @@ +494,3 @@ }, + _isPendingRow: function(row){ Space before brace, though I'm not sure it's worth splitting that out - if you move the 'row' check into _moveSelectionFull(), the inline callback becomes '(row) => { return row.hasPending; }', which is already fairly concise ...
Created attachment 321375 [details] [review] roomList: Switch to channels with unread/pending messages Added the functionality to switch to channels that have pending/unread messages. The Alt+Shift+Down and Ctrl+Shift+PageDown combinations can be used to switch to the next room that has pending/unread messages, while the Alt+Shift+Up and Ctrl+Shift+PageUp combinations can be used to switch to the previous room with pending/unread messages. Also, the keyboard shortcuts have been updated.
Review of attachment 321375 [details] [review]: I'm not quite sure why, but with the two new shortcuts, the groups in the shortcuts window are now lined up vertically instead of horizontally as before. It's probably the length of the shortcut names, but you can force the previous layout by setting the section's :max-height property to 14 - the designers agree that the horizontal alignment looks better, so let's squeeze that into the patch :-) Other than that there's just some style nits: ::: src/roomList.js @@ +444,2 @@ _moveSelection: function(direction) { + this._moveSelectionFull(direction, (index) => { return true; }); Nit: The parameter passed to the callback is a row, not an index - but as it's unused anyway, you can just leave it out completely ( () => { return true; } ) @@ +459,3 @@ + index += inc; + row = this._getRoomRowAtIndex(index); + } while(row && !testFunction(row)); Space after 'while' ... @@ +461,3 @@ + } while(row && !testFunction(row)); + + if(row) ... and after if!
Created attachment 321550 [details] [review] roomList: Switch to channels with unread/pending messages Added the functionality to switch to channels that have pending/unread messages. The Alt+Shift+Down and Ctrl+Shift+PageDown combinations can be used to switch to the next room that has pending/unread messages, while the Alt+Shift+Up and Ctrl+Shift+PageUp combinations can be used to switch to the previous room with pending/unread messages. Also, the keyboard shortcuts have been updated.
Looks good, thanks! Attachment 321550 [details] pushed as 63db114 - roomList: Switch to channels with unread/pending messages