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

feat: add Linux support for Live Notifications toasts w/ libnotify #5881

Merged
merged 2 commits into from
Feb 8, 2025

Conversation

camporter
Copy link
Contributor

Add libnotify notification support for channels going live. Would be nice to have notifications for other things as well, but this is just adding an equivalent for wintoast.

Disabled QT keywords to not clash with glib 😞 there's probably a better way to do this.

Fixes #5782

There's a lot of testing that needs to be done and build concerns, so leaving as a draft.

@Nerixyz
Copy link
Contributor

Nerixyz commented Feb 1, 2025

Disabled QT keywords to not clash with glib 😞 there's probably a better way to do this.

Could you do this in another PR? This makes the diff way larger than it actually is. You can remove emits (imo) - they're cosmetic and don't do anything.

@pajlada
Copy link
Member

pajlada commented Feb 2, 2025

Hi! Thanks for your initial PR, I just wanted to let you know that the use of libnotify seems like a good idea from my perspective.

@8thony
Copy link
Contributor

8thony commented Feb 2, 2025

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

src/singletons/Toasts.cpp Outdated Show resolved Hide resolved
src/singletons/Toasts.cpp Outdated Show resolved Hide resolved
@killertofus
Copy link
Contributor

the aur version of chatterino will also need to include libnotify as a dep then

@camporter
Copy link
Contributor Author

Thanks, happy to provide a PKGBUILD patch but it should just be libnotify in the deps.

Missed nixos in the docs so updated.

@camporter
Copy link
Contributor Author

I have some Highlight -> Toast behavior done
image

but that should probably wait for the live notification and build issues to be resolved first.

@pajlada
Copy link
Member

pajlada commented Feb 5, 2025

@camporter I would prefer if desktop notifications for highlights is not added in this PR - It's too much of a separate feature. I'm currently looking into changing the way highlights are modified (more options means more checkboxes in the current way of doing things, and that gets really unwieldy)

If you want to add some way to test desktop notifications easier, feel free to add an additional branch to the /debug-test command

@camporter camporter force-pushed the libnotify_support branch 2 times, most recently from 27d6f18 to 898d901 Compare February 6, 2025 02:16
@camporter
Copy link
Contributor Author

Thanks, added a debug option for notifications and tried a variety of random channels.
Screenshot From 2025-02-05 19-38-38

Remaining things:

  • The linux builds I believe are breaking because the container images will need an update.
  • For freebsd, considering leaving libnotify off..

@camporter camporter marked this pull request as ready for review February 6, 2025 02:19
@Wissididom
Copy link
Contributor

For the container update you could make a PR in https://github.com/Chatterino/docker where you update Dockerfile_chatterino2-build-ubuntu-20.04, Dockerfile_chatterino2-build-ubuntu-22.04 and Dockerfile_chatterino2-build-ubuntu-24.04. I think from what I've read in the error message that it might be enough to just install libnotify or smth in those container images. After pajlada than merges the PR in Chatterino/docker and you wait until the build on master is complete, the new docker images should be available for pulling/in the workflow.

@camporter
Copy link
Contributor Author

camporter commented Feb 7, 2025

Ah thanks, confused the separate repo with the .docker dir. Opened a PR over there 🙂

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.

Small nits, otherwise seems good

CHANGELOG.md Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@Nerixyz Nerixyz left a comment

Choose a reason for hiding this comment

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

I wonder if we could make our own small notification library (I'd be fine with a submodule here). Implementing this for macOS doesn't seem to be that complicated either (KNotifications impl - ObjectiveC might be the hardest part here). Maybe after EventSub is in a decent state.

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/singletons/Toasts.cpp Outdated Show resolved Hide resolved
Comment on lines 39 to 43
#elif defined(LIBNOTIFY)
void ensureInitialized();
void sendLibnotify(const QString &channelName, const QString &channelTitle);

bool initialized_ = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could ensureInitialized and initialized_ be defined in a single block? They're the same on Windows and with libnotify. Maybe the send* functions could be united as well. Something like sendPlatformNotification? It would include the platform for now (#5914).

Copy link
Member

Choose a reason for hiding this comment

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

Probably best to leave for follow-up PR, or for whenever notifications are touched again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left these separate just to avoid getting into too much rework here, but combining them or moving them into a library seems good. The macos notification stuff required signing and other conditions that I didn't wade into. I don't boot windows up very often to test there either 😆

For linux/freebsd, you could also avoid libnotify and use dbus directly, but a bit more involved to do.

@pajlada
Copy link
Member

pajlada commented Feb 8, 2025

Looks like you can apply this patch to fix the FreeBSD build, and it also just looks simpler

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 24ddbe7b..05a83ebd 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -1065,10 +1065,8 @@ if (UNIX)
 
     if (NOT APPLE AND BUILD_WITH_LIBNOTIFY)
         find_package(PkgConfig REQUIRED)
-        pkg_check_modules(LIBNOTIFY REQUIRED libnotify)
-        include_directories(${LIBRARY_PROJECT} ${LIBNOTIFY_INCLUDE_DIRS})
-        target_link_libraries(${LIBRARY_PROJECT} PRIVATE ${LIBNOTIFY_LIBRARIES})
-        target_link_directories(${LIBRARY_PROJECT} PRIVATE ${LIBNOTIFY_LIBRARY_DIRS})
+        pkg_check_modules(LIBNOTIFY REQUIRED IMPORTED_TARGET libnotify)
+        target_link_libraries(${LIBRARY_PROJECT} PRIVATE PkgConfig::LIBNOTIFY)
         target_compile_definitions(${LIBRARY_PROJECT} PUBLIC CHATTERINO_WITH_LIBNOTIFY)
     endif ()
 endif ()

@pajlada pajlada changed the title Initial libnotify support feat: add Linux support for Live Notifications toasts w/ libnotify Feb 8, 2025
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.

Notifications works for me, valgrind doesn't complain about anything (new) - Thank you!

@pajlada pajlada enabled auto-merge (squash) February 8, 2025 18:36
@pajlada pajlada merged commit 2656fd0 into Chatterino:master Feb 8, 2025
16 checks passed
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.

Notifications don't work on Ubuntu 22.04 lts
6 participants