Add support for opening message in browser. #991
Conversation
zulipbot
added
size: L
zee-bit
force-pushed
the
view-in-browser
branch
from
5fde88b
to
4e386f4
Compare
zee-bit
changed the title
There was a problem hiding this comment.
@zee-bit Thanks for continuing the work. I have tested this locally with every type of narrow and this seems to work well! 👍
For the first iteration, I have left a few quick in-line comments (except for the last two commits). Of course, we would need to flesh out the security concerns prior to the merge.
While the last two commits extend the capability in a subtle way, we should perhaps introduce them post "Handle media links" PR as to an end-user it might not be apparent why some links (media links) don't open in the web browser while others do. This won't be an issue once we have the ability to handle media links in ZT itself.
zee-bit
force-pushed
the
view-in-browser
branch
from
4e386f4
to
eba298c
Compare
|
@zee-bit Hey, is this ready for another review iteration? |
preetmishra
added
the
PR awaiting update
|
@preetmishra Yes, I have already updated the PR according to your review, so it should be ready for the next iteration. :) |
zulipbot
added
PR needs review
There was a problem hiding this comment.
@zee-bit Thanks for making the amendments! I have tested this locally (except the last two commits, see my last point in the previous review) and it seems to well. The messages open up in the expected near narrow for PMs and streams. 👍
@neiljp The first four commits look good to me! We could have them in if everything looks good to you as well.
zee-bit
force-pushed
the
view-in-browser
branch
2 times, most recently
from
5958586
to
d94e89a
Compare
There was a problem hiding this comment.
@zee-bit Thanks for pushing this feature forward again and @preetmishra for reviewing recently. The core features are looking good.
However, reading this there's a lot of intermediate refactoring of the new code added in this PR, which indicates the commits might be more "as explored" rather than as cleanly laid out with what we know what we want and can achieve - the tests are moved around quite a bit too. I'm not sure if more of the older commits were kept as they were for consistency, but you can always add Co-authored-by: <author> to the commit text to share authorship if things work better when split up, which you may want to do if you significantly add to (author) a commit in any case as the 'commit name' might be replaced by whoever merges the commits eventually. Of course if you completely rewrite, that's all you :)
I'm not completely opposed to the current structure and happy to discuss in the stream, but I'd suggest something like the following might be clearer (possibly with intermediate commits which we can squash together later after discussion):
- Add output suppression functionality (same commit, by Puneeth)
- Add open url in browser feature in near-final form, using output suppression & UI error handling (part of 2nd, adapted as per 5th?)
- Add key+UI for opening message (current first commit + part of second + easy update using our library from the 4th)
- General url opening implementation (to be discussed)
For sanity we could add an assert and/or tests to the url-opening method to ensure it's from the server, before we add general link opening, but it depends how we expect to proceed with general links - was the idea to maybe have a prompt like with media?
neiljp
added
PR awaiting update
zee-bit
added
PR needs review
There was a problem hiding this comment.
@zee-bit This is super-close, just left a few small points 👍
Maybe we'll squash the 3rd and 4th commit, and if we're planning to drop the last commit into another PR we might want to do that in this next (hopefully final!) iteration.
I'm pretty excited at the upcoming rework of the last commit too - with the work on topic links too we'll be able to open linkified PRs/issues in the browser much more easily 🎉
neiljp
added
PR awaiting update
zee-bit
force-pushed
the
view-in-browser
branch
2 times, most recently
from
150e3f0
to
55708de
Compare
zee-bit
added
PR needs review
|
Removed the last commit from here, will be introducing it in a new PR as requested. :) |
This commit creates a context-manager that temporarily redirects all standard output and error to /dev/null so that the console does not log those in the corresponding log files or current terminal window.
This adds open_in_browser() to add support for opening any link in the web browser. This can later be used to either view any message or open an external link in the browser. The open_in_browser() accepts a URL and uses webbrowser module to open that in browser. If no runnable browser is found in the system running ZT, then an appropriate error message will be displayed in the footer. Test added. Co-authored-by: Puneeth Chaganti <[email protected]> Co-authored-by: Preet Mishra <[email protected]> Co-authored-by: Zeeshan <[email protected]>
This commit connects this functionality with a new VIEW_IN_BROWSER key
('v'). Any message can now be viewed in the browser by pressing the key
from the MessageInfo view.
Regenerated hotkeys.md.
Test added.
Co-authored-by: Puneeth Chaganti <[email protected]>
Co-authored-by: Preet Mishra <[email protected]>
|
@zee-bit Thanks for the adjustments; I did that squash, adjusted some commit text and simplified the tests slightly and am merging now! 🎉 Thanks for @punchagan and @preetmishra for starting this and keeping this going too! |
neiljp
changed the title
Thanks to @punchagan and @preetmishra for their major initial work in #397 and #698 respectively. 👍
This PR extracts some initial commits from both PR's and cleans and refactors them. It also adds support for opening external links in the browser from the
MessageLinkButton. All the reviews on the previous PR's have also been addressed, facilitating further reviews.Fixes last point of #452.