Conversation
|
Hi, thanks for this, I will find some time to review on the weekend, but looks pretty good at a glance. |
|
Great! Appreciate it. |
|
Please split the taskbar fix off to a separate PR, other comments inline. |
internal/dbus/idle_inhibitor.go
Outdated
| if i.fd == 0 { | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
| if i.fd == 0 { | |
| return false | |
| } | |
| return true | |
| return i.fd != 0 |
| } | ||
| } | ||
| } | ||
|
|
internal/dbus/idle_inhibitor.go
Outdated
| return err | ||
| } | ||
| return nil | ||
|
|
|
|
||
| } | ||
|
|
||
| func (i *idleInhibitor) toggle() error { |
There was a problem hiding this comment.
This widget may exist multiple times, or on multiple panels. Rather than toggling based local state for each widget instance, state should be propagated to the widget via an event on state update (push to the internal event bus from the dbus module would be easiest, see the Power or Audio modules for examples), otherwise widgets will be out of sync.
There was a problem hiding this comment.
Didn't think about that, I will fix it)
internal/dbus/idle_inhibitor.go
Outdated
| return true | ||
| } | ||
|
|
||
| func (i *idleInhibitor) String() string { |
There was a problem hiding this comment.
Yes, I wanted to make a menu with options for each target for inhibiting, but didn't have enough time.
|
|
||
| func (i *idleInhibitor) Inhibit(target eventv1.InhibitTarget) error { | ||
| var what string | ||
| switch target { |
There was a problem hiding this comment.
Can you explain the difference in string values between these cases and the previous String() method?
Also, prefer const declarations for these string values, rather than inline value declarations.
There was a problem hiding this comment.
sure, I thought it would be interesting if I add all possible inhibit targets: shutdown, sleep and idle. "idle" option prevents from locking and suspending, "sleep" from suspend only, and "shutdown" is the strongest option, which as I read, doesn't let to poweroff and reboot.
The String() method I thought would serve as a label for the options of inhibitor, and also use as arguments for org.freedesktop.ScreenSaver.Inhibit method, which as it turned out, doesn't affect hypridle.
| eventCh: eventCh, | ||
| signals: make(chan *dbus.Signal), | ||
| readyCh: make(chan struct{}), | ||
| quitCh: make(chan struct{}), |
There was a problem hiding this comment.
Since we have an fd to clean up, this will need to be closed if the dbus manager is closed. I'm afraid you may have inherited the lack of cleanup behaviour from one of the other dbus modules in this codebase since a number of of those do not handle close correctly (I have a change on my local dev branch to clean up how this is managed, but for now check statusNotifierWatcher#close() for an example of how this might be done currently).
proto/hyprpanel/event/v1/event.proto
Outdated
| enum InhibitTarget { | ||
| INHIBIT_TARGET_UNSPECIFIED = 0; | ||
| INHIBIT_TARGET_IDLE = 1; | ||
| INHIBIT_TARGET_SUSPEND = 2; |
There was a problem hiding this comment.
These names should probably match the DBUS targets?
There was a problem hiding this comment.
yes, they matched values for org.freedesktop.ScreenSaver.Inhibit which did not work
style/style.go
Outdated
| // HudOverlayID element identifier. | ||
| HudOverlayID = `hudOverlay` | ||
| // IdleID element identifier. | ||
| IdleInhibitorID = `idle_inhibitor` |
cmd/hyprpanel/host.go
Outdated
| } | ||
|
|
||
| func (h *host) IdleInhibitorToggle(target eventv1.InhibitTarget) error { | ||
| if i := h.dbus.IdleInhibitor(); i.IsActive() { |
|
Your feedback was very helpful, I will fix the issues you mentioned and comeback. Thanks! |
|
Hi! I fixed the issues with state propagation and added a popup menu to choose any of the available inhibit targets |
2 participants
Uh oh!
There was an error while loading. Please reload this page.