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

Fix: Windows: Prevent Notifications from Showing on Lock Screen #820 #1410

Conversation

avijitdas126
Copy link
Collaborator

@avijitdas126 avijitdas126 commented Feb 22, 2025

This update ensures that notifications are not displayed when the system is locked on Windows. Using Electron's powerMonitor, the app detects when the screen is locked and blocks notifications to prevent potential leaks of sensitive information.

Fixes: #820

  • Implemented powerMonitor event listeners to detect lock-screen and unlock-screen events.
  • Added a flag (isLocked) to control notification visibility.
  • Modified newNotification function to check isLocked before creating a notification.
  • Prevents unnecessary notifications when the user is away.

Platforms this PR was tested on:

  • Windows
  • macOS
  • Linux (specify distro)
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.

Added deep linking support with zulip://, registered the protocol handler, and implemented a dialog to choose between opening the link in the app or browser.
This update ensures that notifications are not displayed when the system is locked on Windows. Using Electron's powerMonitor, the app detects when the screen is locked and blocks notifications to prevent potential leaks of sensitive information.
@zulipbot
Copy link
Member

Hello @avijitdas126, it seems like you have referenced #820 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #820..

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.

To learn how to write a great commit message, please refer to our guide.

@zulipbot zulipbot added size: S and removed size: L labels Feb 22, 2025
Remove some extra code and which is added on previous commit
@avijitdas126 avijitdas126 force-pushed the fix/windows-notification-lockscreen-820 branch from 3e8f43e to 6ebb9be Compare February 22, 2025 12:13
@avijitdas126
Copy link
Collaborator Author

could anyone review this?

@andersk
Copy link
Member

andersk commented Feb 24, 2025

Windows already has a Show notifications on the lock screen setting for this.

Some users might want this behavior, so why shouldn’t we should leave this up to their Windows settings instead of forcing our own opinion on them?

@avijitdas126
Copy link
Collaborator Author

avijitdas126 commented Feb 24, 2025

Windows already has a Show notifications on the lock screen setting for this.

Some users might want this behavior, so why shouldn’t we should leave this up to their Windows settings instead of forcing our own opinion on them?

We should consider that some users might not be aware that Windows allows lock screen notifications, and displaying sensitive information there could be unexpected. While respecting system settings is important, giving users the flexibility to control this behavior within our app ensures a better user experience and prevents potential privacy concerns. Many apps, especially those handling sensitive data, provide this option to avoid unintended exposure. Instead of forcing an opinion, offering an in-app setting alongside the system behavior allows users to make an informed choice.

@andersk
Copy link
Member

andersk commented Feb 28, 2025

This has no such setting, and since this is almost certainly AI generated, I’m not interested in arguing about it with your AI.

@andersk andersk closed this Feb 28, 2025
@avijitdas126
Copy link
Collaborator Author

avijitdas126 commented Feb 28, 2025

This has no such setting, and since this is almost certainly AI generated, I’m not interested
No sir, I cannot use ai for solve this issue

In app/renderer/js/notification/index.ts
powerMonitor detects using

powerMonitor.on("lock-screen", () => {
  isLocked = true;
});

powerMonitor.on("unlock-screen", () => {
  isLocked = false;
});

From electron documentation

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

Successfully merging this pull request may close these issues.

Windows: Notification shown even on lock screen
3 participants