Add support for edit history #663

preetmishra · 2020-05-17T17:34:01Z

This PR builds up on @shreyamalviya's #354 to show edit history for a message. 👍

The popup can be triggered by pressing d from the MsgInfoView for any message (we could discuss other key options and whether it would be better to show the popup only for messages that are edited).

For v1, the popup only shows the content for every snapshot in the 'message_history' (not the diff).

I have split the last two commits for making it easier to review. We could squash the two if necessary.

Fixes #135 partially.

zulipbot · 2020-05-17T17:34:06Z

Hello @preetmishra, it seems like you have referenced #135 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #135..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

Thank you for your contributions to Zulip!

preetmishra · 2020-05-17T17:35:27Z

I'll be adding the tests once we reach a consensus on the design and the implementation details.

preetmishra · 2020-05-17T17:45:42Z

Updated with an improved theme for retro mode.

sumanthvrao

@preetmishra This looks like a great start! 👍

Code-wise I went through only the first couple of commits in detail. From the usage perspective, the edit history theme is consistent with the message theme and looks nice. I'm glad you managed to keep some of Shreya's previous contributions in the PR and tweaked some of those.

Noticed a bugdown when testing, --> I was editing a message (to self) in ZT and then tried to view the history (i followed by d), however, I get a 'no history to view' message. This also makes me wonder if we want to show the Press d to view in message info for all the messages (even those which have not been edited). Might be simpler to just say No Edit History available for such messages.

I tried a double edit and the result looked a little different than what I expected. Check my test-svr-edit-history topic in #test-here for reference.

Just noticed that we show HH:MM:SS (seconds as well) is the edit time for a message. Maybe we can discuss this on ZT stream if seconds is also needed. Personally I don't mind it but it does look a little odd for the normal messages to have their seconds truncated while edit history contains it.

I understand, as you mentioned previously, that the diffs would be a V2. One more future thing to consider is if we want to display details like Posted by XYZ and Edited by XYZ. This information in the web-app actually caught my eye so I thought of mentioning it.

The Current Version and Original Version tags in the edit-view is a nice touch! :)

zulipterminal/core.py

preetmishra · 2020-05-21T16:10:39Z

@sumanthvrao Thanks for the review.

I have fixed the bug that you reported; you should be able to the history as soon as you make an edit (it was an issue with edit_history). In addition to that, I have added the prefixes like the web app (e.g Edited by XYZ).

Re double edit, I am not sure what do you want to suggest by 'different'. Could you elaborate a bit?

I think that precise timestamps would be useful for distinguishing messages accurately (consider a message that has been edited thrice within a minute).

We could discuss whether it would be better to not show EditHistoryView for messages that have not been edited. It would be straightforward to implement with edit_history but edit_history isn't reliable at the moment (see #issues>edit history isn't dynamic).

Also, edit_history is likely to be changed to a boolean for checking if the message has been edited or not (see #backend>edit_history), so the issue might get fixed along the way.

sumanthvrao · 2020-05-22T01:13:16Z

@preetmishra Tried the updated PR, it's a nice improvement 👌 . The edit-from-zt bug is gone now.

By double edit I mean you edit a message twice and the second edit wasn't seen. But I tried this after your rebase and I'm no longer able to reproduce it. I think it was coupled with the edit-from-ZT bug which you fixed.

Re EdithistoryView for non-edited, Sure we can start a discussion on the stream for this. Ah! boolean value would simplify some logic we have. Nice!

I think you can proceed to add tests for the PR.

preetmishra · 2020-05-24T17:35:03Z

@sumanthvrao Thanks for taking another look. I am glad that the 'double edit' and 'edit-from-zt' bug are sorted for you. 👍

I updated the PR with the tests. It covers every aspect except the ones that related to bulid_pile in EditHistoryView. I am not sure with the level of detail that I should cover. Could you share some pointers on how to go about it?

sumanthvrao

@preetmishra I left a quick comment about organizing your commit (especially the second one). I haven't done a comprehensive review yet so this quick feedback.

In the general unit testing pattern it's okay to test the function x() in terms of its functionality alone and not its associated function y(). In fact, it is fairly common to stub y() with expected output and let another unit test for function y() test that logic. In this case, I feel you have covered the EditHistoryView functionality and need not cover the underlying build_pile.

README.md

preetmishra · 2020-05-27T16:14:53Z

@sumanthvrao Thanks for the clarification and the suggestion. 👍

I have split the second commit as per your suggestion in #663 (comment). In addition to that, I have split the last commit into three separate commits for clarity.

Hopefully, this should be easier to review and comprehend.

tests/conftest.py

preetmishra · 2020-05-27T16:25:13Z

zulipterminal/ui_tools/views.py

+        self.msg = msg
+        self.width = 64
+        self.height = 0
+        super().__init__(controller, [], 'MSG_INFO')


The MSG_INFO makes me wonder if it'd be a better idea to rename the self.command, in PopUpView, variable to something like self.exit_popup_command?

sumanthvrao

@preetmishra Thanks for splitting out the commits, they have made things easier to review and readable. I had a chance to look at the code, this time, and have left some comments/suggestions in various places.

I did manual testing (stuck to gruvbox) to a certain extent and wasn't able to catch any other bugs, which is good! Feel free to reach out in case any comment is unclear.

README.md

zulipterminal/config/keys.py

sumanthvrao · 2020-05-30T14:28:58Z

zulipterminal/ui_tools/views.py

@@ -904,6 +904,9 @@ def __init__(self, controller: Any, msg: Message) -> None:
            ('Date & Time', time.ctime(msg['timestamp'])[:-5]),
            ('Sender', msg['sender_full_name']),
            ('Sender\'s Email ID', msg['sender_email']),
+            ('Edit History',


'Press ' + ', '.join(keys_for_command('EDIT_HISTORY')) + ' to view'),

Might be simpler.

Yes, however, the current implementation adds quotes ('') to the keys that are returned from keys_for_command. The quotes make it easier to differentiate the keys from the rest of the text.

tests/ui/test_ui_tools.py

zulipterminal/model.py

zulipterminal/ui_tools/views.py

preetmishra · 2020-06-07T16:58:53Z

@sumanthvrao Thanks for the review and the suggestions.

I have:

added the test for build_pile,
removed the redundant 'Topic Updated' excerpt from the author prefix to stick with Edited by X,
replaced the O(n) logic, in get_author, with your proposed method and
made other amendments as per your suggestions.

sumanthvrao

@preetmishra Thanks for sticking with this and implementing the changes. I feel this is close to getting merged. Had a look at the changes incorporated post my previous review and they look good.

@neiljp FYI :)

zulipterminal/model.py

neiljp · 2020-06-11T23:47:23Z

@preetmishra This is my first pass at looking at manually testing this after your updates, and it seems pretty good 👍

Thoughts from looking at a few messages with histories:

If there is the original and updated topics in every case, it seems unnecessary to have the '(updated from )', and it actually looks a bit confusing if the topics have parentheses too!
To emphasize what changed in each version, how about having 'Content'/'Topic'/'Content & Topic' before 'edited by '?
From the edit menu, e and esc behave the same (back to info menu), but i exits both levels of menu?

preetmishra · 2020-06-14T17:40:49Z

@neiljp Thanks for the review and the suggestions. I have removed the (updated from {}) bit and extended the author prefix to show 'Content'/'Topic'/'Content & Topic' before 'edited by' or 'Edited but no changes made' accordingly.

In addition to that, I have restructured the 4th commit (see in-line comment), extended the commit messages and tidied up the code for clarity.

Re keys, it's been taken from the original PR so I do not have strong opinions. I am not sure about the other keys but entering and exiting with EDIT_HISTORY key feels intuitive.

Note: This update renames build_pile to edit_block (this might come handy in understanding previous discussions where we refer to it as build_pile).

zulipterminal/ui_tools/views.py

neiljp

@preetmishra I had a few more responses to give, but I lost power so wanted to get this feedback to you promptly rather than figuring out what I missed! This is looking tidier 👍

zulipterminal/model.py

zulipterminal/config/keys.py

tests/ui_tools/test_popups.py

neiljp · 2020-08-17T17:33:03Z

zulipterminal/config/themes.py

+        ('edit_author',    'yellow',          'black',
+         None,             YELLOW,            BLACK),
+        ('edit_tag',       'light blue',      'black',
+         None,             LIGHTBLUE,         BLACK),


I see you adjusted the theme slightly - this fits better with gruvbox existing colors?

Yes, it seems to personally. I have also adjusted the edit_tag style in other themes.

Generally the colors look fine, over the themes 👍 My only concern - which we can address later, since it's a minor layout/color issue - is that the 'version' (tag) appears in a date-like style, and the date in a bar/topic-like style. I don't know if we might want to swap them around in future. It's totally readable as it is, but since the styles are so similar then that one difference stands out more.

Agreed. In the latest update, I have swapped tag with time and slightly adjusted the themes to improve this.

zulipterminal/ui_tools/views.py

tests/ui/test_ui_tools.py

zulipterminal/ui_tools/views.py

preetmishra · 2020-08-18T16:29:59Z

@neiljp Thanks for the feedback.

In the latest update, I have shelved the extra get_* method, simplified the patch that you mentioned and improved the Date & Time label behaviour. I have also replied to your other in-line comments.

We could discuss the potential refactors in the stream and handle them separately.

neiljp

@preetmishra Nice update 👍

One comment I missed in the last review was suggesting the 'link' commit occur after the commit which fleshes out the edit history view, so that we don't end up with an empty box for the history before that commit - what do you think?

I'm not sure if it's strictly in O(1), but it's definitely better :)

tests/model/test_model.py

neiljp · 2020-08-18T20:04:13Z

zulipterminal/config/themes.py

+        ('edit_author',    'yellow',          'black',
+         None,             YELLOW,            BLACK),
+        ('edit_tag',       'light blue',      'black',
+         None,             LIGHTBLUE,         BLACK),


Generally the colors look fine, over the themes 👍 My only concern - which we can address later, since it's a minor layout/color issue - is that the 'version' (tag) appears in a date-like style, and the date in a bar/topic-like style. I don't know if we might want to swap them around in future. It's totally readable as it is, but since the styles are so similar then that one difference stands out more.

tests/ui_tools/test_popups.py

tests/ui/test_ui_tools.py

zulipterminal/ui_tools/views.py

preetmishra · 2020-08-20T20:29:29Z

@neiljp Thanks for the review. 👍

In the latest update, I have:

Incorporated the suggested commit order.
Separated the fetch_message_history test into *_success and *_error.
Added a comment re why we just need the message ID in TestEditHistoryView.
Parametrized the edit history keypress test over index and initial_data.
Swapped tag with time in the EditHistoryView to improve consistency between the timestamps.

Tests added with message_history fixture in conftest.py.

Note: The EDIT_HISTORY key(s) is(are) supposed to only work from MsgInfoView.

This lays the groundwork for viewing edit history from MsgInfoView. Note: EditHistoryView is linked with MsgInfoView in a subsequent commit. Rebased and tests added (into a new tests/ui_tools/test_popups.py) by Preet Mishra. Co-authored-by: Preet Mishra <[email protected]>

This uses get_message_history() in EditHistoryView to fetch and show edit history from MsgInfoView. The following information is displayed using _make_edit_block(): * topic * timestamp * tag (for current and original message) * content Tests added.

This links EditHistoryView with MsgInfoView using the keypress method and adds an 'Edit History' label in MsgInfoView. The label is only shown for edited messages. Tests amended and added for the keypress.

This would be used as a helper to get the author name while rendering message edit history later. Tests added. Lookup-suggested-by: Sumanth V Rao <[email protected]>

This extends _make_edit_block() to show the author name and adds _get_author_prefix() to increase its verbosity by indicating the changes that were made in a particular version (e.g. Content, Topic, Content & Topic). The implementation has a workaround to spot a false alarm for content or topic change (A NOTE comment has been added) and an author prefix, 'Edited but no changes made', for edits that do not have any 'real' changes. Test amended and added.

preetmishra · 2020-08-30T06:57:46Z

Updated to resolve conflicts.

neiljp · 2020-08-30T17:35:50Z

@preetmishra Thanks for persisting with this and building upon the work of @shreyamalviya 👍 I've just merged this after a very minor commit text change 🎉

This is another good feature to have in ZT and with some good subtleties to the UI, and taking into account some slightly unexpected responses from the server 👍

zulipbot added the size: XL [Automatic label added by zulipbot] label May 17, 2020

preetmishra force-pushed the edit-history branch 2 times, most recently from bf33001 to d0f4760 Compare May 17, 2020 17:44

sumanthvrao added the PR needs review PR requires feedback to proceed label May 19, 2020

sumanthvrao reviewed May 20, 2020

View reviewed changes

zulipterminal/core.py Outdated Show resolved Hide resolved

sumanthvrao removed the PR needs review PR requires feedback to proceed label May 20, 2020

preetmishra force-pushed the edit-history branch 2 times, most recently from bb43dab to 708f2c6 Compare May 21, 2020 15:34

preetmishra force-pushed the edit-history branch from 708f2c6 to ae91895 Compare May 24, 2020 17:01

sumanthvrao reviewed May 27, 2020

View reviewed changes

README.md Show resolved Hide resolved

preetmishra force-pushed the edit-history branch from ae91895 to 6bfce8e Compare May 27, 2020 16:03

preetmishra commented May 27, 2020

View reviewed changes

tests/conftest.py Show resolved Hide resolved

preetmishra commented May 27, 2020

View reviewed changes

sumanthvrao reviewed May 31, 2020

View reviewed changes

preetmishra force-pushed the edit-history branch 2 times, most recently from 3bb47ca to ddf924c Compare June 7, 2020 16:14

sumanthvrao added the PR needs review PR requires feedback to proceed label Jun 9, 2020

sumanthvrao approved these changes Jun 10, 2020

View reviewed changes

zulipterminal/model.py Outdated Show resolved Hide resolved

preetmishra force-pushed the edit-history branch from ddf924c to 37b637c Compare June 14, 2020 17:14

preetmishra commented Jun 14, 2020

View reviewed changes

zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved

preetmishra added the PR needs review PR requires feedback to proceed label Aug 17, 2020

preetmishra changed the title ~~[WIP] Add support for edit history~~ Add support for edit history Aug 17, 2020

neiljp reviewed Aug 17, 2020

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Aug 17, 2020

preetmishra force-pushed the edit-history branch from 3eee95b to dda0ce4 Compare August 18, 2020 15:49

preetmishra added the PR needs review PR requires feedback to proceed label Aug 18, 2020

neiljp reviewed Aug 20, 2020

View reviewed changes

preetmishra removed the PR needs review PR requires feedback to proceed label Aug 20, 2020

preetmishra changed the title ~~Add support for edit history~~ [WIP] Add support for edit history Aug 20, 2020

preetmishra force-pushed the edit-history branch from dda0ce4 to 18e5dff Compare August 20, 2020 20:22

preetmishra added the PR needs review PR requires feedback to proceed label Aug 20, 2020

preetmishra changed the title ~~[WIP] Add support for edit history~~ Add support for edit history Aug 20, 2020

preetmishra and others added 9 commits August 30, 2020 12:10

conftest/model: Add fetch_message_history() for message edit history.

2746c20

Tests added with message_history fixture in conftest.py.

themes: Add themes for edit history pop-up.

4dc00d7

keys/README: Add EDIT_HISTORY key to view edit history.

817060a

Note: The EDIT_HISTORY key(s) is(are) supposed to only work from MsgInfoView.

views: Link EditHistoryView with MsgInfoView to view edit history.

b8bad18

This links EditHistoryView with MsgInfoView using the keypress method and adds an 'Edit History' label in MsgInfoView. The label is only shown for edited messages. Tests amended and added for the keypress.

model: Introduce user_name_from_id() to access name in constant time.

4896959

This would be used as a helper to get the author name while rendering message edit history later. Tests added. Lookup-suggested-by: Sumanth V Rao <[email protected]>

views: Mark the 'Date & Time' label original for edited messages.

f5c5484

preetmishra force-pushed the edit-history branch from 18e5dff to f5c5484 Compare August 30, 2020 06:52

neiljp closed this Aug 30, 2020

neiljp mentioned this pull request Aug 30, 2020

Displays message info on pressing i, edit history on pressing e #354

Closed

preetmishra deleted the edit-history branch August 30, 2020 18:53

preetmishra mentioned this pull request Aug 30, 2020

Show message info on pressing i. #135

Closed

3 tasks

Uh oh!

Add support for edit history #663

Add support for edit history #663

Uh oh!

Conversation

preetmishra commented May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zulipbot commented May 17, 2020

Uh oh!

preetmishra commented May 17, 2020

Uh oh!

preetmishra commented May 17, 2020

Uh oh!

sumanthvrao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

preetmishra commented May 21, 2020

Uh oh!

sumanthvrao commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

preetmishra commented May 24, 2020

Uh oh!

sumanthvrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

preetmishra commented May 27, 2020

Uh oh!

Uh oh!

preetmishra May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumanthvrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sumanthvrao May 30, 2020

Choose a reason for hiding this comment

Uh oh!

preetmishra Jun 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

preetmishra commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sumanthvrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neiljp commented Jun 11, 2020

Uh oh!

preetmishra commented Jun 14, 2020

Uh oh!

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neiljp Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

preetmishra Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

preetmishra commented May 17, 2020 •

edited

Loading

sumanthvrao left a comment •

edited

Loading

sumanthvrao commented May 22, 2020 •

edited

Loading

preetmishra May 27, 2020 •

edited

Loading

preetmishra commented Jun 7, 2020 •

edited

Loading

preetmishra commented Aug 18, 2020 •

edited

Loading

preetmishra commented Aug 20, 2020 •

edited

Loading

preetmishra commented Aug 30, 2020 •

edited

Loading