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(#2989): (optional) hover for all modules #3145

Merged
merged 1 commit into from
Apr 20, 2024
Merged

feat(#2989): (optional) hover for all modules #3145

merged 1 commit into from
Apr 20, 2024

Conversation

haug1
Copy link
Contributor

@haug1 haug1 commented Apr 17, 2024

A second attempt at implementing hover for all modules in a much less intrusive way.

See #3139 also (the more naive implementation which I found a similar approach had been tried and reverted).

Let me know if any additional documentation is required to get this merged. Alternatively, I think this could just be merged directly exposing the functionality for the super users immediately and then improved upon later on.

@haug1
Copy link
Contributor Author

haug1 commented Apr 17, 2024

(Force-push removed debug log statements)

src/AModule.cpp Outdated Show resolved Hide resolved
@Alexays
Copy link
Owner

Alexays commented Apr 18, 2024

Nice approach, it might be the perfect workaround to have hover effect.
I would even remove the "hover" config and enable it for everyone, as it's just a class.

@Alexays
Copy link
Owner

Alexays commented Apr 18, 2024

Can you also add it to the man?

@haug1
Copy link
Contributor Author

haug1 commented Apr 18, 2024

Can you also add it to the man?

Thanks, I'll handle your feedback tonight when I have time!

@haug1
Copy link
Contributor Author

haug1 commented Apr 18, 2024

Made the following changes:

  • removed unnecessary include
  • removed configuration option, hover signals are attached by default
  • added documentation in waybar-styles man-page

@haug1 haug1 requested a review from Alexays April 18, 2024 14:27
@alebastr
Copy link
Contributor

Might be useful: after a short dive into the GTK internals, I figured out how to make the :hover pseudo-class work:

diff --git a/src/AModule.cpp b/src/AModule.cpp
index f941ab97..a285da77 100644
--- a/src/AModule.cpp
+++ b/src/AModule.cpp
@@ -87,12 +87,16 @@ auto AModule::doAction(const std::string& name) -> void {
 }
 
 bool AModule::handleMouseEnter(GdkEventCrossing* const& e) {
-  event_box_.get_style_context()->add_class("hover");
+  if (auto* module = event_box_.get_child(); module != nullptr) {
+    module->set_state_flags(Gtk::StateFlags::STATE_FLAG_PRELIGHT);
+  }
   return true;
 }
 
 bool AModule::handleMouseLeave(GdkEventCrossing* const& e) {
-  event_box_.get_style_context()->remove_class("hover");
+  if (auto* module = event_box_.get_child(); module != nullptr) {
+    module->unset_state_flags(Gtk::StateFlags::STATE_FLAG_PRELIGHT);
+  }
   return true;
 }
 

Important part of the trick is that GtkEventBox does not propagate the state flags to the child widgets, so you have to apply the PRELIGHT (:hover) state flag directly to the module widget.

@haug1
Copy link
Contributor Author

haug1 commented Apr 19, 2024

I could attempt @alebastr's solution if that is preferrable. It looks good to me, I don't know, maybe it's a bit more elegant.

If my understanding is correct, I think the CSS would look like this instead, which I think shouldn't be breaking any configurations out in the wild, although I guess it's more likely that some people have failed attempts at applying hover like this in their configs:

#pulseaudio:hover {
  // styles
}

@haug1
Copy link
Contributor Author

haug1 commented Apr 19, 2024

I jumped the gun and applied those changes. I think it's much more elegantly done like this. We get the normal CSS :hover and applied on the correct GTK component. Kudos to @alebastr for digging into GTK a lot better than me! 👍

@Alexays Alexays merged commit 87cc40e into Alexays:master Apr 20, 2024
9 checks passed
@Alexays
Copy link
Owner

Alexays commented Apr 20, 2024

🔥

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