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

Make app notifications appear in new workspaces + dismiss on all workspaces #23432

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Jan 21, 2025

The keymap error notifications got convoluted to support displaying the notification on startup. This change addresses it systemically for all future app notifications.

Reverts most of #20531, while keeping the fix to handle keyboard layout switching. This is a better fix for #20531

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 21, 2025
…spaces

The keymap error notifications got convoluted to support displaying the notification on startup. This change addresses it systemically for all future app notifications
@mgsloan mgsloan force-pushed the make-app-notifications-appear-in-new-workspaces branch from f670953 to 045e8a1 Compare January 21, 2025 22:44
@mgsloan mgsloan enabled auto-merge (squash) January 21, 2025 22:44
@notpeter
Copy link
Member

Would this also fix:

Because the update toast would appear in all workspaces simultaneously (not just the one)?

@contrast-jproberts
Copy link
Contributor

contrast-jproberts commented Jan 21, 2025

Would this also fix:

Because the update toast would appear in all workspaces simultaneously (not just the one)?

Heh, I was about to open a PR for this using the older behavior. The new show_app_notification sounds great and I think it would resolve #23236 ! However, I'm not sure that it's working as described. It's totally possible that my testing method is flawed though. Here's what I tried:

  • Pull this branch
  • Apply this patch (with newline before EOF, that's usually truncated by GitHub)
diff --git a/crates/auto_update_ui/src/auto_update_ui.rs b/crates/auto_update_ui/src/auto_update_ui.rs
index 9114375e88..8c75ce838f 100644
--- a/crates/auto_update_ui/src/auto_update_ui.rs
+++ b/crates/auto_update_ui/src/auto_update_ui.rs
@@ -124,7 +124,7 @@ pub fn notify_of_any_new_update(cx: &mut ViewContext<Workspace>) -> Option<()> {
 
     cx.spawn(|workspace, mut cx| async move {
         let should_show_notification = should_show_notification.await?;
-        if should_show_notification {
+        if true {
             workspace.update(&mut cx, |workspace, cx| {
                 let workspace_handle = workspace.weak_handle();
                 workspace.show_notification(
  • Build and run Zed
  • Open 2 workspaces
  • I see notifications in both workspaces, which is great
  • However, when I dismiss the notification in 1 workspace, it doesn't get dismissed in the other. I've tried this with both the first workspace and the second workspace in the window stack. Neither case dismissed the notification across workspaces.

Do you mind taking a look at this scenario before merging this work? For folks who are messy and have multiple workspaces (like me), it would be a little tedious to go through every workspace and dismiss the notification after auto-update. If the first dismissal propagates across all workspaces like you planned, that sounds perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants