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.

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

settings: Add option to disable seeing typing notifications. #29719

Merged
merged 2 commits into from Apr 16, 2024

Conversation

roanster007
Copy link
Collaborator

This PR adds an option to the advanced section of Preferences settings, that would allow users to choose whether or not to receive typing notifications from other users.

Fixes: #29642

Screenshots and screen captures:

image

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@zulipbot
Copy link
Member

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings (user)" label, so you may want to check it out!

@timabbott
Copy link
Sponsor Member

This looks basically good; had some comments on API documentation and started a naming thread in https://chat.zulip.org/#narrow/stream/378-api-design/topic/Disabling.20showing.20typing.20notifications/near/1779758.

Generally whenever you're starting an API change project, you should start an "api design" thread about what to call the thing, since it's a detail that should always be chosen carefully. (Unlike most other names, they're a pain to change).

@roanster007 roanster007 force-pushed the iss-29642 branch 2 times, most recently from 8a57953 to 65334ba Compare April 15, 2024 14:15
@timabbott
Copy link
Sponsor Member

This looks basically good pending a final decision on renaming the field, and any other API details.

@roanster007 roanster007 force-pushed the iss-29642 branch 3 times, most recently from 98fec62 to b9ea61f Compare April 16, 2024 14:10
@roanster007
Copy link
Collaborator Author

updated to use receives_typing_notifications

@@ -208,6 +208,7 @@ def json_change_settings(
default=None,
),
starred_message_counts: Optional[bool] = REQ(json_validator=check_bool, default=None),
receives_typing_notifications: Optional[bool] = REQ(json_validator=check_bool, default=None),
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

If we aren't sorting these parameters, it would probably be best to put this next to the others related to typing notifications. But maybe it's worth just doing a follow-up PR to sort the parameters both here and in zerver/views/realm.py; one less thing to make decisions about will be helpful.

This commit re orders the 'Use full width on wide screens' and
'Show counts for starred messages' settings of Preferences settings.
@@ -72,6 +72,7 @@ class UserBaseSettings(models.Model):
display_emoji_reaction_users = models.BooleanField(default=True)
twenty_four_hour_time = models.BooleanField(default=False)
starred_message_counts = models.BooleanField(default=True)
receives_typing_notifications = models.BooleanField(default=True)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Moved this to be next to the other typing settings in this file for better readability.

notifications from other users.

**Changes**: New in Zulip 9.0 (feature level 253). Previously, there were
only options to disable sending typing notifications.
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

These changes entries should appear in all the places in this file that the field is mentioned -- it looks like it's missing in a couple places.

This commit adds an option to the advanced section of
Preferences settings, that would allow users to choose
whether to receive typing notifications from other
users.

Fixes zulip#29642
@timabbott timabbott merged commit 68b4298 into zulip:main Apr 16, 2024
5 checks passed
@timabbott
Copy link
Sponsor Member

Merged, after fixing the small details noted above, thanks @roanster007! I'd appreciate your doing a follow-up PR to sort the keyword argument settings fields as noted above, and maybe the similar one in update_realm; maybe also passing * to indicate the boundary with kwargs might be helpful:

def update_realm(                                                                                       
    request: HttpRequest,                                                                               
    user_profile: UserProfile,                                                                          
    *,
     ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to disable seeing typing notifications
3 participants