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

model/views: Add notification checkboxes in Stream Info popup. #900

Merged
merged 8 commits into from
Jul 13, 2021
Merged

model/views: Add notification checkboxes in Stream Info popup. #900

merged 8 commits into from
Jul 13, 2021
+222 −20

Conversation

zee-bit
Copy link
Member

@zee-bit zee-bit commented Jan 30, 2021

  • Added Visual desktop notification checkbox in StreamInfo.

Gradually Fixes #887.

@zulipbot zulipbot added the size: XL [Automatic label added by zulipbot] label Jan 30, 2021
Base automatically changed from master to main January 30, 2021 20:31
@zee-bit zee-bit force-pushed the StreamNotifications branch 2 times, most recently from 3691fc4 to 12d2537 Compare February 1, 2021 18:50
@zee-bit
Copy link
Member Author

zee-bit commented Feb 3, 2021

@zulipbot add "PR needs review"

@zulipbot zulipbot added the PR needs review PR requires feedback to proceed label Feb 3, 2021
@zee-bit zee-bit force-pushed the StreamNotifications branch 2 times, most recently from 0934814 to c0d22b6 Compare February 22, 2021 07:09
@zee-bit zee-bit force-pushed the StreamNotifications branch 2 times, most recently from 68c3cdf to c3c6206 Compare March 15, 2021 19:08
@zee-bit zee-bit force-pushed the StreamNotifications branch from c3c6206 to 58d970e Compare March 26, 2021 14:35
neiljp pushed a commit that referenced this pull request Mar 30, 2021
@zee-bit @neiljp
run: Add --notify and no-notify options as command-line arguments.
d45cdf5 
This commit adds new command-line arguments for notify and no-notify
that corresponds to desktop notifications being enabled or disabled
in the current session, respectively. This should allow the user to
specify a notify option and override its corresponding value in the
zuliprc.

This commit is a potential prep for #900, that enables modifying
Stream notification settings in a running session.

Tests added.
@zee-bit zee-bit force-pushed the StreamNotifications branch from 58d970e to 460281c Compare June 5, 2021 21:00
preetmishra
preetmishra reviewed Jul 4, 2021
View reviewed changes
Copy link
Member

@preetmishra preetmishra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@zee-bit Thanks for working on this! This seems to work well locally for the most part. 👍

This looks good overall. However, I think we would benefit from a discussion on the public stream regarding how should we present to the end-users that ZT is currently relying on the Desktop Notifications settings. And, most importantly that toggling notification settings in ZT would affect Desktop Notifications directly.

I have left a couple of in-line comments. This currently doesn't handle updates while the StreamInfoView is active.

tests/model/test_model.py Outdated Show resolved Hide resolved
zulipterminal/model.py Outdated Show resolved Hide resolved
tests/model/test_model.py Outdated Show resolved Hide resolved
tests/model/test_model.py Outdated Show resolved Hide resolved
tests/model/test_model.py Outdated Show resolved Hide resolved
tests/model/test_model.py Outdated Show resolved Hide resolved
tests/model/test_model.py Outdated Show resolved Hide resolved
tests/ui_tools/test_popups.py Outdated Show resolved Hide resolved
tests/ui_tools/test_popups.py Outdated Show resolved Hide resolved
zulipterminal/ui_tools/views.py Show resolved Hide resolved
@preetmishra preetmishra added further discussion required Discuss this on #zulip-terminal on chat.zulip.org PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jul 4, 2021
@zee-bit zee-bit force-pushed the StreamNotifications branch from 460281c to e23e4bf Compare July 5, 2021 14:39
@zee-bit zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 5, 2021
@zee-bit zee-bit force-pushed the StreamNotifications branch from e23e4bf to 40d829b Compare July 10, 2021 09:08
zee-bit added 8 commits July 13, 2021 12:37
@zee-bit @neiljp
model: Store streams with "desktop_notifications" enabled in model.
dbc5299 
This commit stores all subscribed streams that have desktop_notifications
enabled to model's visual_notified_streams instance variable. We use a set
data-structure to store since the stream_id would be unique and we do not
need to sort it like pinned_streams.

Tests amended.
@zee-bit @neiljp
model: Add helper to check if a stream has visual notification enabled.
0fbc468 
We add a helper function in model, is_visual_notifications_enabled(), that
accepts a stream_id as argument and checks if the stream has visual desktop
notifications turned on, which is indicated by the "desktop_notifications"
field in the Subscription data.

Tests added.
@zee-bit @neiljp
model: Modify notify_user to depend upon new state.
9918d70 
Tests amended.
@zee-bit @neiljp
model: Handle subscription events for desktop_notifications property.
e28c68f 
This commit adds handling for subscription events related to
desktop_notifications. The model._handle_subscription_events now also
checks for events with change in "desktop_notifications" property and
adds/removes those streams from model.visual_notified_streams variable
depending on the value specified in the event. This allows the model to
stay in sync with the server for changes in visual notified streams.

Tests added.

Code & test cases for adding-existing/removing-absent ids by neiljp.
@zee-bit @neiljp
model: Add helper function to toggle visual notifications for streams.
36d0f19 
This commit adds a helper function in model to send toggle requests to
the server for "desktop_notifications" field of subscription data. This
function accepts a stream_id for which desktop_notifications are to be
toggled, then checks the current status of the field with
model.is_visual_notifications_enabled and then sends the appropriate
toggle request to the server.

Tests added.
@zee-bit @neiljp
views: Add "Visual desktop notification" checkbox in Stream Info.
28bafd6 
This commit adds a new checkbox 'Visual desktop notification' in the
Stream settings section inside StreamInfoView. It uses model's
visual_notified_streams to get the initial state of the checkbox and
henceforth events to sync the "desktop notifications" data between
ZT <-> server.

The checkboxes are not updated dynamically after events but the
underlying data structure gets updated, hence re-opening the StreamInfo
view will display the correct state. We have a WIP PR zulip#981 that deals
with updating these checkboxes dynamically while the popup is still open.

Tests amended.

Fixes zulip#887.
@zee-bit @neiljp
config/themes: Add "widget_disabled" style attribute for all themes.
2979db9 
This commit adds a "widget_disabled" style for all themes that could be
passed as a Display attribute to any widget which needs to be disabled
for certain cases. This atttribute can be coupled with urwid.WidgetDisable
to effectively produce a completely disabled widget both visually and
functionally.

This renders the widget foreground in DARK_GRAY in most themes and the
background is kept as the default theme background. For 1-bit color-depth
the widget will render with a strikethrough.
@zee-bit @neiljp
views: Disable Visual notification checkbox when notify is disabled.
070dee8 
This commit disables the "Visual desktop notification" checkbox when notify
is disabled i.e. when --notify is not passed as CL arg. It wraps the widget
in urwid.WidgetDisable decoration class and uses the "widget_disabled"
display attribute to render a completely disabled checkbox, both visually
and functionally.
@neiljp neiljp force-pushed the StreamNotifications branch from 40d829b to 070dee8 Compare July 13, 2021 20:09
@neiljp
Copy link
Collaborator

neiljp commented Jul 13, 2021

@zee-bit Thanks for working on this - this is really clean and it's great to have another configuration option available 👍
I pushed here with the fixups as proposed in the stream and the slight reorder. Merging now! 🎉

We discussed splitting the solution to #666 into a separate PR (there was a part in this PR originally), but we also shouldn't forget the adjustment of the stream notification text to explain the disabling of the checkbox - might be worth addressing that first since it's likely small?

@neiljp neiljp merged commit 88e072e into zulip:main Jul 13, 2021
@neiljp neiljp added this to the Next Release milestone Jul 13, 2021
@zee-bit zee-bit mentioned this pull request Jul 13, 2021
Merged
2 tasks
@zee-bit zee-bit mentioned this pull request Aug 14, 2021
Closed
5 tasks
@neiljp neiljp added area: event handling How events from the server are responded to area: UI General user interface update missing feature: user A missing feature for all users, present in another Zulip client and removed further discussion required Discuss this on #zulip-terminal on chat.zulip.org PR needs review PR requires feedback to proceed labels Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@preetmishra preetmishra preetmishra left review comments

Assignees
No one assigned
Labels
area: event handling How events from the server are responded to area: UI General user interface update missing feature: user A missing feature for all users, present in another Zulip client size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

Add notification checkboxes in Stream Info view.
4 participants
@zee-bit @neiljp @preetmishra @zulipbot