GNOME Bugzilla – Bug 760853
chatView: Show a contextual popover when clicking a nickname
Last modified: 2016-10-11 17:51:13 UTC
Created attachment 319364 [details] contextual popover revision 3. Creating this bug for tracking the design and development of contextual popovers. The contextual popover primary purpose would be to provide a useful shortcut for private messaging.
marking bug to needing ui-review.
Some quick comments: - we should sync this with the user details in the user list (still useful for users who haven't said anything) - we need a busy state (fetching the information can take a while) - "Track Presence" sounds a bit creepy (the wording, not the functionality)
Created attachment 319839 [details] contextual popover rev 4 iteration. One thing i want your opinion on: Do you think its feasible to make some prediction about the user's availability based on past history?
I have made some some more progress at https://wiki.gnome.org/Design/Apps/Potential/Polari/ContextualPopup#Tentative_Design
Created attachment 333208 [details] polari-user-notify icon to ship with polari The mockup uses an icon from a mockup by Jakub Steiner, currently not shipped by the adwaita icon theme. I'm attaching it here so we can add it to the resources. Another option considered was using the 'preferences-system-notifications' icon, but it doesn't work so well at the required scale.
Created attachment 333506 [details] polari-user-notify icon to ship with polari
Created attachment 333511 [details] [review] userPopover: Add user-notify icon to icon path Just as a reminder to not forget integrating that when we land the user popovers: This is a patch we can squash to have 'polari-user-notify-symbolic' in the icon theme ...
Created attachment 334930 [details] [review] userDetails: Restrict length of fullnameLabel We need to impose this limit as names can sometimes get pretty long and we don't want a long name to stretch out the UserDetails.
Created attachment 334931 [details] [review] userDetails: Refactor UserDetails class and UI Refactor the UserDetails UI so that it matches the mockups and add the nickname setter that is used primarily for setting the _fullnameLabel.
Created attachment 334932 [details] [review] userListRow: Remove the avatar-default-symbolic icon The new design implies that the UserDetails class contains the avatar-default-symbolic icon for the user's full name. Consequently, having so many icons in the UserList would make it look overloaded. As a result, the UserListRow should have the icons removed.
Created attachment 334933 [details] [review] userListPopover: Change the search entry icon As the mockups suggest, the search entry should have another icon in order to transmit the idea that the search entry is actually searching for users, not anything else. We need this because the previous patch removes the avatar-default-symbolic icon from each UserListRow and the icon was a strog visual indicator.
Created attachment 334934 [details] [review] chatView: Split out status tracking As we want a new stand-alone way of tracking user status across rooms, we want this code to be placed separately from other modules in the app. As consequence, all the code that is related to user status tracking needs to be split out from the ChatView and moved to the new UserTracker module.
Created attachment 334935 [details] [review] userTracker: Add global tracking The first userTracker patch adds only the local tracking functionality of the userTracker, but we still need the global one too. This patch adds the global tracking functionality. First of all we create the UserStatusMonitor, a singleton that handles all other instances of UserTrackers. UserTrackers are now account-specific, meaning that they track rooms that are on the same account (network). Obviously, each room is tracked both individually (local tracking) and globally (global tracking). Global tracking is achieved through emitting the status-changed::basenick detailed signal while the local one uses custom callbacks. The reason we use custom callbacks is that it avoids filtering of the parameters sent by a signal. Local status is in connection with both the basenick and the room, hence double filtering would be required in the case of using a detailed signal for the local tracking part. Also, there is a new contacts-changed::basenick signal that is emitted whenever the contacts change for a specific basenick (useful when there are more users with the same basenick).
Created attachment 334936 [details] [review] application: Initialize UserStatusMonitor on startup The UserStatusMonitor connects to the AccountsMonitor signals in order to get notified whenever accounts are added or removed. Since we want to listen to those signals right from the beginning (so that we don't miss any signals) we need to instantiate the UserStatusMonitor as soon as possible, but after both the AccountsMonitor and ChatroomManager, since both are used in the UserTracker module. Also, as a safety measure, the UserTracker manually adds any accounts that are added prior to its construction.
Created attachment 334937 [details] [review] userDetails: Rename UserListDetails to UserDetails In order to reuse the old UserListDetails class into both the UserList and the upcoming UserPopover, we need to first give it a new name that does not restrict the class to the UserList. Future patches will also refactor the new UserDetails class.
Created attachment 334938 [details] [review] userDetails: Add offline support We want to be able to use the UserDetails class both when the user is offline and online. The offline functionality of the UserDetails class will only be visible in the UserPopover, as there are no offline users displayed in the UserList. We know which case we are in by the presence of the _user variable, which is passed as parameter at construction time. We also update the messageButton visibility whenever we set the _user variable, as messaging a user is only available in the online case, and only when the user is different from self.
Created attachment 334939 [details] [review] userPopover: Add user popover Right now, the UserPopover contains the nickname, the status (online/offline) and the UserDetails class. The UserPopover knows which user it is linked to by keeping a _nickname variable. The basenick is derived from the _nickname, the basenick being used to track the changes that the UserTracker signals about that specific basenick, thus correctly updating the status. The popover tracks the local and global status and also the changes that occur with the contacts associated to a basenick.
Created attachment 334940 [details] [review] chatView: Don't mingle separating whitespace with nick We get away with piggy-backing the '\t' character that separates nick and message with the former as long as the style we apply to the nick doesn't affect whitespace - this is about to change, so insert the whitespace separately. https://bugzilla.gnome.org/show_bug.cgi?id=752968
Created attachment 334941 [details] [review] chatView: Add HoverFilterTag We want to show the newly added UserPopover when clicking a nick in the chat log. Currently those nicks use a common tag for general styling and a per-nick tag to represent the online status of the nick, however we will also want to indicate that the element is activatable by highlighting it on hover. To allow this effect while keeping the status color in a per-nick tag, add a HoverFilterTag class that takes the color from an existing tag and applies an opacity effect when hovered.
Created attachment 334942 [details] [review] chatView: Split out _onNickTagClicked() and bind it to the nickTag We used to use the TextTag class for the nickTag, but now we replace that with the ButtonTag class, since we will be able to click the nickTag. The _onNickTagClicked() method is responsible for creating the popover at the right position upon clicking a nickTag. The popover is created only once for each nickTag, but it is created on demand, i.e. when the _onNickTagClicked() is run for the first time.
Created attachment 334943 [details] [review] userTracker: Add notify actions In order to be able to show a notification whenever a watched user comes online, we will use a GAction, as it makes the whole process a lot cleaner. Each basenick has an associated GAction, thus making the notifications basenick-specific (just like all the status tracking that is done) on that account (network). Whether or not the action is enabled is decided by the basenick's status. Whether or not a notification should be emitted is decided by the state of the GAction.
Created attachment 334944 [details] [review] userDetails: Add notification support We want to be able to emit a notification whenever a watched user comes online. The notification will be emitted by the new userTracker module, but we also need support for it in both the UserDetails and the UserPopover. The UserDetails class needs to show an icon and a label whenever the corresponding user is being watched, and hide those when the user is not being watched. The new notifications-enabled property is used to set the visibility of the two widgets. This property is controlled by the ToggleButton located in the UserPopover. We also add a new icon that will be used in the UserDetails and UserPopover classes.
Created attachment 334945 [details] [review] userPopover: Add notify button and action The ToggleButton (notifiButton) that is connected to the notify action is added to the popover. We also bind its visible property to its sensitive property, so that the button is hidden whenever the action is disabled. Moreover, the notifications-enabled property of the UserDetails are bound to the active property of the notifyButton, so that we can display the notifyLabel whenever the user is watched.
Review of attachment 334930 [details] [review]: Nitpick: We need to impose *a* limit - it's not like a value of 24 or 26 would be wrong
Review of attachment 334931 [details] [review]: Most of the code changes belong to another patch, see below. Regarding the commit message: - body needs to be manually wrapped (at ~75 characters) - the word "refactor" implies code restructuring, something like "userListDetails: Update to match latest mockups" would be more appropriate - the body talks about a nickname setter that's not even in the patch ... There's not much to say about the patch, so can be brief - something like: "We eventually want to offer an alternative way to show a user's detail by adding a contextual popover to the chat log, so update the UI to work in both contexts." ::: src/userList.js @@ -111,3 @@ 'detailsGrid', 'fullnameLabel', - 'lastHeader', This ... @@ +222,2 @@ if (last) { + this._lastLabel.label = this._formatLast(last); ... and this look like the only changes that belong into this patch. All the other code changes deal with this._user being unset, which is what the offline patch is about.
Review of attachment 334932 [details] [review]: Some minor suggestions for the commit message: - "avatar-default-symbolic" is overly specific, just "Remove the user icon" reads more naturally - "The new design implies" - I'd refer to the last patch instead, i.e. "Since the last patch, the details use icons instead of labels as headers. As a result, having icons in the rows as well looks overloaded, so remove those."
Review of attachment 334933 [details] [review]: Not a big fan of the body, but ok with s/strog/strong/
Review of attachment 334935 [details] [review]: The commit message goes to great length explaining what code is added (which can be seen by looking at the code), but not *why* (which is what really belong into a commit message). At the least, it needs to state: - why we want "global tracking" - what that actually is Something like: userTracker: Allow tracking users across rooms The tracker can be used to track the status of users in a particular room, however sometimes it is more interesting to know whether a user is online at all (for example to determine if it is possible to start a private conversation with them). While we don't have a way to get that information, we can approximate it by tracking users across all rooms we have joined. ::: src/chatView.js @@ +354,3 @@ + this._nickStatusChangedId = + this._userTracker.watchRoomStatus(this._room, null, + Lang.bind(this, this._onNickStatusChanged)); Wrong indentation @@ +1178,3 @@ } tags.push(nickTag); + Gratuitous whitespace change @@ +1219,3 @@ + _onNickStatusChanged: function(baseNick, status) { + let nickTagName = this._getNickTagName(baseNick); + let nickTag = this._lookupTag(nickTagName); Splitting out the nickTagName bit isn't wrong, but distracts a bit from the actual code change (removal of 'tracker' parameter + nickName -> baseNick) ::: src/userTracker.js @@ +27,3 @@ + + this._accountsMonitor.connect('account-added', Lang.bind(this, this._onAccountAdded)); + this._accountsMonitor.connect('account-removed', Lang.bind(this, this._onAccountRemoved)); Those lines could use some breaks to keep them roughly within 80 characters @@ +29,3 @@ + this._accountsMonitor.connect('account-removed', Lang.bind(this, this._onAccountRemoved)); + + this._accountsMonitor.accounts.forEach(a => { this._onAccountAdded(this._accountsMonitor, a); }); Dto. @@ +43,3 @@ + return; + + this._userTrackers.delete(account); It is not an error to try to delete an element that's not in the map (in fact, the return value of delete() indicates whether an element was actually removed), so the check above isn't needed. @@ +48,3 @@ + getUserTrackerForAccount: function(account) { + if (this._userTrackers.has(account)) + return this._userTrackers.get(account); We don't check for null vs. undefined, so this check isn't really useful - just returning "this._userTrackers.get(account);" unconditionally works just fine. @@ +204,3 @@ + this._runHandlers(room, member, status); + + this.emit("contacts-changed::" + baseNick, member.alias); Nit: Here the signal is emitted after updating the per-room map, while _untrackMember() does it before that. The signal really refers to the contacts stored in the global map, so emitting it after updating that map in both cases looks more consistent (doing it at the end looks nice though, so maybe update the roomMap first in both functions?) @@ +258,3 @@ + + if (contacts.length == 0) + return null; Explicitly handling this case is probably cleaner, but not strictly necessary - the loop doesn't run for an empty array, and contacts[0] is undefined in that case (which is just as good as null).
Review of attachment 334937 [details] [review]: Sorry, commit message again: There's nothing that prevents us from using a class called UserListDetails outside the user list, so the language is a bit strong ("need", "restrict"). "We will soon re-use the details outside the UserList, so use a more generic name." or something like that.
Review of attachment 334938 [details] [review]: Scrap the second part of the commit message that just rewords what the code is doing. Instead, extend the first part to briefly explain "UserPopover" (keep in mind that this is code that will only be added later), ideally in a way that makes it obvious why we need to handle the offline case. (Code looks fine at a glance, but I'll take a closer look after the corresponding bits from the style updates are moved here)
Review of attachment 334939 [details] [review]: Why?
Review of attachment 334940 [details] [review]: Might want to kill off the original bug URL
Review of attachment 334941 [details] [review]: It's a bit dodgy to review my own patches, but meh ...
Review of attachment 334942 [details] [review]: The commit messages really misses the point - the patch is about showing a UserPopover when a nickname is clicked, not about splitting out some method. ::: src/chatView.js @@ +1332,3 @@ + rect1.width = rect2.x - rect1.x; + + //TODO: special chars? I cannot confirm that it works, as both GNOME and Freenode servers reject nicknames that aren't spec compliant, but in any case this shouldn't be an issue - iters work on character positions, not byte indexes
Review of attachment 334944 [details] [review]: Again, a lot of talk about *what* changed, but nothing about the *why* (unless we count "it needs to show an icon and a label ... because of reasons"). Also "Add notification support" misses the point, the patch is adding additional information to the details, not emitting any notifications :-)
Review of attachment 334943 [details] [review]: The commit message should focus on the feature (notifying when a user comes online), not all the nitty-gritty of the implementation ... ::: src/userTracker.js @@ +304,3 @@ + notification.set_default_action_and_target('app.join-room', param); + + this._app.send_notification(this._getNotifyActionNameInternal(member.alias), notification); It's probably a good idea to withdraw that notification when the user goes offline
Created attachment 335040 [details] [review] userDetails: Restrict length of fullnameLabel We need to impose a limit as names can sometimes get pretty long and we don't want a long name to stretch out the UserDetails.
Created attachment 335041 [details] [review] userDetails: Update to match latest mockups We eventually want to offer an alternative way to show a user's details by adding a contextual popup to the chatView, so update the UI to work in both contexts.
Created attachment 335042 [details] [review] userListRow: Remove the user icon Since the last patch, the details use icons instead of labels as headers. As a result, having icons in the rows as well looks overloaded, so remove those.
Created attachment 335043 [details] [review] userListPopover: Change the search entry icon As the mockups suggest, the search entry should have another icon in order to transmit the idea that the search entry is actually searching for users, not anything else. We need this because the previous patch removes the avatar-default-symbolic icon from each UserListRow and the icon was a strong visual indicator.
Created attachment 335044 [details] [review] chatView: Split out status tracking As we want a new stand-alone way of tracking user status across rooms, we want this code to be placed separately from other modules in the app. As consequence, all the code that is related to user status tracking needs to be split out from the ChatView and moved to the new UserTracker module.
Created attachment 335045 [details] [review] userTracker: Allow tracking users across rooms The UserTracker can be used to track the status of users in a particular room, however sometimes it is more interesting to know whether a user is online at all (for example to determine if it is possible to start a private conversation with them). While we don't have a way to get that information, we can approximate it by tracking users across all rooms we have joined.
Created attachment 335046 [details] [review] application: Initialize UserStatusMonitor on startup The UserStatusMonitor connects to the AccountsMonitor signals in order to get notified whenever accounts are added or removed. Since we want to listen to those signals right from the beginning (so that we don't miss any signals) we need to instantiate the UserStatusMonitor as soon as possible, but after both the AccountsMonitor and ChatroomManager, since both are used in the UserTracker module. Also, as a safety measure, the UserTracker manually adds any accounts that are added prior to its construction.
Created attachment 335047 [details] [review] userDetails: Rename UserListDetails to UserDetails We will soon re-use te details class outside the UserList, so give it a more generic name so that its name doesn't exclusively tie it to the UserList.
Created attachment 335048 [details] [review] userDetails: Add offline support We want to be able to use the UserDetails class both when the user is offline and online. The offline functionality of the UserDetails class will only be visible in the UserPopover (which will be added in future patches) as there are no offline users displayed in the UserList. That being said, details about the user need to be shown inside the UserPopover when the user is offline aswell.
Created attachment 335049 [details] [review] userPopover: Add user popover We add the UserPopover because we want to add a more convenient way of accessing the information of the UserList without actually locating the user inside a potentially long list. Moreover, the UserPopover will be able to provide details when the user is offline aswell.
Created attachment 335050 [details] [review] chatView: Don't mingle separating whitespace with nick We get away with piggy-backing the '\t' character that separates nick and message with the former as long as the style we apply to the nick doesn't affect whitespace - this is about to change, so insert the whitespace separately.
Created attachment 335051 [details] [review] chatView: Add HoverFilterTag We want to show the newly added UserPopover when clicking a nick in the chat log. Currently those nicks use a common tag for general styling and a per-nick tag to represent the online status of the nick, however we will also want to indicate that the element is activatable by highlighting it on hover. To allow this effect while keeping the status color in a per-nick tag, add a HoverFilterTag class that takes the color from an existing tag and applies an opacity effect when hovered.
Created attachment 335052 [details] [review] chatView: Add click and hover support for the nickTag We want to be able to highlight the nickname upon hovering above it and we also want to be able to click it so that the UserPopover becomes visible. this is achieved through two separate tags, the ButtonTag (for the clicking part) and the HoverTag.
Created attachment 335053 [details] [review] userTracker: Add notify actions In order to be able to show a notification whenever a watched user comes online, we will use a GAction, as it makes the whole process a lot cleaner. Each basenick has an associated GAction, thus making the notifications basenick-specific (just like all the status tracking that is done) on that account (network). Whether or not the action is enabled is decided by the basenick's status. Whether or not a notification should be emitted is decided by the state of the GAction. Also, if the user becomes globally offline, the notification is withdrawn.
Created attachment 335054 [details] [review] userDetails: Add notification icon and label The UserDetails needs additional features before the actual notification feature is added. The two features are a new icon that will be used in both the UserPopover and the UserDetails and the label that signals that the user is being watched.
Created attachment 335055 [details] [review] userPopover: Add notify button and action The ToggleButton (notifiButton) that is connected to the notify action is added to the popover. We also bind its visible property to its sensitive property, so that the button is hidden whenever the action is disabled. Moreover, the notifications-enabled property of the UserDetails are bound to the active property of the notifyButton, so that we can display the notifyLabel whenever the user is watched.
Review of attachment 335040 [details] [review]: OK
Review of attachment 335041 [details] [review]: Yup
Review of attachment 335042 [details] [review]: LGTM
Review of attachment 335045 [details] [review]: Sure
Review of attachment 335047 [details] [review]: OK with s/te details/the details/
Review of attachment 335050 [details] [review]: LGTM
Review of attachment 335051 [details] [review]: LGTM
Review of attachment 335043 [details] [review]: Better: userListPopover: Change the search entry icon Since the last commit removed icons from rows, there's no longer any hint about the popover's purpose unless a row is expanded. As the filter entry is visible in most rooms, it is a good place to add back that indication.
Review of attachment 335044 [details] [review]: Suggestion: chatView: Split out status tracking Tracking the online status of nicks is not just useful for updating colors in the chat log, so move the code to a dedicated module.
Review of attachment 335046 [details] [review]: I was about to suggest to drop the patch, as it wasn't obvious why it was needed in the first place. It took me a while to realize that if we rely on the status monitor singleton being accessed from the chatView, we'll have missed the ::room-added signal for that room, so it wouldn't be tracked (and the "safety measure" doesn't catch it, as it only handles previously added accounts, but not rooms). Now *that* is something the commit message should mention, not a non-working safety measure, a non-existent module or pointless ordering (AccountsMonitor and RoomManager are both singletons, so whatever accesses them first makes sure they exist)
Review of attachment 335049 [details] [review]: Not great, but OK with s/aswell/as well/
Review of attachment 335054 [details] [review]: Better something along the lines of: userDetails: Indicate when status notifications are enabled We are about to add an image button to the popover to enable the previously added status notifications. However by itself its purpose won't be entirely clear, so show a brief description when the feature is enabled.
I'll add suggestions for the remaining four messages tomorrow.
Review of attachment 335048 [details] [review]: Not too happy with what I came up with, but here it is: userDetails: Add offline support Currently the details have a fixed associated TpContact, which is fine in the case of the user list where rows represent a particular nickname and are destroyed when that nickname leaves the room. However when we expose the details in the chat log, nicks are grouped together by basenick and may be either on- or offline, so allow changing and unsetting that property, and add a nickname property that is used as fallback when no contact is available.
Review of attachment 335052 [details] [review]: Mmmh, if someone asks you: "What did you do this summer" - are you going to say: "I added click and hover support to nicks"? I'd go with something like: chatView: Add a contextual popover to nicks We now have everything in place to make contextual nick information and actions available from the chat log, so turn nicknames into interactive elements and show the newly added UserPopover when clicked.
Review of attachment 335053 [details] [review]: My take: userTracker: Add notify-user actions It is not uncommon that users need to chat with a particular person who is not connected at the time. As we are already tracking the online status of nicks, we can support this case by showing a notification when a particular user comes online. For that purpose, add a getNotifyActionName() method that returns the name of a stateful action that controls whether this feature is enabled for a particular basenick.
Review of attachment 335055 [details] [review]: Not really happy with this either, but meh: userPopover: Allow requesting a notification when a user comes online As the user popover is still available when nicks are offline, it is a good place to expose the newly added notify-user functionality, so add a toggle button to the popover to get a notification when the user comes online.
Created attachment 335519 [details] [review] userDetails: Restrict length of fullnameLabel We need to impose a limit as names can sometimes get pretty long and we don't want a long name to stretch out the UserDetails.
Created attachment 335520 [details] [review] userDetails: Update to match latest mockups We eventually want to offer an alternative way to show a user's details by adding a contextual popup to the chatView, so update the UI to work in both contexts.
Created attachment 335521 [details] [review] userListRow: Remove the user icon Since the last patch, the details use icons instead of labels as headers. As a result, having icons in the rows as well looks overloaded, so remove those.
Created attachment 335522 [details] [review] userListPopover: Change the search entry icon Since the last commit removed icons from rows, there's no longer any hint about the popover's purpose unless a row is expanded. As the filter entry is visible in most rooms, it is a good place to add back that indication.
Created attachment 335523 [details] [review] chatView: Split out status tracking Tracking the online status of nicks is not just useful for updating colors in the chat log, so move the code to a dedicated module.
Created attachment 335524 [details] [review] userTracker: Allow tracking users across rooms The UserTracker can be used to track the status of users in a particular room, however sometimes it is more interesting to know whether a user is online at all (for example to determine if it is possible to start a private conversation with them). While we don't have a way to get that information, we can approximate it by tracking users across all rooms we have joined.
Created attachment 335525 [details] [review] application: Initialize UserStatusMonitor on startup The UserStatusMonitor connects to the AccountsMonitor signals in order to get notified whenever accounts are added or removed. We want to listen to those signals right from the beginning, because we need to able to listen for the ::room-added signal (emitted by the RoomManager inside each UserTracker) right from the start, as failing to receive this signal would mean that the room is not tracked.
Created attachment 335526 [details] [review] userDetails: Rename UserListDetails to UserDetails We will soon re-use the details class outside the UserList, so give it a more generic name so that its name doesn't exclusively tie it to the UserList.
Created attachment 335527 [details] [review] userDetails: Add offline support Currently the details have a fixed associated TpContact, which is fine in the case of the user list where rows represent a particular nickname and are destroyed when that nickname leaves the room. However when we expose the details in the chat log, nicks are grouped together by basenick and may be either on- or offline, so allow changing and unsetting that property, and add a nickname property that is used as fallback when no contact is available.
Created attachment 335528 [details] [review] userPopover: Add user popover We add the UserPopover because we want to add a more convenient way of accessing the information of the UserList without actually locating the user inside a potentially long list. Moreover, the UserPopover will be able to provide details when the user is offline as well.
Created attachment 335529 [details] [review] chatView: Don't mingle separating whitespace with nick We get away with piggy-backing the '\t' character that separates nick and message with the former as long as the style we apply to the nick doesn't affect whitespace - this is about to change, so insert the whitespace separately.
Created attachment 335530 [details] [review] chatView: Add HoverFilterTag We want to show the newly added UserPopover when clicking a nick in the chat log. Currently those nicks use a common tag for general styling and a per-nick tag to represent the online status of the nick, however we will also want to indicate that the element is activatable by highlighting it on hover. To allow this effect while keeping the status color in a per-nick tag, add a HoverFilterTag class that takes the color from an existing tag and applies an opacity effect when hovered.
Created attachment 335531 [details] [review] chatView: Add a contextual popover to nicks We now have everything in place to make contextual nick information and actions available from the chat log, so turn nicknames into interactive elements and show the newly added UserPopover when clicked.
Created attachment 335532 [details] [review] userTracker: Add notify-user actions It is not uncommon that users need to chat with a particular person who is not connected at the time. As we are already tracking the online status of nicks, we can support this case by showing a notification when a particular user comes online. For that purpose, add a getNotifyActionName() method that returns the name of a stateful action that controls whether this feature is enabled for a particular basenick.
Created attachment 335533 [details] [review] userDetails: Indicate when status notifications are enabled We are about to add an image button to the popover to enable the previously added status notifications. However by itself its purpose won't be entirely clear, so show a brief description when the feature is enabled.
Created attachment 335534 [details] [review] userPopover: Allow requesting a notification when a user comes online As the user popover is still available when nicks are offline, it is a good place to expose the newly added notify-user functionality, so add a toggle button to the popover to get a notification when the user comes online.
Looks good to me now, thanks!
Created attachment 335574 [details] [review] userDetails: Restrict length of fullnameLabel We need to impose a limit as names can sometimes get pretty long and we don't want a long name to stretch out the UserDetails.
Created attachment 335575 [details] [review] userDetails: Update to match latest mockups We eventually want to offer an alternative way to show a user's details by adding a contextual popup to the chatView, so update the UI to work in both contexts.
Created attachment 335576 [details] [review] userListRow: Remove the user icon Since the last patch, the details use icons instead of labels as headers. As a result, having icons in the rows as well looks overloaded, so remove those.
Created attachment 335577 [details] [review] userListPopover: Change the search entry icon Since the last commit removed icons from rows, there's no longer any hint about the popover's purpose unless a row is expanded. As the filter entry is visible in most rooms, it is a good place to add back that indication.
Created attachment 335578 [details] [review] chatView: Split out status tracking Tracking the online status of nicks is not just useful for updating colors in the chat log, so move the code to a dedicated module.
Created attachment 335579 [details] [review] userTracker: Allow tracking users across rooms The UserTracker can be used to track the status of users in a particular room, however sometimes it is more interesting to know whether a user is online at all (for example to determine if it is possible to start a private conversation with them). While we don't have a way to get that information, we can approximate it by tracking users across all rooms we have joined.
Created attachment 335580 [details] [review] application: Initialize UserStatusMonitor on startup The UserStatusMonitor connects to the AccountsMonitor signals in order to get notified whenever accounts are added or removed. We want to listen to those signals right from the beginning, because we need to able to listen for the ::room-added signal (emitted by the RoomManager inside each UserTracker) right from the start, as failing to receive this signal would mean that the room is not tracked.
Created attachment 335581 [details] [review] userDetails: Rename UserListDetails to UserDetails We will soon re-use the details class outside the UserList, so give it a more generic name so that its name doesn't exclusively tie it to the UserList.
Created attachment 335582 [details] [review] userDetails: Add offline support Currently the details have a fixed associated TpContact, which is fine in the case of the user list where rows represent a particular nickname and are destroyed when that nickname leaves the room. However when we expose the details in the chat log, nicks are grouped together by basenick and may be either on- or offline, so allow changing and unsetting that property, and add a nickname property that is used as fallback when no contact is available.
Created attachment 335583 [details] [review] userPopover: Add user popover We add the UserPopover because we want to add a more convenient way of accessing the information of the UserList without actually locating the user inside a potentially long list. Moreover, the UserPopover will be able to provide details when the user is offline as well.
Created attachment 335584 [details] [review] chatView: Don't mingle separating whitespace with nick We get away with piggy-backing the '\t' character that separates nick and message with the former as long as the style we apply to the nick doesn't affect whitespace - this is about to change, so insert the whitespace separately.
Created attachment 335585 [details] [review] chatView: Add HoverFilterTag We want to show the newly added UserPopover when clicking a nick in the chat log. Currently those nicks use a common tag for general styling and a per-nick tag to represent the online status of the nick, however we will also want to indicate that the element is activatable by highlighting it on hover. To allow this effect while keeping the status color in a per-nick tag, add a HoverFilterTag class that takes the color from an existing tag and applies an opacity effect when hovered.
Created attachment 335586 [details] [review] chatView: Add a contextual popover to nicks We now have everything in place to make contextual nick information and actions available from the chat log, so turn nicknames into interactive elements and show the newly added UserPopover when clicked.
Created attachment 335587 [details] [review] userTracker: Add notify-user actions It is not uncommon that users need to chat with a particular person who is not connected at the time. As we are already tracking the online status of nicks, we can support this case by showing a notification when a particular user comes online. For that purpose, add a getNotifyActionName() method that returns the name of a stateful action that controls whether this feature is enabled for a particular basenick.
Created attachment 335588 [details] [review] userDetails: Indicate when status notifications are enabled We are about to add an image button to the popover to enable the previously added status notifications. However by itself its purpose won't be entirely clear, so show a brief description when the feature is enabled.
Review of attachment 335583 [details] [review]: LGTM
Review of attachment 335579 [details] [review]: ::: src/userTracker.js @@ +85,3 @@ + _onShutdown: function() { + for (let [room, data] of this._roomData) 'data' isn't used, so this could be for (let room of this._roomData.keys()) ... instead
The other patches haven't changed as far as I can see, so bulk-approving - you could just upload the patches that changed (ideally with a small comment about the change); the patch order would get messed up, which makes applying them harder, but that's what we have the WIP branch for ...
Created attachment 335692 [details] [review] userTracker: Allow tracking users across rooms The UserTracker can be used to track the status of users in a particular room, however sometimes it is more interesting to know whether a user is online at all (for example to determine if it is possible to start a private conversation with them). While we don't have a way to get that information, we can approximate it by tracking users across all rooms we have joined.
Review of attachment 335692 [details] [review]: LGTM now
Created attachment 337359 [details] [review] userDetails: Restrict length of fullnameLabel We need to impose a limit as names can sometimes get pretty long and we don't want a long name to stretch out the UserDetails.
Created attachment 337360 [details] [review] userDetails: Update to match latest mockups We eventually want to offer an alternative way to show a user's details by adding a contextual popup to the chatView, so update the UI to work in both contexts.
Created attachment 337361 [details] [review] userListRow: Remove the user icon Since the last patch, the details use icons instead of labels as headers. As a result, having icons in the rows as well looks overloaded, so remove those.
Created attachment 337362 [details] [review] userListPopover: Change the search entry icon Since the last commit removed icons from rows, there's no longer any hint about the popover's purpose unless a row is expanded. As the filter entry is visible in most rooms, it is a good place to add back that indication.
Created attachment 337363 [details] [review] chatView: Split out status tracking Tracking the online status of nicks is not just useful for updating colors in the chat log, so move the code to a dedicated module.
Created attachment 337364 [details] [review] userTracker: Allow tracking users across rooms The UserTracker can be used to track the status of users in a particular room, however sometimes it is more interesting to know whether a user is online at all (for example to determine if it is possible to start a private conversation with them). While we don't have a way to get that information, we can approximate it by tracking users across all rooms we have joined.
Created attachment 337365 [details] [review] application: Initialize UserStatusMonitor on startup The UserStatusMonitor connects to the AccountsMonitor signals in order to get notified whenever accounts are added or removed. We want to listen to those signals right from the beginning, because we need to able to listen for the ::room-added signal (emitted by the RoomManager inside each UserTracker) right from the start, as failing to receive this signal would mean that the room is not tracked.
Created attachment 337366 [details] [review] userDetails: Rename UserListDetails to UserDetails We will soon re-use the details class outside the UserList, so give it a more generic name so that its name doesn't exclusively tie it to the UserList.
Created attachment 337367 [details] [review] userDetails: Add offline support Currently the details have a fixed associated TpContact, which is fine in the case of the user list where rows represent a particular nickname and are destroyed when that nickname leaves the room. However when we expose the details in the chat log, nicks are grouped together by basenick and may be either on- or offline, so allow changing and unsetting that property, and add a nickname property that is used as fallback when no contact is available.
Created attachment 337368 [details] [review] userPopover: Add user popover We add the UserPopover because we want to add a more convenient way of accessing the information of the UserList without actually locating the user inside a potentially long list. Moreover, the UserPopover will be able to provide details when the user is offline as well.
Created attachment 337369 [details] [review] chatView: Don't mingle separating whitespace with nick We get away with piggy-backing the '\t' character that separates nick and message with the former as long as the style we apply to the nick doesn't affect whitespace - this is about to change, so insert the whitespace separately.
Created attachment 337370 [details] [review] chatView: Add HoverFilterTag We want to show the newly added UserPopover when clicking a nick in the chat log. Currently those nicks use a common tag for general styling and a per-nick tag to represent the online status of the nick, however we will also want to indicate that the element is activatable by highlighting it on hover. To allow this effect while keeping the status color in a per-nick tag, add a HoverFilterTag class that takes the color from an existing tag and applies an opacity effect when hovered.
Created attachment 337371 [details] [review] chatView: Add a contextual popover to nicks We now have everything in place to make contextual nick information and actions available from the chat log, so turn nicknames into interactive elements and show the newly added UserPopover when clicked.
Created attachment 337372 [details] [review] userTracker: Add notify-user actions It is not uncommon that users need to chat with a particular person who is not connected at the time. As we are already tracking the online status of nicks, we can support this case by showing a notification when a particular user comes online. For that purpose, add a getNotifyActionName() method that returns the name of a stateful action that controls whether this feature is enabled for a particular basenick.
Created attachment 337373 [details] [review] userDetails: Indicate when status notifications are enabled We are about to add an image button to the popover to enable the previously added status notifications. However by itself its purpose won't be entirely clear, so show a brief description when the feature is enabled.
Created attachment 337374 [details] [review] userPopover: Allow requesting a notification when a user comes online As the user popover is still available when nicks are offline, it is a good place to expose the newly added notify-user functionality, so add a toggle button to the popover to get a notification when the user comes online.