[WIP] Add support for viewing a message in the web browser #698

preetmishra · 2020-06-19T18:59:09Z

Thanks to @punchagan for the initial work in #397. 👍

Unlike the original PR, which narrowed every message to All Messages in the web app, this opens a message in stream/topic/near if it belongs to a stream or pm-with/near narrow if it belongs to a PM/huddle (see discussion).

The original commit has been split to facilitate reviews. Moreover, the test has been improved based on @neiljp's feedback on the original PR.

The logic for generating near message URLs has been borrowed from Zulip's zerver/lib/url_encoding.py.

Partially fixes #452.

neiljp · 2020-06-19T23:34:33Z

@preetmishra We discussed this on czo, but let me know if you need more specific feedback.

preetmishra · 2020-06-22T19:02:23Z

@neiljp I have updated the PR based on your suggestions on CZO. 👍

I have split the commit from the original PR into three, without any alternations, and added my changes into separate commits.

I will proceed to work on other enhancements that we discussed once the commit structure looks good to you.

zulipbot · 2020-07-30T10:14:09Z

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

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-07-30T10:17:28Z

Updated to resolve conflicts.

preetmishra · 2020-07-31T14:39:33Z

Updated to resolve conflicts.

preetmishra · 2020-08-30T07:22:23Z

Updated to resolve conflicts.

neiljp

@preetmishra Thanks for continuing to build on @punchagan 's work 👍

I've left come feedback inline, but we'll definitely want to do a solid review before final merge to ensure our URL opening is secure.

neiljp · 2020-08-09T15:52:35Z

zulipterminal/core.py

+                and not os.environ.get('DISPLAY') and os.environ.get('TERM')):
+            # Don't try to open web browser if running without a GUI
+            return
+        webbrowser.open(url)


This can raise an exception?

neiljp · 2020-08-09T15:53:13Z

zulipterminal/ui_tools/boxes.py

@@ -978,6 +978,8 @@ def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
        elif is_command_key('MSG_INFO', key):
            self.model.controller.show_msg_info(self.message,
                                                self.message_links)
+        elif is_command_key('VIEW_IN_BROWSER', key):
+            self.model.controller.view_in_browser(self.message['id'])


If this fails (exception, unsupported environment, etc), could we let the user know?

neiljp · 2020-08-09T15:57:24Z

zulipterminal/core.py

+        if (not MACOS and not WSL and not os.environ.get('DISPLAY')
+                and os.environ.get('TERM')):


Our platform code relies on only one of the platforms being set; we definitely want to improve that handling (see #791), but we should probably be able to rely on the 'other' option being set for now, ie LINUX.

In the more general sense, the webbrowser module can run terminal browsers, so it would be interesting to see how DISPLAY-less systems handle that. Another aspect is whether graphical non-X11 systems set DISPLAY meaningfully?

Did you mean to capitalize the commit title?

neiljp · 2020-08-31T18:37:30Z

zulipterminal/helper.py

@@ -666,3 +667,63 @@ def suppress_output() -> Iterator[None]:
    finally:
        os.dup2(out, 1)
        os.dup2(err, 2)
+
+
+def hash_util_encode(string: str) -> str:


Let's use a different file for these, since we could combine these with the URL deconstruction too and/or put them into the API repo like that other method in future?

neiljp · 2020-08-31T18:52:15Z

zulipterminal/core.py

+        # Truncate extra '/' from the server url.
+        self.url = near_message_url(self.model.server_url[:-1], message)


It's good to use the pre-existing code here, though using urljoin in the new code would mean we don't need to worry about this. I'm guessing the server code avoids this since it knows the url exactly and relies on tests, which we should too really :)

neiljp · 2020-08-31T18:52:46Z

zulipterminal/core.py

        if (not MACOS and not WSL and not os.environ.get('DISPLAY')
                and os.environ.get('TERM')):
            # Don't try to open web browser if running without a GUI
            return
        with suppress_output():
            # Suppress anything on stdout or stderr when opening the browser
-            webbrowser.open(url)
+            webbrowser.open(self.url)


Why do we need the self?

neiljp · 2020-08-31T18:53:24Z

zulipterminal/core.py

@@ -298,15 +300,16 @@ def show_all_mentions(self, button: Any) -> None:
                        anchor=None,
                        mentioned=True)

-    def view_in_browser(self, message_id: int) -> None:
-        url = '{}#narrow/near/{}'.format(self.model.server_url, message_id)
+    def view_in_browser(self, message: Message) -> None:


This change breaks the other use of this method - but no tests break?

zulipbot · 2021-01-30T20:34:38Z

Heads up @preetmishra, 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 upstream/main branch and resolve your pull request's merge conflicts accordingly.

zee-bit · 2021-03-24T22:07:36Z

@preetmishra We recently merged #854, which centralizes the logic of generating near_message_url() to a new module server_url.py(along with the tests in a corresponding test_* file). This should simplify this PR to a great extent. :)

(Let me know if you are busy and want me to take this further)

preetmishra · 2021-03-31T03:58:30Z

@zee-bit, thanks for the update! I will update the PR soon.

This adds view_in_browser to add support for viewing any message in the web browser. For now, the message is always opened in the 'All messages' narrow. A message can be viewed in the browser by pressing VIEW_IN_BROWSER key(s) directly. Test added.

This makes MsgInfoView handle VIEW_IN_BROWSER keypress in order to avoid too many/unintended triggers. Tests amended and added.

This improves view_in_browser() to open messages in the exact/topic narrow for streams and in the pm-with narrow for PMs and huddles. Tests amended.

preetmishra · 2021-04-04T09:04:47Z

@zee-bit Hey! I am a bit busy with work. However, I rebased my local branch onto master. Feel free to take it further. Best of luck!

neiljp · 2021-06-11T01:07:52Z

@preetmishra As you know this was replaced by the now-merged #991 so I'm closing this 🎉

zulipbot added the size: XL [Automatic label added by zulipbot] label Jun 19, 2020

preetmishra force-pushed the view-in-browser branch from 7efdafd to 6657f4d Compare June 22, 2020 18:46

preetmishra changed the title ~~Add support for viewing a messsage in the web browser~~ Add support for viewing a message in the web browser Jun 23, 2020

neiljp added the PR needs review PR requires feedback to proceed label Jun 23, 2020

preetmishra force-pushed the view-in-browser branch from 6657f4d to 5316996 Compare July 30, 2020 10:14

preetmishra force-pushed the view-in-browser branch from 5316996 to aa51e10 Compare July 31, 2020 14:38

preetmishra marked this pull request as ready for review August 6, 2020 18:07

neiljp added the PR blocks other PR label Aug 8, 2020

neiljp mentioned this pull request Aug 9, 2020

TRACKING: Message rendering bugs/enhancements #452

Open

12 tasks

neiljp added this to the Next Release milestone Aug 16, 2020

preetmishra force-pushed the view-in-browser branch from aa51e10 to 3912780 Compare August 30, 2020 07:21

neiljp reviewed Aug 31, 2020

View reviewed changes

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

preetmishra changed the title ~~Add support for viewing a message in the web browser~~ [WIP] Add support for viewing a message in the web browser Sep 2, 2020

neiljp mentioned this pull request Nov 23, 2020

Opens message links popup menu #424

Closed

neiljp modified the milestones: 0.6.0, Release after next Jan 28, 2021

neiljp mentioned this pull request Jan 28, 2021

Copy/pasting links to messages #898

Open

Base automatically changed from master to main January 30, 2021 20:30

zulipbot added the has conflicts label Jan 30, 2021

hotkeys/keys: Add VIEW_IN_BROWSER for viewing a message in the browser.

621c1e6

punchagan and others added 5 commits April 4, 2021 14:27

core/helper: Suppress stderr and stdout for view_in_browser().

4b080fa

refactor: core: Use MACOS AND WSL in view_in_browser().

fa82f8a

views: Amend trigger sequence for view_in_browser().

7c0c9fc

This makes MsgInfoView handle VIEW_IN_BROWSER keypress in order to avoid too many/unintended triggers. Tests amended and added.

core/views: Use near_message_url() to open messages in the topic narrow.

584c94e

This improves view_in_browser() to open messages in the exact/topic narrow for streams and in the pm-with narrow for PMs and huddles. Tests amended.

preetmishra force-pushed the view-in-browser branch from 3912780 to 584c94e Compare April 4, 2021 08:57

zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Apr 4, 2021

zee-bit mentioned this pull request Apr 10, 2021

Add support for opening message in browser. #991

Merged

neiljp closed this Jun 11, 2021

neiljp added the enhancement New feature or request label Jun 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add support for viewing a message in the web browser #698

[WIP] Add support for viewing a message in the web browser #698

preetmishra commented Jun 19, 2020 •

edited

Loading

neiljp commented Jun 19, 2020

preetmishra commented Jun 22, 2020

zulipbot commented Jul 30, 2020

preetmishra commented Jul 30, 2020

preetmishra commented Jul 31, 2020

preetmishra commented Aug 30, 2020

neiljp left a comment

neiljp Aug 9, 2020

neiljp Aug 9, 2020

neiljp Aug 9, 2020

neiljp Aug 31, 2020

neiljp Aug 31, 2020

neiljp Aug 31, 2020

neiljp Aug 31, 2020

zulipbot commented Jan 30, 2021

zee-bit commented Mar 24, 2021

preetmishra commented Mar 31, 2021

preetmishra commented Apr 4, 2021

neiljp commented Jun 11, 2021

		if (not MACOS and not WSL and not os.environ.get('DISPLAY')
		and os.environ.get('TERM')):

		# Truncate extra '/' from the server url.
		self.url = near_message_url(self.model.server_url[:-1], message)

[WIP] Add support for viewing a message in the web browser #698

[WIP] Add support for viewing a message in the web browser #698

Conversation

preetmishra commented Jun 19, 2020 • edited Loading

neiljp commented Jun 19, 2020

preetmishra commented Jun 22, 2020

zulipbot commented Jul 30, 2020

preetmishra commented Jul 30, 2020

preetmishra commented Jul 31, 2020

preetmishra commented Aug 30, 2020

neiljp left a comment

Choose a reason for hiding this comment

neiljp Aug 9, 2020

Choose a reason for hiding this comment

neiljp Aug 9, 2020

Choose a reason for hiding this comment

neiljp Aug 9, 2020

Choose a reason for hiding this comment

neiljp Aug 31, 2020

Choose a reason for hiding this comment

neiljp Aug 31, 2020

Choose a reason for hiding this comment

neiljp Aug 31, 2020

Choose a reason for hiding this comment

neiljp Aug 31, 2020

Choose a reason for hiding this comment

zulipbot commented Jan 30, 2021

zee-bit commented Mar 24, 2021

preetmishra commented Mar 31, 2021

preetmishra commented Apr 4, 2021

neiljp commented Jun 11, 2021

preetmishra commented Jun 19, 2020 •

edited

Loading