Refresh topic list view on update message events #785
Conversation
There was a problem hiding this comment.
@kaustubh-nair Thanks for looking into this - this will fix a bug, though I couldn't find an issue for it.
zulipterminal/ui_tools/buttons.py
Outdated
| topic_view, | ||
| self.view.left_panel.options(height_type="weight") | ||
| ) | ||
| self.view.left_panel.show_topics_view(self) |
There was a problem hiding this comment.
This appears to be the only current place where this code occurs, and while I know this is a refactoring aimed purely at the following commit, it would be useful to combine the extraction/encapsulation of this method with related ones. That could be anywhere we update the contents, which is anywhere near is_in_topic_view (which we could move to a renamed property, perhaps)
There was a problem hiding this comment.
I've done this by making two methods, one each for updating topics view and stream view, and making minor tweaks in update_stream_view (previously update_structure). I hope this covers everything.
zulipterminal/model.py
Outdated
| for msg_id in event['message_ids']: | ||
| self.index['messages'][msg_id]['subject'] = new_subject | ||
| self._update_rendered_view(msg_id) | ||
| if stream_id in self.index['topics']: | ||
| # Reload topic list. | ||
| self._fetch_topics_in_streams([stream_id]) |
There was a problem hiding this comment.
While this seems reasonable as a v0, don't these events allow us to reorder what we already have instead?
There was a problem hiding this comment.
@neiljp No, as I mentioned on czo, due to the topic data in the index never being complete, there is no way of knowing when a topic is empty (i.e doesn't have any messages left).
It would be useful if the backend response could include an extra flag when this happens, though it's not present currently AFAIK.
There was a problem hiding this comment.
Let's discuss on #zulip-terminal.
|
@kaustubh-nair I think we've clarified via czo that it's OK to use the topic fetch approach, though as I mentioned elsewhere it'd be useful if we didn't fetch it unless/until we actively need it (for the subsequent conditional, or possibly another state we may be in?). For example, if we were in a stream narrow and triggered a topic rename and not showing the topic list, we could wait until autocompleting topics or showing the topic narrow before fetching it? You didn't respond regarding the first point for the refactoring, but we can chat on czo regarding this if you like. |
kaustubh-nair
force-pushed
the
topic_data_update_v2
branch
from
a8a60a9
to
64bfd61
Compare
zulipbot
added
size: L
kaustubh-nair
force-pushed
the
topic_data_update_v2
branch
from
64bfd61
to
8833583
Compare
zulipbot
added
size: XL
kaustubh-nair
force-pushed
the
topic_data_update_v2
branch
from
8833583
to
ffe3df2
Compare
kaustubh-nair
force-pushed
the
topic_data_update_v2
branch
from
ffe3df2
to
d9e63ba
Compare
|
@neiljp Thanks for the review! I have made the topic fetch conditional, resetting the cache when not needed. I've also incorporated the refactor you mentioned (and a few others), if something has been left out in that let me know. Tests fixed and added. |
|
@kaustubh-nair As per comment in the stream just now, I've just merged most of the early commits with minor text/layout changes. The topic is not being updated in some cases, so I'd like to debug that before merging. |
|
Heads up @kaustubh-nair, we just merged some commits that conflict with the changes your 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 |
kaustubh-nair
force-pushed
the
topic_data_update_v2
branch
from
d9e63ba
to
c136471
Compare
This updates the topic list when it is open, else resets topic cache if present. Tests amended.
kaustubh-nair
force-pushed
the
topic_data_update_v2
branch
from
c136471
to
1cbaef1
Compare
No description provided.