[Feature] Add local cover art when importing tracks

[poetaster]

This PR contains 3 primary changes all related to obtaining local cover art, especially when cover art cannot be otherwise found.

1. Cleans up the GenericCacheLocation use in favour of the application specific CacheLocation.
2. Ensures the location exists (to be exact, that the sub-directory media-art exists) and enforces its use with Sailjail.
3. Adds a method to datareader.cpp to check, if album and artist are known, if a file's containing directory contains a jpg/jpeg and if so, copy it to the album art cache.

[Olf0]

@poetaster, thank you very much for your contribution.
This is looking good AFAICT, though I do have some minor questions, which are likely due to my limited programming experience and know-how.

P.S. / just whining: I start to understand why reviewers want a PR for each small coherent change. Never mind, this changeset (which comprises all three points in the description) is definitely not large.

P.P.S.: Extra thanks for having performed an lupdate run.

P.P.P.S.: BTW, what does "a proof of concept method" in point 3 of the PR description mean? I would understand that sentence well, if these four words are omitted, but fail to fully comprehend the complete sentence with them. Does this code block really constitute a "method"? Do you mean that this code block is just a "proof of concept" (it seems to be fully functional and do what it is supposed to do, or are you thinking about adding extra functionality to it later?)?
Please expand or omit these four words according to your / their original intention and your liking.

[poetaster]

BTW, what does "a proof of concept method"

It's a method which works as intended, but as a proof of concept. Which means that it needs more work. It's not much, but I didn't want to overload the PR :) What is missing:

1. Don't repeat operation for every file in a directory (it's not 'harmful' to make the file more often, but wasteful).
2. Implement for artist image
3. check for multiple file types (png, jpg and jpeg) and common names (cover, front, etc.)

[Olf0]

BTW, what does "a proof of concept method"

It's a method which works as intended,

O.K., so "method" in generic speak, but not as in "an object comprises a method and data" in OO-programming, correct?

but as a proof of concept. Which means that it needs more work. It's not much, but I didn't want to overload the PR :)

Thank you, that is what I wanted to know.

So we have as to-do list:

What is missing:

1. Don't repeat operation for every file in a directory (it's not 'harmful' to make the file more often, but wasteful).

2. Implement for artist image

3. check for multiple file types (png, jpg and jpeg) and common names (cover, front, etc.)

[dcaliste]

Thank you very much @poetaster for the changes. I didn't have the opportunity to test yet. I will try next week. I have some suggestion on the current code, but it looks already quite good. Thanks again.

[poetaster]

BTW, what does "a proof of concept method"

It's a method which works as intended,

O.K., so "method" in generic speak, but not as in "an object comprises a method and data" in OO-programming, correct?

A method in the generic sense.

but as a proof of concept. Which means that it needs more work. It's not much, but I didn't want to overload the PR :)

Thank you, that is what I wanted to know.

So we have as to-do list:

What is missing:

1. Don't repeat operation for every file in a directory (it's not 'harmful' to make the file more often, but wasteful).

2. Implement for artist image

3. check for multiple file types (png, jpg and jpeg) and common names (cover, front, etc.)

3, in the course of correcting vis. dcaliste's suggestions is done.

[Olf0]

BTW, by …

So we have as to-do list:

… I meant to imply "for later", not for this PR.

---

IMHO the code additions are fine now, but @dcaliste has the concluding say on this.
BTW, my question WRT oFile is still open, see the corresponding review comment.

What I am still concerned about is the SailJail configuration per se (not its content), see review comment above.

[poetaster]

BTW, by …

So we have as to-do list:

… I meant to imply "for later", not for this PR.

IMHO the code additions are fine now, but @dcaliste has the concluding say on this. BTW, my question WRT oFile is still open, see the corresponding review comment.

What I am still concerned about is the SailJail configuration per se (not its content), see review comment above.

In the main, as with all migrations from pre to post sailjail, you have to choose a 'window' during which time the migration code is enabled and sailjail is not. I think (please correct me) that the users installing/using flowplayer now, are rarely in need of migrating the local database/cache (or are more than willing to just re-import).

The PR I made here is making the bold assumption that the migration phase is finished. In any case, combining the changes while still using the old cache file patterns is not possible. So, this PR either waits for the conclusion of the 'migration phase' or we say, swallow the bitter pill and just enable sailjail and the small extra feature. The latter I could enhance (ie, I'd like artist pics) later.

[dcaliste]

Thanks @poetaster for the changes. I've got some style comments I forgot to mentioned last review, and a minor concern with the newly added code.

[dcaliste]

I think the new feature (and the new media-art location) can go in first when the PR is ready. Then, the code should be ready for sandboxing, and the desktop file can be adjusted to reflect this. I'm in favour to separate the sandboxing changes into its own PR that can be merged later. That being said, it's not a major issue to merge both at the same time. @poetaster could separate it in a stand-alone third commit to this PR ? Thank you in advance.

[Olf0]

What I am still concerned about is the SailJail configuration per se (not its content), see review comment above.

In the main, as with all migrations from pre to post sailjail, you have to choose a 'window' during which time the migration code is enabled and sailjail is not.

Logically, but as already stated, I expect this "window" to comprise more than just a single release, which also was the first one not marked as "beta" or "RC".

I think (please correct me) that the users installing/using flowplayer now, are rarely in need of migrating the local database/cache (or are more than willing to just re-import).

I doubt the latter.

Anyway, I depicted some routes above in the flowplayer.desktop review, which depend on some technical aspects. But as I still do not really comprehend, why the SailJail config conflicts with which migration functions, I cannot determine which route is the best.

I could support some of these routes by creating an intermediate release soon, but would have to have an idea which route to take.

The PR I made here is making the bold assumption that the migration phase is finished. In any case, combining the changes while still using the old cache file patterns is not possible. So, this PR either waits for the conclusion of the 'migration phase' or we say, swallow the bitter pill and just enable Sailjail and the small extra feature. The latter I could enhance (ie, I'd like artist pics) later.

See above and for details the review comment. But I can assure you that we both, me and supposedly also @dcaliste want this PR in.

[poetaster]

What I am still concerned about is the SailJail configuration per se (not its content), see review comment above.

In the main, as with all migrations from pre to post sailjail, you have to choose a 'window' during which time the migration code is enabled and sailjail is not.

Logically, but as already stated, I expect this "window" to comprise more than just a single release, which also was the first one not marked as "beta" or "RC".

I don't see a reason to extend the 'migration' window for more than one release. What reason do you see?

I think (please correct me) that the users installing/using flowplayer now, are rarely in need of migrating the local database/cache (or are more than willing to just re-import).

I doubt the latter.

Since the flowplayer has only recently become available in a usable form, I don't see the foundation for your doubts?

Anyway, I depicted some routes above in the flowplayer.desktop review, which depend on some technical aspects. But as I still do not really comprehend, why the SailJail config conflicts with which migration functions, I cannot suggest which route to take.

It's very simple. The migration functions will no longer be able to read the files outside the scope of the declared namespaces once you deploy with sailjail. So, the copy from old namespace to new will simply fail.
Functions that use formulations like:

   const QString olderDb = QStandardPaths::writableLocation(QStandardPaths::ConfigLocation) + "/cepiperez/flowplayer.db";
    const QString oldDb = QStandardPaths::writableLocation(QStandardPaths::ConfigLocation) + "/flowplayer/flowplayer/flowplayer.db";

will not work, since the ConfigLocation itself is now unrelated (that is, will be .../sailfishos-applications/flowplayer) and not reflect the old installation paths.

But I went further and removed the old 'Generic' paths which were a 'bad idea(tm)' when they were originally implemented and certainly don't have anything to do with the migration code in the code base. That is to say: under no conditions would the migration code have found and moved the old album cover art cache files. So, it's a bit of a moot point. You need to rebuild the cache if we want to use sensible app defaults, namely, sailjail defaults.
I just don't have the resources to do this in some extended time frame. But how these rather sparse changes are applied can be broken up as you wish/see fit. It' s just a PR.

I could support some of these routes by creating an intermediate release soon, but would have to know which rout to take.

The PR I made here is making the bold assumption that the migration phase is finished. In any case, combining the changes while still using the old cache file patterns is not possible. So, this PR either waits for the conclusion of the 'migration phase' or we say, swallow the bitter pill and just enable Sailjail and the small extra feature. The latter I could enhance (ie, I'd like artist pics) later.

See above and for details the review comment. But I can assure you that we both, @dcaliste and me want this PR in.

Ok. I'm tight on time the next week or two. Having looked in some depth at the very pleasant and well built code base of unplayer, I had some ideas. But, as I say, I'm a bit tight on time.

[poetaster]

I think the new feature (and the new media-art location) can go in first when the PR is ready. Then, the code should be ready for sandboxing, and the desktop file can be adjusted to reflect this. I'm in favour to separate the sandboxing changes into its own PR that can be merged later. That being said, it's not a major issue to merge both at the same time. @poetaster could separate it in a stand-alone third commit to this PR ? Thank you in advance.

Although I understand an 'abundance of caution', I tested this in 'one go' since the major issue IS getting desktop standard cache/config/local storage. That just happens to conform with Sailjail which only 'enforces' the limit that you can't read outside. It's precisely because the Sailjail thing and the removal of paths like GenericCache coincide that I did them together. Next week, I could try to suggest a more formal test procedure which could allay fears that this is 'too much' in one PR . Some systematic tests. Ok?

[Olf0]

Social aspects first

Ok. I'm tight on time the next week or two. Having looked in some depth at the very pleasant and well built code base of unplayer, I had some ideas. But, as I say, I'm a bit tight on time.

Please @poetaster take your time: as much as you want or need. We are not in a hurry, this is spare time activity and should be (basically) fun.

Do put this PR aside for a while, if it has become a stress factor for you.

Please also respect, that I (IMHO this also applies to @dcaliste) do have to find the role to play here. I simply deployed the infrastructure changes to FlowPlayer which I originally had developed and applied to FileCase, because CepiPerez handed over both of his apps to me, hence I implicitly became a maintainer of it. But I did not plan to do much more, maybe handle some PRs by asking checks & balances questions; specifically I did not expect FlowPlayer to generate more interest than FileCase (a fundamentally incorrect assessment, as it turned out), which I am primarily interested in. Heck, I even do not use FlowPlayer at all, because I do not like to listen to music when being on the move.

This PR has grown over my head, too, and made me occasionally consider to drop my maintainership. But I feel that this would be unfair to @dcaliste (and others, e.g. rubdos), who takes care about the hard-core technical aspects very well, only because I drag him into reviewing stuff by asking kindly, each time substantial changes to C++ or QML code are suggested. And I am actually very happy about his slightly nitpicking style, because I know how quickly code becomes deranged and hence hard to maintain, if one does not permanently care about quality much. OTOH, I am basically still willing to keep infrastructure work as release management, translations infrastructure and automatisation, CI workflows, GH repo config, RPM packaging, keeping READMEs up-to-date, etc. away from @dcaliste and other developers, including you @poetaster; I think this is a fair deal and qualifies me as primary maintainer, despite avoiding to deal with C++ and QML code.
In the same line of thinking I do not want to stress @dcaliste beyond code reviewing.

[poetaster]

Social aspects first

Ok. I'm tight on time the next week or two. Having looked in some depth at the very pleasant and well built code base of unplayer, I had some ideas. But, as I say, I'm a bit tight on time.

Please @poetaster take your time: as much as you want or need. We are not in a rush, this is spare time activity and should be (basically) fun.

Do put this PR aside for a while, if it has become a stress factor for you.

No stress, I'm stressed on other fronts so that carries over. In any case don't worry about the back and forth. So far I have made all the changes that @dcaliste has asked me to make and don't mind if the process involves more granular splits. I just don't have the muse to invest in nitpicking. But, let's let it settle. @dcaliste if you have a specific order of moves you'd like me to apply, please specify.

Thanks for all your work!

[Olf0]

Technical stuff

the copy from old namespace to new will simply fail. [with sandboxing enabled]

Thanks, understood now, sorry for being too blind to see that late at night(s), when I have time for this.

Consequently all three migration functions will fail when sandboxing is enabled. Then it does not make any sense to keep the migration functions as soon sandboxing is enabled.

Thus I recur to asking to pose a separate PR, which employs the sandboxing configuration, which currently is part of this PR, and additionally deletes the three migration functions. Edit: Done, see PR #77.

This will render this PR as a coherent set of changes and the new one, too. I will then "squash merge" this PR, hence @poetaster do not worry about the commits (order, content, whatever), because they will end up being applied to the devel branch as a single, merged commit anyway.

Ultimately I consider to create a release between merging these two PRs, so we will have published two releases with the migration code, and you @poetaster get your new feature in its basic form published very soon.

Some systematic tests. Ok?

While I would never contradict to extend testing, I suggest to leave this WRT workload. This is still flowplayer 0.x.y, so expectations should be set low, and it is impossible for a single person to perform testing so extensively on a large variety of setups / configurations as all the users out there will. Let us just finish this PR and then release.

---

Edit: I performed the trivial style changes as requested by @dcaliste, made a suggestion for the single non-trivial style change, and omitted the SailJail config, so @poetaster can focus on the things I am unable resolve.

[dcaliste]

It occured to me yesterday evening that album covers are called everywhere in the code as a JPEG file, see:

grep "media-art/album" -r src/

Thus, copying a PNG file there will eventually fail without further modifications to the code.

I suggest to leave this as an open question, and remove temporarily the PNG matching from this PR. Like that, @Olf0 can accept the PR which is already a huge improvement over the (non-)existing possibility of local covers.

[Olf0]

It occurred to me yesterday evening that album covers are called everywhere in the code as a JPEG file, see:
grep "media-art/album" -r src/

Or equivalently via GitHub's web-frontend: https://github.com/search?q=repo%3Asailfishos-applications%2Fflowplayer+path%3A%2F%5Esrc%5C%2F%2F+media-art%2Falbum&type=code

@poetaster:

Should I remove the png (almost all cover art comes as jpegs as far as I've seen, so no loss) work and commit?

Yes please, as @dcaliste suggested:

… remove temporarily the PNG matching from this PR.

Cover art in PNG format shall be addressed in a later PR (if somebody volunteers), because it requires further, intrusive changes, see first paragraph of this message.

---

Please mind, that there are a two other open ToDos for you, see

1. unresolved comment WRT line 226 of src/datareader.cpp: https://github.com/sailfishos-applications/flowplayer/pull/75/files#diff-380f2b4fb214da5db2e2f6c4d0c1f8d22f2d74b1fbe40671cb3966c6900fc6b6R226 Done by myself by commit e5ff617.
2. unresolved comment WRT line 191 of src/datareader.cpp (may be a misunderstanding on my side, if so, please denote that): https://github.com/sailfishos-applications/flowplayer/pull/75/files#diff-380f2b4fb214da5db2e2f6c4d0c1f8d22f2d74b1fbe40671cb3966c6900fc6b6R191

[Olf0]

@dcaliste, was the change in indention by commit 6f54866 to your liking?
Superseded by #75 (comment).

If so (please state that clearly, or if you want changes, what exactly), I will resurrect this formatting, which was altered to the old formatting style in later commits.

[Olf0]

@dcaliste, please review my final changes, comprising to omit the suffix "jpg" (because other code locations do not support it; see also issue #78) and some formatting changes: https://github.com/sailfishos-applications/flowplayer/pull/75/files/e40b96426a712b8c3bd3bd80e65a202f892bae21..72261f18e8cd6b9efb3099679eca4f5c8039ab6f

Is the indention etc. fine now?

Edit: Superseded, corrected and improved by @dcaliste via commit 73b543a.

[Olf0]

As @poetaster missed to reply to my unresolved comment WRT line 191 of src/datareader.cpp (may be a misunderstanding on my side, if so, please denote that), maybe you, @dcaliste can clarify.

[Olf0]

From the ToDo list above I created issues #78, #79 and #80.

[dcaliste]

I'm pushing an additional commit with some modifications. Please, disregard this commit if you think I'm wrong. I've tested the PR and I noticed the following:

• *.jpg files can actually be copied (and renamed) since they are JPEG files, it's just the PNG types that will need transcode or a different handling,
• @poetaster, may I ask you why you needed to recurse into the subdirectories of a song when looking for a cover ? This actually creates false positive in some cases. For instance, I've a song directly in the ~/Music/ directory, let say ~/Music/foo.ogg. And a cover for the album Bar in its own directory, like ~/Music/Bar/cover.jpg. Then, recursing inside subdirectories will associate the Bar cover to the foo song. Which is erroneous.

So I've removed the recursing, but maybe this is addressing a case I didn't think about…

I've an album which is not associated to a cover. Typically, an album with various artists. In that case, the cover is associated to artist-album, but the UI, then use album-album scheme for multi-artist album. If you don't mind, let's keep this issue as pending for the moment.

[Olf0]

• *.jpg files can actually be copied (and renamed) since they are JPEG files, it's just the PNG types that will need transcode or a different handling,

Yes, I just understood that when looking through your changes.

I've an album which is not associated to a cover. Typically, an album with various artists. In that case, the cover is associated to artist-album, but the UI, then use album-album scheme for multi-artist album. If you don't mind, let's keep this issue as pending for the moment.

Sure, it is already covered by issue #80.

---

I plan to merge this in a couple of hours (towards midnight UTC) and then prepare a release. It is code, we can still change anything later, if necessary.

[poetaster]

I'm pushing an additional commit with some modifications. Please, disregard this commit if you think I'm wrong. I've tested the PR and I noticed the following:

* `*.jpg` files can actually be copied (and renamed) since they are JPEG files, it's just the PNG types that will need transcode or a different handling,

* @poetaster, may I ask you why you needed to recurse into the subdirectories of a song when looking for a cover ? This actually creates false positive in some cases. For instance, I've a song directly in the `~/Music/` directory, let say `~/Music/foo.ogg`. And a cover for the album Bar in its own directory, like `~/Music/Bar/cover.jpg`. Then, recursing inside subdirectories will associate the Bar cover to the foo song. Which is erroneous.

I have to admit, I don't recall why! In any case, all of the file based cover art I have is located at the same level as the songs within (for album downloads). I'm guessing that I was over/under thinking it.

So I've removed the recursing, but maybe this is addressing a case I didn't think about…

Thank you!

I've an album which is not associated to a cover. Typically, an album with various artists. In that case, the cover is associated to artist-album, but the UI, then use album-album scheme for multi-artist album. If you don't mind, let's keep this issue as pending for the moment.

Yes, that's one I'm working on. It's a minefield. But flowplayer has a search (which sadly uses google apis) to allow you to deal with it. I was thinking of starting with at least checking if the tag data already contained binaries and then working from there.

[Olf0]

LGTM

Requested changes are all carried out, partially by @dcaliste himself.