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.

Sign up for GitHub

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

Jump to bottom

Add DM panel to the left side view (rebased #1416) #1532

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add DM panel to the left side view (rebased #1416) #1532

wants to merge 12 commits into from

Conversation

neiljp
Copy link
Collaborator

@neiljp neiljp commented Jul 11, 2024

What does this PR do, and why?

This is an update on the current work in #1416.

Currently this only includes a few small fixups over #1416, that may be included or not:

  • Make the new stream messages button not selectable (cannot receive focus)
  • Adds a simple > prefix for the new Stream messages button as STREAM_MESSAGE_MARKER
  • Make the DM messages button not selectable (cannot receive focus) [for now]

The way this is currently structured, this could be merged in two chunks (note that the summary of each chunk is not on a commit-wise basis):

  • up to and including my symbols/buttons commit
    • adds the data for the total stream messages into a new (non-selectable) button
    • changes the left panel structure to have a simple dividing line from the top-left view buttons, rather than a ---Streams--- or ---Topics--- title
    • clarifies the context after that change by specifying 'Search topics' and 'Search streams' rather than the previous 'Search' in the panel search box
  • that plus the rest of the commits
    • Moves the Direct messages button into a section with its own solid line separating it
    • Adds lots of model support for DM data, and updating it
    • Adjusts hotkeys so that from the left panel, S shows the view after the first PR chunk (above), with P switching to a view with the P view fully expanded
    • The fully expanded P view shows conversations with unreads, and user status
      (group conversations are included, unlike in the user list)
    • position in the DM list (and stream list) is preserved on switching between them

An additional UI 'chunk' could be to do the first bullet of the last chunk distinct from the data and view changes, but this change is currently combined with one of the later commits in the second chunk. That last chunk could also be split into data and UI updates, potentially.

Feedback sought

Manual testing and feedback on:

  1. the first chunk (eg. git rebase -i and 'edit' on the 'symbols/buttons' commit)
  • This is the anticipated UI direction to enable the full PR to merge
  • how does it feel during use?
  • does stream/topic switching look good?
  • how does it compare to main for you?
  1. the full PR

Outstanding aspect(s)

  • P for DM feed functionality is currently removed, even from message context; it would be useful to retain this feature generally? [discuss]
    • Even the P DM feed button is not usable right now, for simplicity
  • From the user list, P changes the left panel view, but doesn't re-focus; that's clearly buggy, but should it do something like that? [discuss]
  • S now works differently in left panel (new view-change) vs message context (zoom-in); is that OK?
  • While position in the stream list, and in any connected topic list, is preserved upon switching to/from the DM list, if the topic list was shown previously then it instead shows the stream list, which seems inconsistent? (eg. streams -t-> topics -P-> DMs -S-> streams)
  • Sort user names in DM list like web app?
  • Ensure DM list status symbols are updated
  • Use 'huddle' symbols for conversations with >2 people (eg. ∷)

Note that not all of these are necessary for a full merge and could belong in follow-ups, but some relate to what seems like buggy, inconsistent or reduced behavior.

External discussion & connections

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

theViz343 and others added 12 commits July 10, 2024 21:39
@theViz343 @neiljp
buttons/views: Replace stream view with stream panel on left sidebar.
88487e6 
This commit reworks the stream view section in the left sidebar by
replacing it with the new Stream Panel widget. The stream panel consists
of the newly added stream messages button (StreamPanelButton), and the
existing stream/topic view section.

Test added, updated and renamed.
@theViz343 @neiljp
boxes/views: Add context label parameter to PanelSearchBox.
2c846b6 
This commit adds a label parameter to the PanelSearchBox to provide
context to the user which searching the streams/topics list.

Tests updated.
@theViz343 @neiljp
helper/views: Add all_streams unread count to the UnreadCounts dict.
afd829c 
This commit adds an all_streams key to the UnreadCounts dict to store
the unread stream messages count. This is subsequently used by the
StreamPanelButton in the stream panel.

Test case added and tests updated.
@neiljp
fixup: Make StreamPanelButton not selectable for now.
781962f
@neiljp
symbols/buttons: fixup: Add simple prefix for Stream messages.
a2451af
@theViz343 @neiljp
model: Fetch recent direct messages from the Zulip API.
367b6a8 
This commit adds a list of recent direct messages for the user to the ZT
model (recent_dms). It also adds a _sort_recent_dms function to keep the
list in sorted order according to the max_message_id of the dm.

Fixture added and test updated.
@theViz343 @neiljp
model: Update recent_dms while handling a message_event.
9270537 
This commit adds an _update_recent_dms method which updates the
recent_dms list when a message event is received. It keeps the list in
sorted order as well.

Tests modified.
@theViz343 @neiljp
buttons/views:Add DMButton, DMPanel and DMView classes.
23f28ad
@theViz343 @neiljp
views: Rework direct messages UI.
88ff377 
This commit reworks the direct messages button by moving it to a
different section between the menu and the streams panel. It also
adds a direct messages panel which shows all DM recipients.
@theViz343 @neiljp
buttons/views: Rename PMButton to DMPanelButton.
3298cb6
@theViz343 @neiljp
buttons/views: Add panel folding for DM and stream panels.
518e8d2
@neiljp
fixup: Switch DM panel button to not be selectable.
9167fb3
@neiljp neiljp added enhancement New feature or request area: UI General user interface update feedback wanted labels Jul 11, 2024
@zulipbot zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 11, 2024
@Niloth-p
Copy link
Collaborator

Niloth-p commented Jul 17, 2024 •
edited
Loading

P for DM feed functionality is currently removed, even from message context; it would be useful to retain this feature generally? [discuss]

I don't see why we'd need to retain the feed.

Sort user names in DM list like web app?

Yep, I agree, that's a required feature.

S now works differently in left panel (new view-change) vs message context (zoom-in); is that OK?

I am not sure that's intuitive.

From the user list, P changes the left panel view, but doesn't re-focus; that's clearly buggy, but should it do something like that? [discuss]

Well, re-focusing could help.

Ensure DM list status symbols are updated

By status, do you mean the presence status? Is that not currently in sync?

Next Steps:

These are just my thoughts and opinions, please share feedback on whether these are good next steps.

  • Fix tests and linting.
  • Adding new tests.
  • Currently only fetches recent DMs, as was previously discussed in CZO and the PR - I have not personally tested this.
  • Refactor the DMButton code, as per Vishwesh's last comment on his PR.
  • The DM button unread counts are not updated. The total DM counts get updated, but not for each DM.
  • DMs with self are not handled properly. Sets user as None.
  • Focus is currently retained on the stream list, must also retain focus on the topic list.

Entries picked from the "outstanding aspects" list in the PR description by @neiljp,

  • Ensure DM list status symbols are updated
  • Use 'huddle' symbols for conversations with >2 people (eg. ∷)
  • Sort user names in DM list like web app
  • Opening stream panel with S must open the previously active list (stream / topic)

Optionally, after discussion:

  • Switch focus to the DM Panel on pressing P
  • Pressing P/S again must fold/collapse the open panel, and/or expand the other panel
    Or alternatively, hide the [P] or [S] suggestion on the buttons when the respective panel is open.
  • The S key behavior needs to be adjusted. Currently,
    • Opens the stream panel only from the left panel.
    • Topic narrows from the middle column. But it shows the narrow as "# <stream_name> topic narrow".
    • Does not work from the user list.
  • Handling the no DM case smoothly.
    We don't currently handle the no messages case / empty narrows either.

The empty space in the DM Panel can be dominating, when there aren't enough DMs, as suggested by @neiljp, previously on a call. The Stream Panel being folded to the bottom can be easy to miss until one gets used to the UI.

@neiljp neiljp changed the title Add DM panel to the left side view (from #1416) Add DM panel to the left side view (rebased #1416) Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area: UI General user interface update enhancement New feature or request feedback wanted size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@neiljp @Niloth-p @zulipbot @theViz343