mpc icon indicating copy to clipboard operation
mpc copied to clipboard

Support partition commands.

Open ieure opened this issue 3 years ago • 8 comments
trafficstars

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 avatar Mar 05 '22 06:03 ieure

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

ieure avatar Mar 21 '22 17:03 ieure

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

MaxKellermann avatar Mar 26 '22 05:03 MaxKellermann

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)
      |                ~~~~^~~~

MaxKellermann avatar Apr 26 '22 17:04 MaxKellermann

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 avatar Apr 26 '22 23:04 ieure

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

ieure avatar Jun 06 '22 16:06 ieure

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.

MaxKellermann avatar Jun 13 '22 08:06 MaxKellermann

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.

ieure avatar Jun 13 '22 15:06 ieure

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.

MaxKellermann avatar Jun 13 '22 17:06 MaxKellermann

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

ieure avatar Aug 30 '22 18:08 ieure