MusicPlayerDaemon / mpc

Command-line client for MPD
GNU General Public License v2.0
191 stars 36 forks source link

Support partition commands. #78

Closed ieure closed 2 years ago

ieure commented 2 years ago

This commit adds support for MPD’s partition feature to mpc.

Tested locally and it seems to work fine. But, it's been over 20 years since I wrote C regularly, so it's exceedingly possible that I've done something ignorant in here. I'm happy to correct whatever gets pointed out.

ieure commented 2 years ago

Pinging on this. Are the changes acceptable now, or do you want further changes?

MaxKellermann commented 2 years ago

Sorry for the delay, I've had a very busy time at dayjob.

MaxKellermann commented 2 years ago

Doesn't compile:

../../src/output.c: In function ‘cmd_moveoutput’:
../../src/output.c:300:10: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
  300 |     name = mpd_output_get_name(output);
      |          ^
../../src/output.c:284:20: error: unused parameter ‘argc’ [-Werror=unused-parameter]
  284 | cmd_moveoutput(int argc, char **argv, struct mpd_connection *conn)
      |                ~~~~^~~~
ieure commented 2 years ago

Doesn't compile:

Fixed. Those didn't fail the compile for me, they were warnings only. But I agree that it's undesirable to introduce new warnings.

ieure commented 2 years ago

Pinging on this again. I think this is in good shape, but please let me know if you want other changes.

MaxKellermann commented 2 years ago

Sorry, I'm busy on dayjob, my response times are even longer than usual. Your PR fails to build with old libmpdclient versions (down to 2.16 is supported).

../../src/main.c: In function 'run':
../../src/main.c:305:8: error: implicit declaration of function 'mpd_run_switch_partition'; did you mean 'mpd_run_sticker_set'? [-Werror=implicit-function-declaration]
   if (!mpd_run_switch_partition(conn, options.partition)) {

When you do fixups in a PR that is not yet merged, please amend existing commits.

ieure commented 2 years ago

Sorry, I'm busy on dayjob, my response times are even longer than usual. Your PR fails to build with old libmpdclient versions (down to 2.16 is supported).,

2.16 was released in 2018 and doesn't support partition commands, which first appeared in 2.17.

So, while it would build if I wrapped literally every change with #if LIBMPDCLIENT_CHECK_VERSION(2, 17, 0) … #endif, none of the partition stuff would work, or be visible in the resulting binary. Also, it would be horribly ugly.

Do you prefer that I litter the code with cpp conditionals, or bump the library dependency to 2.17?

If it's a requirement is that support for a four-year-old library is maintained, that should be enforced by the CI checks. They're currently targeting 2.18, since that's the version shipped with Ubuntu 20.04 Focal, released in 2020.

MaxKellermann commented 2 years ago

There are currently compile-time checks for libmpdclient 2.17 and 2.18. I agree to raise the minimum version to 2.18 and remove all checks. Please make that one new commit, inserted before the one that (one commit that raises the minimum version in meson.build and documentation and removes the checks). It should precede commits that use 2.17+ features.

ieure commented 2 years ago

@MaxKellermann This is rebased and commits combined, I believe it's ready.