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

Update topic and stream headers on update message events mid-compose.

Open prah23 opened this issue 3 years ago • 3 comments

PR structure:

  • tests: Generalize the update message index using factory. This commit adds a factory to generate the messages index relevant to update message event tests, in an effort to reduce code duplications for future tests. The factory requires three parameters - msg_stream_id, (stream_id of the messages in the index), topics (dict containing streams as keys, and a list of topics as the values, consistent with the structure in the index) and message_ids (which is a list of the message ids to be generated in the index). The factory uses these parameters to customize the index.
  • boxes: Create helper function to update the topic compose header. This commit adds a helper function facilitating abstracted updates to the topic in the topic compose header (title_write_box), and also sets the edit_pos appropriately. Test added.
  • api_types: Extend UpdateMessageEvent to accomodate topic updates. This commit includes more fields under the UpdateMessageEvent class to ensure type-consistent handling for said fields in the update message events conveying topic updates.
  • model: Handle topic updates in the compose header mid-compose. This commit enables dynamic behaviour in the compose box when a topic is updated when the user is currently composing. The topic that the user is currently writing a message to is checked and the relevant update is done if the topic in the event matches the corresponding header. Tests added.
  • boxes: Create helper function to update the stream compose header. This commit adds a helper function facilitating abstracted updates to the stream name in the stream compose header (title_write_box), the corresponding edit_pos modification, as well as the stream_id attribute of the WriteBox object in use. Test added.
  • api_types: Extend UpdateMessageEvent to handle cross-stream updates. This commit extends the UpdateMessageEvent class by including the new_stream_id field to ensure type-consistent handling of cross-stream updates (moving topics between streams) in the update message events.
  • model: Handle stream updates in the compose header mid-compose. This commit updates the compose header when a stream is updated while the user is currently composing. The stream that the user is currently writing a message to is checked and the relevant update is done if the stream_id in the event matches the corresponding header. Tests added.

Testing and linting: I've ran tests locally on each commit. I've also ran black checks and then the more extensive ./tools/lint-all.

Fixes #1040.

prah23 avatar Jun 02 '21 15:06 prah23

Please hold on the stream headers update aspect of this PR for now, the topic header update is ready.

prah23 avatar Jul 05 '21 15:07 prah23

Heads up @prah23, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot avatar Feb 24 '23 21:02 zulipbot

@prah23 Heya - remember this PR? It's pretty useful and it'd be great to get at least partially merged :)

I remembered this PR was still open and have just rebased it such that in principle it passes tests & linting - I've not pushed it here, as I wanted to check with you first to make sure you have a version available for comparison if you want?

The changes required were:

  • adjusting the test factory function; I think it may have been a PR/commit of yours that added an additional model parameter. Provisionally I added this to the parameters, but as per my previous comment on the PR this may be able to be simplified.
  • added test function types; again, that was your doing in another PR, but its easy enough to change :)
  • one event type change is dropped, and the second for stream changes is still there but may be a small change incorporated into another commit if #1401 merges first

We have tools/check-branch now, which is pretty useful for fixing these kinds of changes :) (as well as checking - ensuring commits pass individually in CI)

I've only tested this for topic moves (I'm not sure if the stream part was complete) and this works as before from what I remember :+1:

Outstanding points are:

  • this is not necessarily related to this issue, but we have a bug which relates to if the topic list is updated then we get an error relating to the search_lock overwriting the screen; that doesn't block the topic update, but it messes up the UI and it's difficult to tell what's happening :/
  • if there is only one message in the topic and only it changes name, should this change a response, do you think?
  • we now have a single draft saved; I've not checked with the web app for comparison, but I wonder if we should update any draft topics based on events too?
  • do you remember the status of the stream updating part?

I'll do a deeper review before moving on or merging (partial or otherwise) in any case.

neiljp avatar May 17 '23 02:05 neiljp