eonpatapon / mpDris2

MPRIS V2.1 support for mpd
GNU General Public License v3.0
211 stars 46 forks source link

Add many new options for notifications #156

Closed Barbaross93 closed 2 years ago

Barbaross93 commented 2 years ago

This primarily adds the ability to format the notification's summary and body for either the paused or playing states. Additionally, an option to enable/disable notifications when the player is paused and an option for notification timeout have been added.

The only remaining issue I have is that the format strings are in the style of %%...%% instead of mimicking mpc's style of %...%. From what I've gathered, I'd need to disable interpolation in ConfigParser() but doing so removes the convenience of detecting subsections. I'm not sure if what I have currently is good enough or if it's worth the trouble of trying to extract the values without interpolation.

Well that turned out easy.

This PR mostly implements https://github.com/eonpatapon/mpDris2/issues/148

imsamuka commented 2 years ago

It doesn't implements timeout yet. Adding self._notification.set_timeout(params['timeout']) inside NotifyWrapper.notify should work. But you need to change examples/defaults and comply with https://lazka.github.io/pgi-docs/#Notify-0.7/constants.html I also suggest to set the timeout default to Notify.EXPIRES_DEFAULT, it's a better default than Notify.EXPIRES_NEVER.

Barbaross93 commented 2 years ago

@imsamuka I'm not sure how I messed that up, but I most definitely had self._notification.set_timeout(params['timeout']) in there. No idea how I removed it. Regardless, thanks for catching that!

When you say to set the default timeout to Notify.EXPIRES_DEFAULT do you mean to use that constant literally? Because apparently -1 is the value of Notify.EXPIRES_DEFAULT and is therefore already there. The documentation I had was simply incorrect.

Barbaross93 commented 2 years ago

@eonpatapon I'm not going to lie, I think I bit off more than I can chew with trying to implement mpc's formatting. I think @imsamuka's re-factoring makes this PR much more attractive for merging. If you're ok with a future PR for mpc style formatting, I'll go ahead and add default values to the format_strings dictionary. This PR will be ready for merging then!

imsamuka commented 2 years ago

When you say to set the default timeout to Notify.EXPIRES_DEFAULT do you mean to use that constant literally? Because apparently -1 is the value of Notify.EXPIRES_DEFAULT and is therefore already there. The documentation I had was simply incorrect.

I was referring to the documentation, but you already solved it.

eonpatapon commented 2 years ago

Tested it and looks like it's ready to merge. I'll merge this tomorrow if nobody complains in the meantime.

Barbaross93 commented 2 years ago

@imsamuka done! @eonpatapon I also squashed everything to a single commit to keep things atomic.

eonpatapon commented 2 years ago

Thanks @Barbaross93 !