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

IPC Updates and Minor Bugfies (Revised) #39

Closed
wants to merge 4 commits into from

Conversation

corigne
Copy link

@corigne corigne commented Oct 18, 2023

Replaces PR ticket #38 as it was particularly unreadable due to formatting choices I initially made.
I went back and revised my changes into a new fork. Having a moment to carefully pick through my original additions, I have revised them a decent amount as well, including improving the new polybar-msg routine error handling.

I was careful this time to avoid making formatting changes which might cause an excessive diff. Thank you for the feedback! Please try this fork and let me know if anything needs to be fixed/improved.

The following major changes are proposed for merge:

  • Updated IPC method to align with with polybar version 3.6 IPC changes which require the use of polybar-msg.
    Addresses Writing directly to the named pipe to send IPC messages has been deprecated #36 .
  • Added an initialization check using gio (dbus) during which the listener service can now detect Spotify on service launch or restart.
  • Add dependency of glib2, which provides gobject and gio, which enables the initialization checks. This can be reconstructed to work within low level dbus, I think, which would avoid the dependency but will be much more verbose.*
  • Added a verbose debug statement to the name change handler and changed the name change handler to allow it to detect spotify taking control of the dbus interface. Because new_owner is the dbus sender id, we can also set that as soon as spotify starts instead of just on properties change.

Bug-fixes:

  • Fixed a bug which prevented the module hooks from running until the track was changed at least once.
  • Fixed a bug where only only some hooks were updated on property change. At launch, in combination with the other bug resulted in a partially rendered module until the user executed all 3 types of playback actions (Play/Pause, Next, Previous)

*edited

@corigne
Copy link
Author

corigne commented Dec 16, 2023

There are some new side effects I'm getting from the changes that only recently cropped up, such as "%output %output %output %output" in the status bar on boot, which weren't happening before. Will look into this new bug and see if I can't sort it, and then I'll update the PR.

@corigne
Copy link
Author

corigne commented Feb 9, 2024

I've identified the cause of the output issues. They are actually unrelated to the changed proposed in this PR.

Polybar 3.7 added new functionality to the IPC module configuration which results in empty hook defaulting to labels printing %output%, even when echoing an empty string.

To switch back to the output formatting it must now be explicitly stated in the configuration.
Note line 3 of the example below from my configuration:

[module/spotify]
type = custom/ipc
format = <output>
format-font = 5
format-prefix = "<ICON HERE> "
format-prefix-font = 4
format-prefix-foreground = ${colors.active}
; Default
hook-0 = echo
; Playing/paused show song name and artist
hook-1 = spotifyctl status --format '%artist% - %title%' --max-title-length 25 --max-artist-length 25 --trunc '...'

I will add read-me changes identifying the fix for this.
Based on previous changes from polybar I will keep an eye out for output type format labels deprecating. 🥲

Reference: https://github.com/polybar/polybar/wiki/Module:-ipc

@corigne
Copy link
Author

corigne commented Feb 9, 2024

README changes added in 2545bec

@corigne corigne deleted the branch mihirlad55:master February 9, 2024 17:28
@corigne corigne closed this Feb 9, 2024
@corigne corigne deleted the master branch February 9, 2024 17:28
@corigne
Copy link
Author

corigne commented Feb 9, 2024

Please refer to: #40 to comment. This PR has all of the details regarding that PR.

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.

1 participant