core/run/views: Add --notify and --no-notify flags as a command-line argument for ZT
#964
Conversation
There was a problem hiding this comment.
@zee-bit From the brief discussion in the stream I'm open to this option on the command-line 👍
However, I'm wondering if we want to have something along the lines of --notify and no-notify like we do for autohide? I know that currently doesn't parallel the valid values, but requiring --key=value when there are only two values makes it more verbose and possibly extra understanding of how there are two parts (key + value), rather than just an option.
Re autohide, I think it would be reasonable to add additional values there so we also can have autohide=enabled|disabled in the config?
The commits are quite small, but I appreciate the clarity of keeping each change isolated 👍
zulipterminal/core.py
Outdated
| notify=self.notify_enabled, | ||
| autohide_enabled=self.autohide, |
There was a problem hiding this comment.
This naming is inconsistent.
neiljp
added
the
PR awaiting update
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 #T900, that enables modifying Stream notification settings in a running session. Tests added.
This commit shows the configuration for notify option that the current ZT session is loaded with. If no command-line argument for notify was passed, then it checks for the config in zuliprc file(if present), else uses the default value. Tests amended.
zee-bit
force-pushed
the
add-notify-cli-arg
branch
from
966e28f
to
c047e1f
Compare
zulipbot
added
size: L
zee-bit
changed the title
--notify flag as a command-line argument for ZT--notify and --no-notify flags as a command-line argument for ZT
zulipbot
added
PR needs review
zulipterminal/core.py
Outdated
| @@ -49,7 +49,7 @@ def __init__(self, config_file: str, maximum_footlinks: int, | |||
| self.color_depth = color_depth | |||
| self.in_explore_mode = in_explore_mode | |||
| self.autohide = autohide | |||
| self.notify_enabled = notify | |||
| self.notify = notify | |||
There was a problem hiding this comment.
Let's leave the controller attribute descriptive too.
There was a problem hiding this comment.
(I noticed this since this commit now modifies more files than indicated in the commit)
zulipterminal/ui_tools/views.py
Outdated
| ('Color depth', str(color_depth))]) | ||
| ('Color depth', str(color_depth)), | ||
| ('Notifications', | ||
| 'enabled' if notify_enabled else 'disabled')]) |
There was a problem hiding this comment.
You added some good trailing commas; it'd be good if we could have one here too.
There was a problem hiding this comment.
I have added the trailing comma here as requested, but note that it won't make the diff smaller on adding extra fields(?), since the linter forced me to add extra whitespace after the comma. :)
Add Notifications status(i.e. "enabled" or "disabled") in Application configuration section of About menu. A user can now check the notification configuration that the current ZT session is loaded with. Tests amended.
zee-bit
force-pushed
the
add-notify-cli-arg
branch
from
c047e1f
to
e40748c
Compare
|
@zee-bit Thanks for the update - I merged manually with a few minor textual changes to the commit text, and I just bumped the brackets onto the following line to solve the whitespace issue you mentioned 🎉 |
neiljp
added
area: config
enhancement
This PR adds new command-line configurations for ZT - the
--notifyand--no-notifyflags. These should specify whether the notifications will be enabled in the current session or not respectively. If these flags are used as a command-line argument then the corresponding value in thezuliprcfile is overridden by its value for the current session.The attribute of this config is also shown in the terminal while ZT is loading, and in the About menu when it has loaded.