ravachol / kew

A command-line music player
GNU General Public License v2.0
567 stars 21 forks source link

adding libnotify to the project, updating code and README #143

Closed kazemaksOG closed 2 months ago

kazemaksOG commented 2 months ago

See #142

Replaced execv with a more proper usage of the library. On the downside, this library now needs to be added to the project.

Main reason for this is that notifications flood (my) notification bar, so this ensures that only one notification is used and updated.

The layout also slightly changed, since the library doesn't seem to allow NULL or "" to be set as the summary (from what i tested), so I split it into artist and track name.

Previous: image

Now: image

I am unaware of how this works on different setups (mine is arch + wayland), so perhaps this is not as visually appealing as it is for me.

There is a an alternative way of making this work by using excev, since the notify-send can return the replacment-id of the notification, which can then be used to update it. This would be a bit more of a barbaric solution.

ravachol commented 2 months ago

Thank you! This looks much better than the previous solution when browsing through music. If people want it as an optional thing, we could perhaps add that in later.

xplshn commented 2 months ago

Will it be optional at build-time or at run-time? I don't have libnotify and don't intend on adding it to my system...

ravachol commented 2 months ago

Build-time. Would it work for you if you'd have to type:

make USE_LIBNOTIFY=0

or is there a better solution? I'll document it so that package maintainers can add that option.

xplshn commented 2 months ago

Can't the Makefile also use pkg-config/pkgconfig to check if the user has libnotify-dev installed?

ravachol commented 2 months ago

I think it can be done like this:

PKG_CONFIG ?= pkg-config

ifeq ($(origin USE_LIBNOTIFY), undefined) ifneq ($(shell $(PKG_CONFIG) --exists libnotify && echo yes),yes) USE_LIBNOTIFY = 0 else USE_LIBNOTIFY = 1 endif endif

If you don't set it explicitly, it defaults to whether you have libnotify installed. Maybe not super useful but...

ravachol commented 2 months ago

I have committed some changes regarding this to develop.

ravachol commented 2 months ago

The makefile was missing, I've added it now.