signalapp / Signal-Desktop

A private messenger for Windows, macOS, and Linux.
https://signal.org/download
GNU Affero General Public License v3.0
14.52k stars 2.63k forks source link

[OSX] way to *completely* disable bouncing dock icon #1762

Closed GhostLyrics closed 2 years ago

GhostLyrics commented 6 years ago

Bug description

The Dock icon of Signal bounces and breaks my focus while working

Steps to reproduce

Actual result: There is no control to completely disable bouncing. Expected result: There would be a checkbox saying "enable bouncing Dock icon" from which I can remove the check which is set by default, thereby removing the bounciness :)

Platform info

Operating System: macOS 10.13.1 Signal version: Signal Desktop/Electron 1.0.37

garrettr commented 6 years ago

I'm happy to implement this, but want to run my approach by the core maintainers first. I am considering two approaches for resolving this issue:

  1. Implement a macOS-only option to disable the bouncing dock icon.
    • Shop optional preference option only on macOS.
    • Check whether to bounce the Dock icon in the draw-attention IPC handler in main.js.
  2. Implement a platform-agnostic option to disable visual notifications.
    • Wrap a conditional around window.drawAttention in notifications.js.
    • Picking the right wording for the end user-facing preference label is slightly tricky, since "visual notifications" differ across platforms. I would go with "Enable/Disable visual notifications," but that might not be clear enough.
scottnonnenberg commented 6 years ago

I will note that a change has already been made in beta builds on this front. The location of the drawAttention() call was changed to better reflect the user setting: https://github.com/WhisperSystems/Signal-Desktop/pull/1612

You can find the beta mac install here: https://updates.signal.org/desktop/beta-mac.yml

Though perhaps this is truly different. You'd like to see the banner notifications, but you don't want any dock bouncing. Is that correct?

garrettr commented 6 years ago

You'd like to see the banner notifications, but you don't want any dock bouncing. Is that correct?

@scottnonnenberg Personally I just want to disable notifications entirely. #1612 resolves this issue satisfactorily for me.

GhostLyrics commented 6 years ago

You'd like to see the banner notifications, but you don't want any dock bouncing. Is that correct?

For my personal preferences, yes.

I think my internal reasoning here is:

but that's just what goes in in my head when thinking about notifications.

phillipamann commented 6 years ago

Is there progress being made on this? I'd be happy to implement a solution as well to remove the bouncing dock icon or give the user on the Mac the option to remove the icon. It bothers me and I have the time to fix it.

scottnonnenberg commented 6 years ago

@phillipamann It's not in-progress now, no. What do you have in mind to implement, exactly?

phillipamann commented 6 years ago

Hello @scottnonnenberg, sorry for my delay in replying. First it was the holidays and then I got sick. Anyways, hi. I spent a bit of time exploring the code. I am not a JS developer so all of this is new to me. I noticed this line in main.js. I see this block of code specifically:

ipc.on('draw-attention', () => {
  if (process.platform === 'darwin') {
    app.dock.bounce();
  } else if (process.platform === 'win32') {
    mainWindow.flashFrame(true);
    setTimeout(() => {
      mainWindow.flashFrame(false);
    }, 1000);
  } else if (process.platform === 'linux') {
    mainWindow.flashFrame(true);
  }
});

In notifications.js, on line 56, you call window.drawAttention();. What I propose is a duplicate of the audio notification settings. That is, I make an optional box that includes dock bouncing in the exact same way as the audio setting. If this is checked, after audio, I check for existence of the dock bounce and then call window.drawAttention(); in the same exact way. This is a pretty easy fix I think. Let me know what you think and I'll implement.

scottnonnenberg commented 6 years ago

Part of the thinking here is that dock-bouncing is a setting totally unique to Mac. But this is a cross-platform app. What can we do to introduce a setting which makes sense for everyone? Is that possible? A binary yes/no for notifications, then doing the right thing for the OS is far easier.

phillipamann commented 6 years ago

Haha, I need to turn on notifications for this site. Anyways, what happens when window.drawAttention() is called on Windows and Linux, respectively? If nothing happens in either platform, then perhaps move this specifically into the notification calls rather than existing before the notification calls?

scottnonnenberg commented 6 years ago

This is all the happens when window.drawAttention() is called - it runs the code you were looking at in main.js:

window.drawAttention = function() {
  console.log('draw attention');
  ipc.send('draw-attention');
};
phillipamann commented 6 years ago

OK, I have to be honest. I have since disabled notifications for this app completely so this doesn't even help me anymore but why not contribute?

I read the documentation for flashFrame (https://github.com/electron/electron/blob/master/docs/tutorial/desktop-environment-integration.md). This is claimed to not work on Linux but I don't run a Linux graphical shell / desktop environment when I use Linux. Here is what I finally propose:

  1. Add a 'Dock Bounce (OS X) / Flash Frame (Windows / Linux)' checkbox in the notification settings under the 'Play audio notification' checkbox.

  2. Add this code to line 50 of notifications.js under the audioNotification call.

var dockFlashNotification = storage.get(dockFlash-notification') || false;
if (dockFlash-notification) {
    window.drawAttention();
}

Please tell me what you think. I think this is a good solution that allows flexibility to the user and is simple to add.

phillipamann commented 6 years ago

Also, I turned on email notifications so I'll reply within 24 hours from now on 😉.

scottnonnenberg commented 6 years ago

@phillipamann I think that's a worthwhile checkbox. Go ahead and submit that PR. Just remember that variables can't have dashes in them! :0)

phillipamann commented 6 years ago

OK great. Thanks! One last question. Is there any thing I need to include in this with regards to testing? I have never worked on Electron before or are my two code suggestions enough? I'll test locally on my Mac.

scottnonnenberg commented 6 years ago

Nothing in particular, especially something that isn't algorithmic. That's more data pipelining, and we just need to make sure that everything is connected together properly. No guarantees that I won't come up with additional feedback or advice when I see your code!

phillipamann commented 6 years ago

OK. I’ll submit my PR first and we can go from there. Thanks!

phillipamann commented 6 years ago

OK, I have finished all of the code, all tests pass, the checkbox is added, and I have set up two numbers on staging. However, I am a bit stuck with what I should do now. I have created two profiles for testing on my local machine. I just can't modify the contact list of either so I can send a message from one test account to the other test account to verify the dock bounce behavior is working. I'll keep working on this and if I am able to resolve, I'll comment here. I am close!

scottnonnenberg commented 6 years ago

@phillipamann You can send messages to any arbitrary Signal user using the search box in the top left. Just enter the phone number, then 'Click to add contact' or 'Start conversation' (depending on what branch/version you're using).

phillipamann commented 6 years ago

Hello @scottnonnenberg, I have opened a PR: #2102. Thanks for your help and patience.