-
-
Notifications
You must be signed in to change notification settings - Fork 252
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
Add moved label for topic/stream changes #1331
base: main
Are you sure you want to change the base?
Conversation
Thank you for your effort @Subhasish-Behera ! |
3e1788d
to
ae029fe
Compare
@Sushmey, Changed the commit messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this issue 👍. You have understood the issue and have tried to accommodate all conditions that distinguish MOVED
and EDITED
messages in ZT.
Just a suggestion that it would be really good if you add comments for conditionals related to (un)resolve topics for better understanding.
zulipterminal/helper.py
Outdated
RESOLVED_TOPIC_PREFIX | ||
) | ||
and edit_history_event["prev_topic"][2:] | ||
!= edit_history_event["topic"][2:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have not handled moved message case if message is edited to resolve/unresolve topic with same name.
bb107af
to
c1dc02a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Subhasish-Behera Thanks for looking into this :)
Practically speaking this looks to work OK after a quick test 👍
However, the logic is rather difficult to read in parts, partly due to reuse of long variables, and I'm not sure how much has come from server or web app code.
It also looks like you could potentially implement the update part using similar functionality to the first commit, though that may only be clear once the code is easier to read.
There are effectively 3 changes here (at least), and while the last commit says tests added, it seems like it's only updated some tests to pass. We really need tests for all the new behavior, and the messages code doesn't require any test updates, which suggests we don't have a pre-existing test either.
If you end up extracting the common logic between the two parts that I pointed out, testing that function may simplify testing the other parts.
zulipterminal/helper.py
Outdated
# We know it has a topic edit. Now we need to determine if | ||
# it was a true move or a resolve/unresolve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this logic is mirrored from a file in the server, it'd be good to link to it in a comment around here.
c1dc02a
to
d56cffc
Compare
d56cffc
to
60e4570
Compare
60e4570
to
494707f
Compare
Now, the |
Ready to review? If so, please switch the PR labels :) |
still working on changing the code to be a single function and its tests. |
8dbda5a
to
81689ec
Compare
@zulipbot add "PR needs review" |
81689ec
to
b6a8aee
Compare
b6a8aee
to
dff8de8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this @Subhasish-Behera !
I ran the code on my local machine and it worked as expected. The tests written also seem to cover all the possible cases. I manually tried testing for all the cases and it worked perfectly fine. As for the commit structure, we can discuss it on the ZT stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Subhasish-Behera I intended to only remove the needs-review label, since this needs a rebase and may have outstanding comments to address, including from Mounil here and in the stream. However, I gave this another look and left some further notes.
It's great to see the test(s) 👍 I've not read all of the main function test cases this time, and that test may become simpler if my comments result in adjustment of eg. the function signature (parameters/return/side-effects).
A point I didn't make in an inline comment, is that limiting the UI from directly accessing the model (index) data is something we should aim for. So we could add a model accessor method which returns the 'edit status' as eg. a string (a limited Literal type, perhaps), with that method being the part that interprets the edited/moved sets - and would be easier to test. We could still test the UI, but it'd be a mapping from a string<->UI, so likely less important.
Keeping that interpretation of the sets in the model may make the way in which the sets are updated, eg. whether the sets are kept disjoint, less important - the UI just gets one of 3 results via the accessor method.
I do think it's important that we resolve the current/old topic issue I mention inline, since currently it's unclear what should happen with multiple topic renames in the message history, at least with the current logic.
"EDITED", | ||
id="message_neither_index_to_edited_messages_or_moved_messages", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If neither, then why is this text "EDITED"?
case(True, False, 7, "EDITED", id="message_indexed_to_edited_messages"), | ||
case(False, True, 6, "MOVED", id="message_indexed_to_moved_messages"), | ||
case( | ||
False, | ||
False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no True, True
case here. Your code doesn't seem to limit this from occurring (considering indexing and events), and I'm not sure if you expect it to?
If we do care about moved and edited being disjoint sets, then somewhere the code needs amending. However, the logic here doesn't currently care - but we need to define that's the case.
else: | ||
edited_label_size = 0 | ||
left_padding = 8 | ||
label_text = "EDITED" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems odd that this is EDITED; I know it was a constant previously and the width determined the visibility, but it seems useful to be explicit now the MOVED case is present.
Maybe set this to ""? If so, since we'd have distinct strings in each case, can the other parameters all be defined in terms of them?
for edit_history_event in msg["edit_history"]: | ||
if "prev_content" in edit_history_event: | ||
content_changed = True | ||
if "prev_stream" in edit_history_event: | ||
stream_changed = True | ||
if "prev_topic" in edit_history_event: | ||
current_topic = edit_history_event["topic"] | ||
old_topic = edit_history_event["prev_topic"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This piece of code is not tested. I don't have an issue with setting the flags on looping through the history, but the current/old_topic data depends on the order of cycling through the history, so could be set to different values multiple times in the history. The test for analyse_edit_history isn't (yet!) in this same commit for me to easily understand if it would matter; it may also be clearer if we can simplify the logic of that function.
While ideally the index method - and so this code - could be tested, I'd feel more comfortable still without a test, if the logic of the passed-in current/old values was clearer, for example if the precise current/old values don't matter in the analyse function. Otherwise I'd question why we don't need to track/analyse all the previous topic changes.
current_topic: Any = None, | ||
old_topic: Any = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid Any
if possible.
I also don't see that you need a default parameter here - you're always passing the current and old topic values, even if they are None?
I also see that both current and old topic must be None or str together, or else the middle conditional breaks. In other similar cases we have an assert at the top of the function to ensure that 'coordinated' values are used correctly by callers. The alternative is to define the parameter as a data structure, where the entire parameter can then be None or multiple values.
msg_id: int, | ||
index: Index, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of duplicate lines in this method, index[something].add(msg_id)
, which suggests the design may be simplified.
Rather than passing in these parameters so that we can modify the Index, can we return eg. a string for each case? That means we decouple this from the index, which should simplify the test, in addition to knowing that it doesn't depend on the previous edited/moved state - or anything else in the index, which we can't say right now.
resolved_prefix = RESOLVED_TOPIC_PREFIX + " " | ||
if content_changed or stream_changed: | ||
index["edited_messages"].add(msg_id) | ||
elif old_topic: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also depends upon current_topic being non-None, which as it stands is not guaranteed. In other similar cases we have an assert at the top of the function to ensure that 'coordinated' values are used correctly. The alternative is to define the parameter as a data structure.
resolve_change = False | ||
resolved_prefix = RESOLVED_TOPIC_PREFIX + " " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These both only apply to the middle section, so move them there?
current_topic = edit_history_event["topic"] | ||
old_topic = edit_history_event["prev_topic"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that topic and prev_topic were both only added in Zulip 5.0, so we'll need a workaround for this, and any test of this part may need to depend upon the feature level - though note that prev_stream works fine since it just skips it if it is not present.
else: | ||
index["edited_messages"].add(msg_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What part of the input parameter space does this cover? Should this be an error branch instead?
The function analyse_edit_history is called from index _messages of helper.py. It decouples the edit/moved label adding mechanism from index_messages to be used later in _handle_update_messages_event.Stream/content changes comes under "edited" while topic editing checks cases related to resolve/unresolve etc which can go either way of "moved"/"edited". But moved label is applied to specific cases while rest of the cases are "edited" using else conditional(expect the resolve_change case).
Instead of EDITED label now urwid.Text takes label_text as argument which can be "EDITED" and "MOVED".
Uses the analyse_edit_history funciton from helper.py The parameters passed to analyse_edit_history is slightly diffrent from earlier as it gets the required values from event information. Tests adapted.
It tests the possible scenarios for the function as expressed in the test ids.
Tests the conditions for moved and edited label along wth UI differences. Fixes zulip#1253
dff8de8
to
9c54b8e
Compare
@Subhasish-Behera I've just rebased and pushed a version of this with the commit text summary split by a blank line (which you didn't have early on while contributing) and rebased to skip adding the symbol and import it instead - since that was the main conflict. I suspect this needs a little rework due to other changes - it didn't quite pass If you do have updates locally, then feel free to push over this version, but please do fix conflicts and compare it to the changes I just pushed first. To get this merged, I'd suggest moving the tests into the commit with the related changes, and leaving the update_message commit at the end since that's the update-during-session extension - and the one that may be failing a test now? Some of my points above were comments, but we should certainly look at the server version dependency, and ensure the tests are clear. |
Heads up @Subhasish-Behera, 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 |
What does this PR do?
Fixes #1253
Tested?
Commit flow
commit 1:
Tried to achieve parity with the logic of Zulip webapp's handling of edits
mainly from the
analyse_edit_history
function ofzulip/static/js/message_list_view.js
(web app's code doesn't use indexing like ZT though)
commit 2:
the second commit focusses on implementing the same logic in case of update_message_event. But in first commit the logic was related to the response values of
Message
but in this case its withupdate_message
event.does similar job.
In both of the commits , the code identifies a real topic change vs unresolve/resolve
Notes & Questions
Interactions