zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard
verified publisher icon zulip

Handling Empty Narrows

Open Niloth-p opened this issue 1 year ago • 0 comments

What does this PR do, and why?

Fixes all the bugs related to empty narrows, and recipient bars when a message is not selected.

External discussion & connections

  • [ ] Discussed in #zulip-terminal in topic
  • [x] Fully fixes #895
  • [x] Partially fixes issue #1506
  • [x] Builds upon previous unmerged work in PR #278
  • [ ] Is a follow-up to work in PR #
  • [ ] Requires merge of PR #
  • [ ] Merge will enable work on #

Solution Space Exploration

Before settling on the dummy message approach, I explored a few other solutions as well:

  1. I explored not switching narrows to empty narrows. We'd need to do some refactoring to get_message_ids_in_current_narrow to allow checking if a future narrow is empty, before switching to it.

  2. And I also explored whether we could expand our system of handling narrows to also cater to narrows without messages, without handling it as an exception case and introducing a dummy message. I tried to move top_search_bar and recipient_header outside of MessageBox to change how they're tightly tied to the focused message, which is why they're not updated currently when in an empty narrow, and the erratic behavior.

  3. Instead of a dummy message, I gave a shot at using a Text widget replacing the entire MessageView, like we do with the PanelSearchBoxes when there are no search hits. But yes, I ran into a few cases with focus issues. I tried moving the Text element inside, and modifying/turning off the ModListWalker's action each time, to make it behave similar to the SimpleFocusListWalker, but that still wasn't a clean fix.

And other similar tangents, before realising the dummy message approach is the solid one, even if we were to implement any of these in the future, especially the 1st approach, the dummy message should be a backup.

I'm sure a more experienced person would have been able to figure this out much faster than I did, and without all this exploration, so I'd be interested in hearing the thinking process of someone who was able to / can narrow in on (pun intended) the dummy message solution right away.

I also decided against removing the message recipients section for empty narrows, because currently when opening the app, it shows up with "DONT HIDE", so I went with a solution that conforms to that behavior.

Outstanding aspect(s)

  • I've added the features for the dummy message, before adding the dummy message, so that it starts working as soon as added.
  • Placed the current narrow state tracking variable in the controller class.
  • I wasn't able to use even a single line of code as-is from #278, as this PR is quite different and uses several different decisions, so I just decided to add 278's author as a co-author for all ideas that were inspired from their PR.
  • I haven't added "bugfix" prefix to the commit messages, should I add it to the commit that wires the dummy message?

Next Steps

  • [ ] Adding tests for empty narrows. None of the commits currently have any new tests.
  • [ ] This introduces a bug. When you have an empty search narrow. And you receive a message event for that narrow, the message does not display until you restart the application, although it adds to the unread count. I didn't want to hold back on the reviews any longer, so I've pushed the PR and will look into this soon.

Follow-ups

  • [ ] When activating user buttons, check if there exist previous DMs with the user, and if not, just open the compose box and do not switch to that narrow.
  • [ ] When a new message arrives in an empty DM narrow, the recipient header (above the message, not the one in the recipient bar) is not inserted newly. It automatically gets fixed when one more new message arrives. I think it's not a high priority bug, since the recipient bar already matches the recipient header for the DM case.

How did you test this?

  • [x] Manually - Behavioral changes
  • [x] Manually - Visual changes
  • [x] 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

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

Niloth-p avatar Aug 05 '24 09:08 Niloth-p