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: special shortcut for Windows live notifications #5975

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

8thony
Copy link
Contributor

@8thony 8thony commented Feb 22, 2025

Fixes #5954

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

@8thony
Copy link
Contributor Author

8thony commented Feb 22, 2025

@Nerixyz there might be a bug with pinning c2 to the taskbar.

Steps
  • Installed
  • Run (from installer or the direct exe file)
  • Pin to taskbar
  • Login / configure a live notification
  • Close and Reopen from taskbar
  • Get a live notification (the name will be "Chatterino" but a new Shortcut will be created in %appdata%\...)
  • Close and reopen from taskbar once more
  • Get a live notification (the name is now "Chatterino2" and on the taskbar c2 is a new separate "window")

-> Tested the Taskbar shortcut, and it don't include the AUMI

Questions:
Do we need to set the AUMI for the exe file as well? (I don't know how to do this, and if it solves the issue)
Should we change the name of the App here?
Should we include the AUMI in the Installer created shortcut at all?
Is this change becoming more painful and not much useful at all?

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

We can just change all AUMI/AppNames to "Chatterino", feel free to do that in this PR - I'm assuming that should not break anything. Rather that be changed now than before we start creating shortcuts

Comment on lines +54 to +61
settings.append(this->createCheckBox(
"Create start menu shortcut (requires "
"restart)",
getSettings()->createShortcutForToasts,
"When enabled, a shortcut will be created inside your "
"start menu folder if needed by live notifications."
"\n(On portable mode, this is disabled by "
"default)"));
Copy link
Member

Choose a reason for hiding this comment

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

You can use SettingWidget here, which means you wouldn't have to update the createCheckBox implementation - see

SettingWidget::dropdown("Show blocked term automod messages",
s.showBlockedTermAutomodMessages)
->setTooltip("Show messages that are blocked by AutoMod for containing "
"a public blocked term in the current channel.")
->addTo(layout);
as a reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on the NotificationPage i can't do ->addTo(layout) using SettingWidget::checkbox
What do i need to change?

@Nerixyz
Copy link
Contributor

Nerixyz commented Feb 23, 2025

Should we change the name of the App here?

This worked for me. I'd advocate for this.


Honestly, I now think this should be disabled by default. From what we know: installations with the installer (probably the majority) aren't affected. But we add a shortcut there by default. In Chatterino7, for example, the created shortcut is named Chatterino7.lnk. With this PR, an additional shortcut (Chatterino.lnk) would be created. But that's not necessary (could also change the app name there, I suppose). It's really only for portable users (or users of cmake --install).

@8thony
Copy link
Contributor Author

8thony commented Feb 23, 2025

installations with the installer (probably the majority) aren't affected

Does it show the correct name and icon for you on stable 2.5.2 installed with its installer?
Remove all related shortcuts from here before installing.

- %AppData%\Roaming\Microsoft\Windows\Start Menu\Programs
- %ProgramData%\Microsoft\Windows\Start Menu\Programs

Should I then undo the changes to the installer and make the setting disabled by default?

@Nerixyz
Copy link
Contributor

Nerixyz commented Feb 23, 2025

Does it show the correct name and icon for you on stable 2.5.2 installed with its installer?

Hmm, no, it doesn't. The installer changes are good, though, I'm fine with them. I guess we could enable it by default... But we should definitely change the name that we give to WinToast then. Because the installer creates Chatterino.lnk, not Chatterino2.lnk.

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.

WinToast needs a special shortcut
3 participants