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

statusicon-qt: Set proper tooltip #147

Conversation

radioactiveman
Copy link
Member

@radioactiveman radioactiveman commented Dec 29, 2023

Even though we override the QSystemTrayIcon::event() method,
it is never called by Qt (at least on Linux when the panel/DE
supports StatusNotifierItem). Therefore our custom info popup
is not shown. Set a proper tooltip as workaround.

@radioactiveman
Copy link
Member Author

@jlindgren90: Why is the title_changed() method never called? What am I missing here? It is registered to the hook and there is no typo.

@jlindgren90
Copy link
Member

@jlindgren90: Why is the title_changed() method never called? What am I missing here? It is registered to the hook and there is no typo.

"title change" is for streaming when the title changes mid-stream. To get the initial title you can use "playback ready" (if memory serves).

@radioactiveman
Copy link
Member Author

Thanks, updated accordingly.

One more question: I wonder if there is a difference regarding number of memory allocations and preventing memory leaks between these options. Which would you choose?

const char * title = aud_drct_get_title ();
tray->setToolTip (title);
String title = aud_drct_get_title ();
tray->setToolTip ((const char *) title);
String title = aud_drct_get_title ();
tray->setToolTip (QString (title));

@radioactiveman radioactiveman marked this pull request as ready for review December 29, 2023 11:42
@jlindgren90
Copy link
Member

One more question: I wonder if there is a difference regarding number of memory allocations and preventing memory leaks between these options. Which would you choose?

const char * title = aud_drct_get_title ();
tray->setToolTip (title);
String title = aud_drct_get_title ();
tray->setToolTip ((const char *) title);
String title = aud_drct_get_title ();
tray->setToolTip (QString (title));

The first one is unsafe since the String will go out of scope too early (leaving a dangling pointer). The last two are equivalent so either one is fine.

@jlindgren90
Copy link
Member

Thanks for working on this. Have you checked how this works with a panel/desktop environment supporting only XEmbed? Do we end up showing a double tooltip/info popup in that case?

@radioactiveman
Copy link
Member Author

Thanks for working on this. Have you checked how this works with a panel/desktop environment supporting only XEmbed? Do we end up showing a double tooltip/info popup in that case?

I have tested it with Xfce and GNOME. Xfce has shown a tooltip also before ("Audacious") which is rather useless. GNOME with this extension neither shows a tooltip nor the info popup. Next week I can test how macOS behaves.

Which panel / DE do you use? Could you please test it yourself too?

@radioactiveman
Copy link
Member Author

@bpanca05: In your bug report I have seen you use KDE and also enabled the status icon plugin. Could you please answer my questions and try the patch from this pull request?

  • Does KDE show the info popup like in my screenshot below, a general tooltip like "Audacious" or no tooltip at all?
  • Does changing the volume work with KDE when using the scroll wheel above the icon?
  • Does KDE show both the info popup and a tooltip with the patch applied (worse behavior than before)?

Thanks for feedback.

info-popup

@jlindgren90
Copy link
Member

To test XEmbed, try lxpanel (the old GTK version, not lxqt-panel). Here it shows a double tooltip:
image

@jlindgren90
Copy link
Member

This seems to fix the double-tooltip issue:

diff --git a/src/statusicon-qt/statusicon.cc b/src/statusicon-qt/statusicon.cc
index e22bbe143..c976b3bde 100644
--- a/src/statusicon-qt/statusicon.cc
+++ b/src/statusicon-qt/statusicon.cc
@@ -164,7 +164,10 @@ bool SystemTrayIcon::event (QEvent * e)
     {
     case QEvent::ToolTip:
         if (! aud_get_bool ("statusicon", "disable_popup"))
+        {
+            setToolTip(QString()); /* prevent double tooltip */
             audqt::infopopup_show_current ();
+        }
         return true;
 
     case QEvent::Wheel:

Even though we override the QSystemTrayIcon::event() method,
it is never called by Qt (at least on Linux when the panel/DE
supports StatusNotifierItem). Therefore our custom info popup
is not shown. Set a proper tooltip as workaround.
@radioactiveman radioactiveman changed the title statusicon-qt: Set basic tooltip statusicon-qt: Set proper tooltip Dec 31, 2023
@radioactiveman
Copy link
Member Author

Thanks John for testing and the double tooltip fix. Meanwhile I have tried it on macOS where the tooltip is shown now.

Happy New Year! 🎆

@radioactiveman radioactiveman merged commit 3415689 into audacious-media-player:master Dec 31, 2023
8 checks passed
@bpanca05
Copy link

bpanca05 commented Jan 1, 2024

@bpanca05: In your bug report I have seen you use KDE and also enabled the status icon plugin. Could you please answer my questions and try the patch from this pull request?

* Does KDE show the info popup like in my screenshot below, a general tooltip like "Audacious" or no tooltip at all?

* Does changing the volume work with KDE when using the scroll wheel above the icon?

* Does KDE show both the info popup and a tooltip with the patch applied (worse behavior than before)?

Thanks for feedback.

info-popup

Sorry for the late response.

  1. It's only show general tooltip like "Audacious"
  2. Nope, it's doesn't work
  3. It's only show general tooltip like "Audacious" (Nothing changed after this commit merged)

image

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.

3 participants