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

Added a 'auto-hide' notifications setting #10043

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

Conversation

levnikmyskin
Copy link

Hi!
I added an "auto-hide"notification setting (it was requested in a now closed issue #4850).

Motivation

On Telegram Desktop, notification will not hide themselves unless the app detects any user input. This can be annoying when e.g. the user is watching a video/movie and has to move the mouse to make the notification disappear.

What I did

Simply added a "auto-hide" notifications setting, which, when enabled, will make the notifications hide themselves after a few seconds (2 at the moment). I'm attaching a screenshot below of the new setting.

Possible improvements

  • The amount of seconds to make a notification disappear is hardcoded to 2 seconds at the moment. Could it be useful to let the user decide this quantity?
  • I added the english string shown in the settings. Since I'm a native italian speaker, I could add that as well if you wish.

Greetings,
levnikmyskin

image

@CLAassistant
Copy link

CLAassistant commented Dec 31, 2020

CLA assistant check
All committers have signed the CLA.

@23rd
Copy link
Collaborator

23rd commented Dec 31, 2020

Please, use tabs for indentations.

@levnikmyskin
Copy link
Author

Thank you, I added and committed the suggested changes

@ilya-fedin
Copy link
Contributor

I always thought that this is a bug with between player's layer and window manager interaction 😮

@@ -662,6 +662,14 @@ void Notification::prepareActionsCache() {

bool Notification::checkLastInput(bool hasReplyingNotifications) {
if (!_waitingForInput) return true;
if (Core::App().settings().autoHideNotifications()) {
auto now = crl::now();
if ((now - _started > 2000) && !hasReplyingNotifications) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

2000 ms should at least be moved to a separate constexpr auto constant in the anonymous namespace at the file beginning.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I thought about doing that, I was just waiting to see if anyone of you guys thought this should be moved to a user-defined settings. For the moment I moved it to a constexpr auto constant

@levnikmyskin levnikmyskin force-pushed the dev branch 2 times, most recently from c8ccc18 to 1a76cd3 Compare January 5, 2021 15:07
@levnikmyskin levnikmyskin force-pushed the dev branch 2 times, most recently from 1772f79 to ff7104a Compare January 24, 2021 10:58
@levnikmyskin
Copy link
Author

Integrated 23rd suggestions and merged new commits from upstream

@ThigSchuch
Copy link

This PR could fix the issue when notification don't disappear when watching a movie or something else in full screen (even outside from TDesktop).
The notification remain on screen forever, unless I move the mouse or type something.

@levnikmyskin
Copy link
Author

This PR could fix the issue when notification don't disappear when watching a movie or something else in full screen (even outside from TDesktop).
The notification remain on screen forever, unless I move the mouse or type something.

From what I gathered, this has never been an "issue" (or a bug), but a dev choice. I think it'd be nice to have this setting so the "choice" is moved to the user.

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Apr 7, 2021

I propose this:

diff --git a/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp b/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp
index cf5a07099..11c859d6c 100644
--- a/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp
+++ b/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp
@@ -727,6 +727,10 @@ bool SkipFlashBounce() {
 	return Inhibited();
 }
 
+bool WaitForInput() {
+	return true;
+}
+
 bool Supported() {
 	return ServiceRegistered;
 }
diff --git a/Telegram/SourceFiles/platform/linux/notifications_manager_linux_dummy.cpp b/Telegram/SourceFiles/platform/linux/notifications_manager_linux_dummy.cpp
index c51b92fed..cec5fa737 100644
--- a/Telegram/SourceFiles/platform/linux/notifications_manager_linux_dummy.cpp
+++ b/Telegram/SourceFiles/platform/linux/notifications_manager_linux_dummy.cpp
@@ -25,6 +25,10 @@ bool SkipFlashBounce() {
 	return false;
 }
 
+bool WaitForInput() {
+	return true;
+}
+
 bool Supported() {
 	return false;
 }
diff --git a/Telegram/SourceFiles/platform/mac/notifications_manager_mac.mm b/Telegram/SourceFiles/platform/mac/notifications_manager_mac.mm
index 310b2829b..1993872a3 100644
--- a/Telegram/SourceFiles/platform/mac/notifications_manager_mac.mm
+++ b/Telegram/SourceFiles/platform/mac/notifications_manager_mac.mm
@@ -159,6 +159,10 @@ bool SkipFlashBounce() {
 	return SkipAudio();
 }
 
+bool WaitForInput() {
+	return true;
+}
+
 bool Supported() {
 	return Platform::IsMac10_8OrGreater();
 }
diff --git a/Telegram/SourceFiles/platform/platform_notifications_manager.h b/Telegram/SourceFiles/platform/platform_notifications_manager.h
index eb3efbddb..4731a97bb 100644
--- a/Telegram/SourceFiles/platform/platform_notifications_manager.h
+++ b/Telegram/SourceFiles/platform/platform_notifications_manager.h
@@ -15,6 +15,7 @@ namespace Notifications {
 [[nodiscard]] bool SkipAudio();
 [[nodiscard]] bool SkipToast();
 [[nodiscard]] bool SkipFlashBounce();
+[[nodiscard]] bool WaitForInput();
 
 [[nodiscard]] bool Supported();
 [[nodiscard]] bool Enforced();
diff --git a/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp b/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp
index 9ea6c222b..59b92667b 100644
--- a/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp
+++ b/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp
@@ -730,7 +730,6 @@ bool SkipToast() {
 
 	if (UserNotificationState == QUNS_PRESENTATION_MODE
 		|| UserNotificationState == QUNS_RUNNING_D3D_FULL_SCREEN
-		//|| UserNotificationState == QUNS_BUSY
 		|| QuietHoursEnabled) {
 		return true;
 	}
@@ -741,5 +740,14 @@ bool SkipFlashBounce() {
 	return SkipToast();
 }
 
+bool WaitForInput() {
+	querySystemNotificationSettings();
+
+	if (UserNotificationState == QUNS_BUSY) {
+		return false;
+	}
+	return true;
+}
+
 } // namespace Notifications
 } // namespace Platform
diff --git a/Telegram/SourceFiles/window/notifications_manager_default.cpp b/Telegram/SourceFiles/window/notifications_manager_default.cpp
index a5ea81f96..d6e216f18 100644
--- a/Telegram/SourceFiles/window/notifications_manager_default.cpp
+++ b/Telegram/SourceFiles/window/notifications_manager_default.cpp
@@ -667,7 +667,9 @@ void Notification::prepareActionsCache() {
 bool Notification::checkLastInput(bool hasReplyingNotifications) {
 	if (!_waitingForInput) return true;
 
-	const auto waitForUserInput = base::Platform::LastUserInputTimeSupported()
+	const auto shouldWaitForUserInput = Platform::Notifications::WaitForInput()
+		&& base::Platform::LastUserInputTimeSupported();
+	const auto waitForUserInput = shouldWaitForUserInput
 		? (Core::App().lastNonIdleTime() <= _started)
 		: false;
 	if (!waitForUserInput) {

This should hide notifications automatically if something is running fullscreen (like video player). Implemented only for Windows, other platforms need implementations.

@Nirzak
Copy link

Nirzak commented Apr 7, 2021

I propose this:

diff --git a/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp b/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp
index cf5a07099..11c859d6c 100644
--- a/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp
+++ b/Telegram/SourceFiles/platform/linux/notifications_manager_linux.cpp
@@ -727,6 +727,10 @@ bool SkipFlashBounce() {
 	return Inhibited();
 }
 
+bool WaitForInput() {
+	return true;
+}
+
 bool Supported() {
 	return ServiceRegistered;
 }
diff --git a/Telegram/SourceFiles/platform/linux/notifications_manager_linux_dummy.cpp b/Telegram/SourceFiles/platform/linux/notifications_manager_linux_dummy.cpp
index c51b92fed..cec5fa737 100644
--- a/Telegram/SourceFiles/platform/linux/notifications_manager_linux_dummy.cpp
+++ b/Telegram/SourceFiles/platform/linux/notifications_manager_linux_dummy.cpp
@@ -25,6 +25,10 @@ bool SkipFlashBounce() {
 	return false;
 }
 
+bool WaitForInput() {
+	return true;
+}
+
 bool Supported() {
 	return false;
 }
diff --git a/Telegram/SourceFiles/platform/mac/notifications_manager_mac.mm b/Telegram/SourceFiles/platform/mac/notifications_manager_mac.mm
index 310b2829b..1993872a3 100644
--- a/Telegram/SourceFiles/platform/mac/notifications_manager_mac.mm
+++ b/Telegram/SourceFiles/platform/mac/notifications_manager_mac.mm
@@ -159,6 +159,10 @@ bool SkipFlashBounce() {
 	return SkipAudio();
 }
 
+bool WaitForInput() {
+	return true;
+}
+
 bool Supported() {
 	return Platform::IsMac10_8OrGreater();
 }
diff --git a/Telegram/SourceFiles/platform/platform_notifications_manager.h b/Telegram/SourceFiles/platform/platform_notifications_manager.h
index eb3efbddb..4731a97bb 100644
--- a/Telegram/SourceFiles/platform/platform_notifications_manager.h
+++ b/Telegram/SourceFiles/platform/platform_notifications_manager.h
@@ -15,6 +15,7 @@ namespace Notifications {
 [[nodiscard]] bool SkipAudio();
 [[nodiscard]] bool SkipToast();
 [[nodiscard]] bool SkipFlashBounce();
+[[nodiscard]] bool WaitForInput();
 
 [[nodiscard]] bool Supported();
 [[nodiscard]] bool Enforced();
diff --git a/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp b/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp
index 9ea6c222b..59b92667b 100644
--- a/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp
+++ b/Telegram/SourceFiles/platform/win/notifications_manager_win.cpp
@@ -730,7 +730,6 @@ bool SkipToast() {
 
 	if (UserNotificationState == QUNS_PRESENTATION_MODE
 		|| UserNotificationState == QUNS_RUNNING_D3D_FULL_SCREEN
-		//|| UserNotificationState == QUNS_BUSY
 		|| QuietHoursEnabled) {
 		return true;
 	}
@@ -741,5 +740,14 @@ bool SkipFlashBounce() {
 	return SkipToast();
 }
 
+bool WaitForInput() {
+	querySystemNotificationSettings();
+
+	if (UserNotificationState == QUNS_BUSY) {
+		return false;
+	}
+	return true;
+}
+
 } // namespace Notifications
 } // namespace Platform
diff --git a/Telegram/SourceFiles/window/notifications_manager_default.cpp b/Telegram/SourceFiles/window/notifications_manager_default.cpp
index a5ea81f96..d6e216f18 100644
--- a/Telegram/SourceFiles/window/notifications_manager_default.cpp
+++ b/Telegram/SourceFiles/window/notifications_manager_default.cpp
@@ -667,7 +667,9 @@ void Notification::prepareActionsCache() {
 bool Notification::checkLastInput(bool hasReplyingNotifications) {
 	if (!_waitingForInput) return true;
 
-	const auto waitForUserInput = base::Platform::LastUserInputTimeSupported()
+	const auto shouldWaitForUserInput = Platform::Notifications::WaitForInput()
+		&& base::Platform::LastUserInputTimeSupported();
+	const auto waitForUserInput = shouldWaitForUserInput
 		? (Core::App().lastNonIdleTime() <= _started)
 		: false;
 	if (!waitForUserInput) {

This should hide notifications automatically if something is running fullscreen (like video player). Implemented only for Windows, other platforms need implementations.

On bool WaitForInput() function will it be UserNotificationState == QUNS_BUSY or UserNotificationState == QUNS_RUNNING_D3D_FULL_SCREEN?

@ilya-fedin
Copy link
Contributor

On bool WaitForInput() function will it be UserNotificationState == QUNS_BUSY or UserNotificationState == QUNS_RUNNING_D3D_FULL_SCREEN?

QUNS_BUSY. QUNS_RUNNING_D3D_FULL_SCREEN is checked in SkipToast, so it shouldn't show the toast at all. QUNS_RUNNING_D3D_FULL_SCREEN should prevent showing notifications when some game is running, but it doesn't work with all games for some reason (#2290)

@Nirzak
Copy link

Nirzak commented Apr 7, 2021

On bool WaitForInput() function will it be UserNotificationState == QUNS_BUSY or UserNotificationState == QUNS_RUNNING_D3D_FULL_SCREEN?

QUNS_BUSY. QUNS_RUNNING_D3D_FULL_SCREEN is checked in SkipToast, so it shouldn't show the toast at all. QUNS_RUNNING_D3D_FULL_SCREEN should prevent showing notifications when some game is running, but it doesn't work with all games for some reason (#2290)

Yeah, I see. But the function will be more reasonable with the name "SkipNotification()" or something to help debug the codes later.

@ilya-fedin
Copy link
Contributor

But the function will be more reasonable with the name "SkipNotification()"

  1. What function?
  2. It's already in Notifications namespace.

@john-preston
Copy link
Member

The idea of current beahaviour is to keep notifications on screen while the user is away from his computer. When he returns he sees the notifications. I'm not against removing notifications completely if the user is busy at the computer without input events. I think if user watches a movie the notifications should not appear, maybe only sound can be played.

@ilya-fedin
Copy link
Contributor

@john-preston won't people from #473 complain again?

@Nirzak
Copy link

Nirzak commented Apr 7, 2021

The idea of current beahaviour is to keep notifications on screen while the user is away from his computer. When he returns he sees the notifications. I'm not against removing notifications completely if the user is busy at the computer without input events. I think if user watches a movie the notifications should not appear, maybe only sound can be played.

ilya-fedin's code is to solve the issue to detect if users are playing any video in fullscreen.

@Nirzak
Copy link

Nirzak commented Apr 7, 2021

@john-preston won't people from #473 complain again?

Looks like there're still some folks who want notification during watching a movie or something?

@ilya-fedin
Copy link
Contributor

Looks like there're still some folks who want notification during watching a movie or something?

Probably

@fguille94
Copy link

Looks like there're still some folks who want notification during watching a movie or something?

Probably

Let that be an option though, some of us do want that notification to go away, think the notification count number should be enough for when "the user comes back and realizes he has new messages".

Personally I quit the app when it happens to me because it's really annoying.

The option to control that behavior should be enough, with a secondary option to execute that method only when there's a video playing in full screen or something would be ideal

@nicofirst1
Copy link

Any update on this?

@ilya-fedin
Copy link
Contributor

It seems @john-preston doesn't want this

@EPecherkin
Copy link

Exactly as @fguille94 said. I really need this feature. It's so annoying to use the app without it
@john-preston Why is it still not there?

@fguille94
Copy link

to this day I still rather close the whole application just to not have the permanent-when-idle notification staring at my AFK face.

we seriously need this feature implemented ASAP

@ilya-fedin
Copy link
Contributor

I finally submitted the code from #10043 (comment) as #25570 (I would do that earlier, but no one here expressed the interest, now someone from another place expressed the interest).

@ilya-fedin
Copy link
Contributor

@levnikmyskin do you still need this setting in v4.5?

@levnikmyskin
Copy link
Author

@levnikmyskin do you still need this setting in v4.5?

honestly yes. To me, permanent notifications are annoying in many more cases, such as watching a youtube video (not in fullscreen) or reading stuff.
In all honesty, I've never really understood all this fuss for a single checkbox (which could also be hidden if one is using system notifications). That said, it's also the second anniversary of this pull request and it doesn't look like it's ever going to be merged :/ should we close it or leave it open?

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Dec 31, 2022

I've never really understood all this fuss for a single checkbox (which could also be hidden if one is using system notifications)

Apparently to avoid feature creep. And the current layout of the notification settings is received from designer's mockups, so it may be that the set of the settings is carved in stone and no community-asked settings could be added there. The only place having less restrictions is the Experimental settings page.

@sstyle
Copy link

sstyle commented Feb 13, 2023

2 years for so simple and useful feature?? Really?

@ilya-fedin
Copy link
Contributor

@sstyle read the discussion

@fguille94
Copy link

would adding the setting to Experimental settings be enough to merge the PR?

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Feb 13, 2023

Well, this PR seem to mix spaces with tabs and adds new logic for hiding instead of using the pre-existing one, so I guess the answer is no. It also conflicts with the current dev branch.

@AndriiDemchenko
Copy link

Seems like on windows 10 works fine now. Notification disappears after short time while playing something in fullscreen

@ilya-fedin
Copy link
Contributor

@levnikmyskin you may want to join https://t.me/tdesktop_contributions

@levnikmyskin
Copy link
Author

Thanks for the invitation @ilya-fedin. It seems to be a mostly russian-speaking telegram group. Despite my nickname, I don't speak any russian though :D

@ilya-fedin
Copy link
Contributor

It's ok to speak English there. Most members understand English just fine and @john-preston is generally more active in Telegram than GitHub.

@St0RM53
Copy link

St0RM53 commented Apr 16, 2024

Still waiting for this..

@iQiexie
Copy link

iQiexie commented Apr 25, 2024

same

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

Successfully merging this pull request may close these issues.