Skip to content

feat: globally suppress highlight notifications#5629

Draft
jupjohn wants to merge 14 commits intoChatterino:masterfrom
jupjohn:feature/5628-globally-suppress-notifications
Draft

feat: globally suppress highlight notifications#5629
jupjohn wants to merge 14 commits intoChatterino:masterfrom
jupjohn:feature/5628-globally-suppress-notifications

Conversation

@jupjohn
Copy link
Contributor

@jupjohn jupjohn commented Oct 6, 2024

This PR aims to implement global suppression of highlight notifications through a "Do Not Disturb"-like feature. I wanted to get this out of my head before I lost interest in it.

TODO:

  • Allow for separate key binds to toggle on/off (just through arguments)
  • Implement notebook tab/split header toggle
  • Add some kind of visual indicator (might de-scope, will discuss)

Implements #5628.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@jupjohn
Copy link
Contributor Author

jupjohn commented Oct 11, 2024

Tested the following against 271d57e (pre-rebase) and works as expected:

Hotkey usage:

  1. Add a hotkey for global notification suppression
  2. Ping self (notifies)
  3. Use hotkey
  4. Ping self (doesn't notify)
  5. Use hotkey
  6. Ping self (notifies)

Window context menu checkbox connection:

  1. With the feature toggled off, open the notebook's context menu by right clicking
  2. Toggle is unchecked
  3. Refocus window, use hotkey
  4. Open context menu, toggle is checked
  5. Refocus window, use hotkey
  6. Open context menu, toggle is unchecked

Window context menu checkbox action:

  1. Ensure the toggle is disabled
  2. Ping self (notifies)
  3. Toggle notification suppression on in notebook's context menu
  4. Ping self (doesn't notify)
  5. Toggle notification suppression off in notebook's context menu
  6. Ping self (notifies)

@jupjohn
Copy link
Contributor Author

jupjohn commented Oct 11, 2024

Would anyone be against me NOT implementing visual feedback to indicate the state of notification suppression (i.e. show an icon next in the notebook header like stream mode?) It would be nice from a user-perspective, but I'd rather get the core functionality in first.

@jupjohn jupjohn force-pushed the feature/5628-globally-suppress-notifications branch from 271d57e to 582f452 Compare October 11, 2024 20:50
@jupjohn jupjohn marked this pull request as ready for review October 11, 2024 20:52
Comment on lines 358 to 370
{"toggleGlobalNotificationSuppression",
ActionDefinition{
.displayName = "Toggle muting of all notifications",
.argumentDescription = "[on, off, or toggle. default: toggle]",
.minCountArguments = 0,
.maxCountArguments = 1,
.possibleArguments{{"Toggle", {}},
{"Enable notification muting", {"on"}},
{"Disable notification muting", {"off"}}},
.argumentsPrompt = "New value:",
.argumentsPromptHover = "Should all highlight notifications be "
"enabled, disabled, or toggled.",
}},
Copy link
Contributor

@Nerixyz Nerixyz Oct 11, 2024

Choose a reason for hiding this comment

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

Bikeshed: Do we want this as a "negative" option? "Disable notification muting" is (kind-of) a double-negative - you're enabling notifications.

Oh, also: what about live notifications? "toggleGlobalNotificationSuppression" sounds like it would disable everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Happy to run with "mute notifications" and "unmute notifications" 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted in 513e190

@jupjohn
Copy link
Contributor Author

jupjohn commented Dec 16, 2024

I'll be implementing the visual feedback in this PR as it's not being included in 2.5.2 (discussed on pajlada's stream a while back)

@jupjohn jupjohn force-pushed the feature/5628-globally-suppress-notifications branch from 513e190 to 1319caa Compare December 16, 2024 06:32
@jupjohn jupjohn marked this pull request as draft December 16, 2024 06:32
@jupjohn
Copy link
Contributor Author

jupjohn commented Dec 16, 2024

Scuffed GIMP design, how do we feel about this icon when this feature is enabled?

image

I originally went with a filled red circle, but it's too much IMO.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// do not disturb
this->doNotDisturbIcon_ = this->addCustomButton();
QObject::connect(this->doNotDisturbIcon_, &NotebookButton::leftClicked,
[this] {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: lambda capture 'this' is not used [clang-diagnostic-unused-lambda-capture]

Suggested change
[this] {
[] {


// do not disturb
this->doNotDisturbTitlebarIcon_ =
this->addTitleBarButton(TitleBarButtonStyle::DoNotDisturb, [this] {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: lambda capture 'this' is not used [clang-diagnostic-unused-lambda-capture]

Suggested change
this->addTitleBarButton(TitleBarButtonStyle::DoNotDisturb, [this] {
this->addTitleBarButton(TitleBarButtonStyle::DoNotDisturb, [] {

@Nerixyz
Copy link
Contributor

Nerixyz commented Dec 16, 2024

how do we feel about this icon when this feature is enabled?

On Windows, this is a bad idea. There's almost no space to drag the window when streamer mode is enabled and do-not-disturb is enabled:

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.

2 participants

Comments