Skip to content

Conversation

preetmishra
Copy link
Member

@preetmishra preetmishra commented Aug 7, 2020

This adds support for rendering timezone-aware time mentions which were recently added to the Zulip's markdown suite.

(See https://zulipchat.com/help/format-your-message-using-markdown#mention-a-time for markdown details.)

Complimentary additions

  • msg_time style in themes.py.
  • TIME_MENTION_MARKER symbol in symbols.py.

Test amended.

Fixes #691.

@zulipbot zulipbot added size: M [Automatic label added by zulipbot] high priority should be done as soon as possible area: message rendering labels Aug 7, 2020
@preetmishra
Copy link
Member Author

preetmishra commented Aug 7, 2020

It turns out the first-party datetime module doesn't have good support for handling ISO format strings for python < 3.7. This is where the module python-dateutil comes in with a parser.

The python-dateutil package also has tzlocal() for working with local time zones. However, it has a few caveats and is too tricky to mock and test. This is where the module tzlocal kicks in.

@preetmishra preetmishra added the PR needs review PR requires feedback to proceed label Aug 7, 2020
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 Thanks for working on this - while entering the dates could be tricky in ZT (as discussed on czo), this will be useful to coordinate sync meetings for the rest of this Summer or generally across multiple timezones :)

I did notice that when I edit a date in ZT (possibly when I enter it wrongly first) then it doesn't update on the webapp, but I'm guessing that's an issue with the webapp - we re-fetch the message content and re-render fine from what I can tell.

This isn't a change in behavior, but perhaps we could indicate this is feature level 8/16 and new in v3.0?

@@ -7,3 +7,4 @@
APPLICATION_TITLE_BAR_LINE = '═'
PINNED_STREAMS_DIVIDER = '-'
LIST_TITLE_BAR_LINE = '━'
TIME = '⧗'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may be clearer to some degree, but I quite like the stopwatch type icon, perhaps from having something similar for 'busy' icons in older systems! We could perhaps discuss this in the stream; these all seem like options:
⌚ U+0231A WATCH
⌛ U+0231B HOURGLASS
⏰ U+023F0 ALARM CLOCK
⏱ U+023F1 STOPWATCH
⏳ U+023F3 HOURGLASS WITH FLOWING SAND
⧖ U+029D6 WHITE HOURGLASS
⧗ U+029D7 BLACK HOURGLASS

Copy link
Member Author

Choose a reason for hiding this comment

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

Out of these, only ⏱, ⧖ and ⧗ seem to not cause the UI distortion bug that we have noticed in urwid.

However, the emoji ⏱ appears to consume an extra space character. To recoup, I have added an extra space in TIME_MENTION_MARKER itself, i.e., TIME_MENTION_MARKER = '⏱ '.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good for this v1 as per discussion in the stream 👍

@neiljp neiljp removed the PR needs review PR requires feedback to proceed label Aug 8, 2020
@preetmishra preetmishra changed the title Markup timezone-aware time mentions [WIP] Markup timezone-aware time mentions Aug 8, 2020
@preetmishra preetmishra force-pushed the feat-markup-time-mentions branch from 31c3a23 to f48afe0 Compare August 10, 2020 15:48
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Aug 10, 2020
@preetmishra
Copy link
Member Author

@neiljp Thanks for the review. I couldn't reproduce the issue that you mentioned with the web app. It worked well for me.

The latest update only has the in-line suggestions incorporated and an extra comment about feature level and server version support.

@preetmishra preetmishra changed the title [WIP] Markup timezone-aware time mentions Markup timezone-aware time mentions Aug 10, 2020
@preetmishra preetmishra added the PR needs review PR requires feedback to proceed label Aug 10, 2020
@neiljp
Copy link
Collaborator

neiljp commented Aug 10, 2020

@preetmishra This looks good now, just pending:

  • fix conflicts after the recent requirements merge
  • maybe assert on input?
  • symbol discussion on czo

I introduced some of the time mention ideas/issues in #general>Time mentions.

@neiljp neiljp added further discussion required Discuss this on #zulip-terminal on chat.zulip.org and removed PR needs review PR requires feedback to proceed labels Aug 10, 2020
@neiljp neiljp added this to the Next Release milestone Aug 10, 2020
This adds python-dateutil and tzlocal to render time mentions in
local time zone.

The package python-dateutil would be used for parsing ISO time string
sent by the server and the package tzlocal would be used for getting
user's local time zone.
@preetmishra preetmishra force-pushed the feat-markup-time-mentions branch from f48afe0 to d97caa2 Compare August 11, 2020 07:32
@preetmishra preetmishra force-pushed the feat-markup-time-mentions branch from d97caa2 to c12d6a2 Compare August 11, 2020 07:34
@preetmishra
Copy link
Member Author

@neiljp Thanks for the review. I have made the suggested amendments in the latest update.

@sumanthvrao
Copy link
Member

@preetmishra This looks great.

I happened to be on WSL when testing this and observed that the stopwatch icon appears much smaller than the other text. Small enough to be mistaken for a point, almost! Is this the same size in other platforms as well?
image

@preetmishra
Copy link
Member Author

@sumanthvrao Thanks for the review. That looks strange. It renders well on my Ubuntu 20.04. Could you test with ?

image

This adds support for rendering timezone-aware time mentions which were
recently added to the Zulip's markdown suite.

See https://zulipchat.com/help/format-your-message-using-markdown#mention-a-time
for markdown details.

Complimentary additions:
* msg_time style in themes.py.
* TIME_MENTION_MARKER symbol in symbols.py.

Test amended.

Fixes zulip#691.
@preetmishra preetmishra force-pushed the feat-markup-time-mentions branch from c12d6a2 to 44901d4 Compare August 11, 2020 16:53
@neiljp
Copy link
Collaborator

neiljp commented Aug 11, 2020

@preetmishra Thanks! Merged with an adjustment to the commit text to indicate the server version/feature-level for time mentions 🎉

@neiljp neiljp closed this Aug 11, 2020
@preetmishra preetmishra deleted the feat-markup-time-mentions branch August 12, 2020 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: message rendering further discussion required Discuss this on #zulip-terminal on chat.zulip.org high priority should be done as soon as possible size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New markdown feature: Mention a time
4 participants