Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle internal links #708

Merged
merged 3 commits into from
Aug 29, 2020
Merged

Handle internal links #708

merged 3 commits into from
Aug 29, 2020

Conversation

preetmishra
Copy link
Member

@preetmishra preetmishra commented Jul 4, 2020 •
edited
Loading

The PR adds support for handling internal links - stream, topic and near narrow.

Commits

The commits introduce MessageLinkButton in MsgInfoView and then incrementally add support for different narrows.

I would greatly appreciate any feedback on the changes, the commit chronology and the structure in general.

Partially fixes #352 and #764.

@zulipbot zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 4, 2020
@preetmishra preetmishra force-pushed the handle-internal-links branch 3 times, most recently from 1865ad0 to 90a7016 Compare July 21, 2020 18:18
@preetmishra
Copy link
Member Author

I was meaning to change the esc focus for successful internal link narrows but I see the current behaviour aligns with esc key behaviour that we already have elsewhere (e.g. web app and zulip-term itself), that is, esc transitions to 'All messages' narrow no matter in which narrow you are in at that moment. Thoughts?

@preetmishra preetmishra marked this pull request as ready for review July 22, 2020 12:39
@preetmishra preetmishra added the PR needs review PR requires feedback to proceed label Jul 22, 2020
@preetmishra preetmishra mentioned this pull request Jul 22, 2020
Open
@neiljp neiljp added this to the Release after upcoming milestone Jul 22, 2020
@preetmishra preetmishra force-pushed the handle-internal-links branch 2 times, most recently from 7c4d7df to 3a0dade Compare July 22, 2020 19:28
sumanthvrao
sumanthvrao reviewed Jul 23, 2020
View reviewed changes
Copy link
Member

@sumanthvrao sumanthvrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@preetmishra Reviewed the first three commits and left one comment. Besides that the first 3 commits look good if we want to merge them separately.

zulipterminal/core.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Outdated Show resolved Hide resolved
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Jul 23, 2020
@preetmishra
Copy link
Member Author

@sumanthvrao Thanks for the review. As discussed in the PM, the first three commits are merged! 🎉

I have pushed a rebased version for further review.

This was referenced Jul 23, 2020
Open
Closed
@preetmishra preetmishra changed the title [WIP] Handle internal links Handle internal links Jul 29, 2020
@zulipbot
Copy link
Member

Hello @preetmishra, it seems like you have referenced #352 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 #352..

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
Copy link
Member Author

Updated to resolve conflicts.

@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Aug 5, 2020
@preetmishra
Copy link
Member Author

Updated with tests.

@preetmishra preetmishra mentioned this pull request Aug 6, 2020
Open
4 tasks
@preetmishra preetmishra force-pushed the handle-internal-links branch 2 times, most recently from ec28c38 to 084da38 Compare August 15, 2020 19:15
@preetmishra preetmishra changed the title [WIP] Handle internal links Handle internal links Aug 15, 2020
@preetmishra preetmishra added PR needs review PR requires feedback to proceed and removed has conflicts labels Aug 15, 2020
@preetmishra
Copy link
Member Author

Updated with tests and rebase onto upstream/master.

neiljp
neiljp reviewed Aug 17, 2020
View reviewed changes
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@preetmishra This looks great! It's a little verbose due to type handling and few other aspects, which is the last thing I'd like to see improve (pending reviewing of tests).

I think we can simplify the type notes sprinkled throughout using some TypedDict use, which will make the code a lot more concise and readable. There are a few other ideas inline on how we can compact the code.

Since I've given a few notes on the types and data being passed around, I've held off on the reviewing the tests for now.

zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/buttons.py Outdated Show resolved Hide resolved
@neiljp neiljp removed the PR needs review PR requires feedback to proceed label Aug 17, 2020
@preetmishra preetmishra changed the title Handle internal links [WIP] Handle internal links Aug 17, 2020
@preetmishra preetmishra force-pushed the handle-internal-links branch 2 times, most recently from 9e1946f to 18548a7 Compare August 20, 2020 13:33
@preetmishra
Copy link
Member Author

@neiljp Thanks for the suggestions. They make the code clearer and concise.

In the latest update, I have incorporated the suggestions that you left in-line and amended the tests accordingly.

@preetmishra preetmishra changed the title [WIP] Handle internal links Handle internal links Aug 20, 2020
@preetmishra preetmishra added the PR needs review PR requires feedback to proceed label Aug 20, 2020
@preetmishra
Copy link
Member Author

Updated to resolve conflicts.

@preetmishra
buttons/helper: Add support for stream narrow in MessageLinkButton.
1f873a9 
This introduces handle_narrow_link(), along with a couple of helper
methods, to handle narrow links and support stream narrow in
MessageLinkButton.

Helper methods:
* _parse_narrow_link().
* _decode_stream_data().
* _validate_narrow_link().
* _validate_and_patch_stream_data().
* _switch_narrow_to().

Additionally, this adds a helper method, hash_util_decode(), in
helper.py to decode server URL excerpts.

Tests added.
@preetmishra
buttons: Add support for topic narrow in MessageLinkButton.
73cc21e 
This extends the existing helper methods, _parse_narrow_link(),
_validate_narrow_link() and _switch_narrow_to(), to add support for
topic narrow.

Tests amended.
@preetmishra
buttons: Support near stream and near topic narrow in MessageLinkButton.
330cef9 
This extends the existing helper methods, _parse_narrow_link(),
_validate_narrow_link() and _switch_narrow_to(), and adds
_decode_message_id() to add support for near stream and near topic
narrow.

The near operator is used to set focus to a particular message (using
the anchor) while switching narrow.

Tests amended.
@preetmishra
Copy link
Member Author

Updated to resolve conflicts.

@neiljp
Copy link
Collaborator

neiljp commented Aug 29, 2020

@preetmishra Thanks for working on this - this is another feature that will make ZT easier to use for jumping between streams/topics 👍 This took a while to review due to the size, and there are a few refactors we can do to improve this, but I'm going to merge this now so we can get it out for wider view 🎉

@neiljp neiljp merged commit 6ccac96 into zulip:master Aug 29, 2020
@preetmishra preetmishra deleted the handle-internal-links branch August 29, 2020 05:19
@Ezio-Sarthak Ezio-Sarthak mentioned this pull request Jan 21, 2021
Open
@zee-bit zee-bit mentioned this pull request Mar 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp neiljp left review comments

@sumanthvrao sumanthvrao sumanthvrao left review comments

Assignees
No one assigned
Labels
PR needs review PR requires feedback to proceed size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.

Cross reference internal links to their respective narrows within ZT
4 participants
@preetmishra @zulipbot @neiljp @sumanthvrao