Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User management #350

Closed
jtdaugherty opened this issue Jan 17, 2018 · 18 comments
Closed

User management #350

jtdaugherty opened this issue Jan 17, 2018 · 18 comments

Comments

@jtdaugherty
Copy link
Member

This ticket is about a few things all at once:

  • Matterhorn has never correctly fetched all users. Historically we've asked for the first 10,000, but we never asked for more if there were any.
  • The switch to API V4 revealed that the new maximum number of users returned in a single request is 200, so on servers with more than 200 users, Matterhorn will never fetch them all at startup.
  • Matterhorn fetches unknown users when it encounters them, e.g. when someone posts a message. But it does this inefficiently when more than one message is being processed. It queues up one request per user rather than batching them.
  • More generally, the UI of showing all users isn't providing much value and Matterhorn should move toward a UI more consistent with that of the official web client, where a heuristic is used to determine which users to show in the sidebar.

We'll explore these issues and others related to them and figure out how we want to tackle this.

@jtdaugherty
Copy link
Member Author

Here's another related bug that I observed:

  • On startup, we fetch messages.
  • Depending on the race to get the user status information, we might not know about some authors of posts, so we queue up fetches for user information.
  • Meanwhile, user status information gets updated.
  • Then the queued fetches for user information finish and blow away the user status updated above, which causes users previously appearing as online to be displayed as offline until the periodic status refresh occurs, at which point they are displayed as being online again.

@jtdaugherty
Copy link
Member Author

jtdaugherty commented Jan 19, 2018

(The above behavior was observed on develop as of 5d83107.)

@jtdaugherty
Copy link
Member Author

The outcomes of our discussion this morning on this issue were:

  • Agreement that showing all the users is something we should stop trying to do.
  • Instead, let's find a reasonable heuristic for showing the subset of users that the logged-in user cares about.
  • To support finding, chatting with, and learning about users, we can consider implementing an interface that uses the server's user search API endpoint. The idea is that the user can type in substrings of user display names and usernames and see the server's search results immediately. This avoids the need to show all users in C-g mode, or even to keep all the user information resident in memory. When the user hits Enter on such a user search result, a DM chat can be initiated. In other uses, this interface might be restricted to the list of users who are members of the current channel (e.g. if /members is modified to trigger this UI instead of dumping the list of users to the channel). So we'd like /members to use thi UI, too.
  • We'd also like to re-use the above UI to allow easier invitations of users to channels.
  • Consequences of not keeping all user information around all the time include but are not limited to:
    • Tab completion won't know about all users, so tab completion needs to be rewritten to use the server's username autocomplete API endpoint. This also needs to be aware that we might not always have a user sigil (@) present on the autocomplete input.
    • The DM channel list needs to be restricted to a smaller set of users that we care about, similar to how the web client operates. We can do some investigation on their heuristics and implement similar ones. One consideration is that any heuristic involving timestamps needs to be very careful mixing local time and server time if that becomes necessary.

We also need to understand the consequences of moving to this scheme in places where the application may currently assume that all users are present in the internal user list.

@jtdaugherty
Copy link
Member Author

This might also possibly enable us to maintain a new invariant that will simplify channel list rendering: all entries in the channel list must actually correspond to a channel ID, since only users with corresponding DM channels will ever appear. (This assumes the heuristic considers channel metadata to decide which users to show, and it also assumes that upon selecting a user from the user search UI described above, a DM channel is created if one doesn't already exist.)

@jtdaugherty
Copy link
Member Author

Another item from our discussion this morning was that the user listing UI described above will also need to display full user details, including username, display name, and status (online, offline, etc.).

@kquick
Copy link
Collaborator

kquick commented Jan 22, 2018

Not entirely clear above, but the "interface to the server's user search" described above is a separate display item, either replacing the messages area or as a dialog window overlaying the messages. This would have its own input box where the user's interactions affect just that portion, with either an Esc to dismiss this display and return to the normal Matterhorn or by selecting one of the presented users, which will then act on that user as described above.

Regarding current user status:

The web UI shows the user status as an icon relative to the user's name each time it appears next to a post in the messages window, and if there happens to be a DM for that user in the sidebar the user status is also shown there.

Historically for Matterhorn, since we (thought we were) showing all users, the sidebar provided the current status indication for the user as well as the DM handle. There is a desire to show the user status, but there will not necessarily be a DM entry in the sidebar to attach the user status to, but adding an indicator to each username in the chat window is more expensive on a character-cell display.

The solution we decided upon for handling user status is to use the user selection display described above. There is a server query that will allow that display to be populated just with members of the current channel, and at that point the current status (online, away, etc.) of each user can be displayed. The DM sidebar will still have status annotations as well, but only for those DM users. It's recommended that this current-channel user display be tied to a single key event for quick and easy display (and selecting a particular user in this mode would go to a DM with that user, just as with a general user selection display).

@jtdaugherty
Copy link
Member Author

Not entirely clear above, but the "interface to the server's user search" described above is a separate display item, either replacing the messages area or as a dialog window overlaying the messages

Yeah, I was deliberately avoiding jumping to conclusions about how it would be represented, although I favor a dialog-style approach.

@jtdaugherty
Copy link
Member Author

Some other considerations:

  • What do we do about user info fetching via mmGetUsers in refreshChannelsAndUsers? Postcondition(s): users visible in the sidebar should be the users we update here.
  • For fetchChannelMembers, that'll do something with the above UI where we search server-side and show the results (as many as will fit) and provide a cursor to select from amongst them.
  • handleNewUsers currently fetches users-in-team data. For this one, look at better ways of dealing with this situation rather than fetching with mmGetUsers.

Other notes about user list UI:

  • Show total channel member count in window title
  • Show user online/offline/etc status
  • Speculatively fetch next/prev page based on cursor position
  • Add search input box (if empty, show the first page of users; otherwise perform server-side search based on input, and reset the cursor position to zero on a new search when the input box contents change)

Other tasks:

  • Change "Users" sidebar section title to "Direct Messages" once we no longer show all users there
  • Investigate web client heuristic for which DM channels to show in the sidebar
  • Make a single request (instead of N) for unknown message authors while processing channel scrollback fetches
  • Investigate how the web client deals with 1) deleted users and 2) user profile fetching and caching
  • User cache: user ID, username, display name, e-mail, nickname
  • Investigate potential for improved encapsulation of user state management and API

@jtdaugherty
Copy link
Member Author

Make a single request (instead of N) for unknown message authors while processing channel scrollback fetches

This is now done in develop.

@jtdaugherty
Copy link
Member Author

Here's a link to the pre-release thread where I got information about how the web client decides which users to show in the sidebar: https://pre-release.mattermost.com/core/pl/6oxrd4yykjb1bjpcai5dpqy5th

@jtdaugherty
Copy link
Member Author

For posterity: @aisamanra started working on a user list overlay in the https://github.com/matterhorn-chat/matterhorn/tree/feature/user-list-overlay branch. I've done some more work on it and it's far enough along that now:

  • The user list window provides a text editor with searches conducted as the user types input.
  • /msg shows the user list window with all users on the server; Enter starts a DM chat with the selected user.
  • /add-user with no arguments shows the overlay with users who are not members of the current channel; Enter adds the selected user to the channel.
  • /members shows the overlay with users who are members of the current channel; Enter starts a DM chat with the selected user.

@jtdaugherty
Copy link
Member Author

jtdaugherty commented Feb 16, 2018

Here is the heuristic summary inlined to avoid broken links later:

Depends on server and account config now, but on pre-release it auto-closes conversations
with no activity for 7 days, and auto-closes one-on-one dms with deactivated users.

Client-side, we store/update a preference whenever the user opens a channel, so the logic
to determine whether a channel is visible is a function of...

* Whether on not the channel is favorited
* The time that the user opened the channel
* The time that the other user in the DM was deactivated (if they aren't active and
  it's a one-on-one DM)
* The server-side CloseUnusedDirectMessages config setting
* The user's account setting for auto-closing DMs
* The last post time of the channel

@jtdaugherty
Copy link
Member Author

Also:

If the server-side config is "false" or the user setting is set to "never" close DMs,
the client keeps DM channels open forever (or until the other user is deactivated).
Otherwise they close after 7 days.

@jtdaugherty
Copy link
Member Author

Related: #376

@jtdaugherty
Copy link
Member Author

(Let's also be sure to check on whether our work on this addresses the concerns raised in #376.)

@jtdaugherty jtdaugherty added this to the User list improvements milestone Nov 10, 2018
jtdaugherty added a commit that referenced this issue Nov 13, 2018
…hold

This change starts us down the path of only showing direct message
channels that are sufficiently relevant to be displayed, based on a
subset of the conditions described in #350. Ultimately we'll implement
more, but this is an initial test of filtering the DM channels.
@jtdaugherty
Copy link
Member Author

The main concerns in this ticket, along with most of the new features that were called for, have been addressed partly by changes to master since this ticket was created, and more recently in the refactor/user-list branch which will be merged soon. Yay!

@no-longer-on-githu-b
Copy link

no-longer-on-githu-b commented Dec 24, 2018

More generally, the UI of showing all users isn't providing much value and Matterhorn should move toward a UI more consistent with that of the official web client, where a heuristic is used to determine which users to show in the sidebar.

Is there a way to restore the old behavior? I just want to ctrl+G to go to any user. I'd also like to see at a glance what users are online now, even if the heuristic doesn't think I want to talk to them. If not then I'll downgrade to the previous version (or submit a patch (or maintain a fork)).

In fact, the reason I'm using Matterhorn is because it has features that the web client doesn't have. If the feature sets are being unified, then there's no reason for me to use Matterhorn.

@jtdaugherty
Copy link
Member Author

@rightfold there is not a way to restore the previous behavior, but there is /msg to search for users and open a DM channel with one.

As far as the previous behavior of C-g is concerned, though, I think we'd be open to considering a patch that does server autocomplete queries to fill out the C-g list with extra results from the server that are not already in the channel sidebar. Some of that infrastructure already exists in the form of the user and channel autocompletion feature in the latest releases, so it's probably not a big leap to re-use it for C-g mode. If you were to undertake this, for best results, we'd like to coordinate the planned work before you begin.

As far as the overall relationship to the web client is concerned, I'm glad to hear that you're using Matterhorn because of its feature set. Our position is as follows: we intend to add web client features to Matterhorn to the extent that they can be added in a terminal setting. Our goal is to do so in a way that minimizes surprise to web client users migrating to Matterhorn while also providing the best terminal user experience that we can think of. That might entail adding the web client features but changing their designs to more terminal-friendly ones, or it might mean omitting aspects of web client features that rely heavily on mouse- or DOM-related UI idioms. It might also entail adding web client features but deviating slightly on specific behaviors, which is definitely the case for the new sidebar changes.

While our intention is not to remove features from Matterhorn to bring it into parity with the web client -- I'd definitely like to see Matterhorn providing features that the web client doesn't -- in this particular case we did so because the fact that all users were shown in the sidebar was actually a combination of historical accident, best effort, bugs, and lack of understanding of the web client's UI. I understand that our motivation for making the change doesn't help you and that you want the old behavior back. We've had some discussions about that ourselves on the development team, which is why I mentioned above that we'd be open to discussing it.

Thanks for your feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants