refact(RhythmBoxFeature): Simplified library detection

[DidierMalenfant]

Unified the method used to detect the library behind maybeDatabaseFilePath() and playlists behind maybePlaylistsFilePath() to make sure code wouldn't be duplicated.

Added support for Rythmbox3 when installed as flatpak

[DidierMalenfant]

Mmmm... The hosted runner lost communication with the server. not sure what happened there.

[daschuer]

This is a GitHub issue. I have started the build again. Hopefully it fixes the issue.

[daschuer]

This is unfortunately not ideal in terms of disk access. It is probably not really relevant performance wise, but let's not introduce this regression here. Make sure that exists() is only called once per possible path.

If you like, you can even improve that further:

The pattern:

    QFile db(db_path);
    mixxx::FileInfo fileInfo(db);
    if (!Sandbox::askForAccess(&fileInfo) ||
            !db.open(QIODevice::ReadOnly)) {

used in may places is not ideal, because

Only QFile::fileName() is used to create QFileInfo:

https://github.com/qt/qtbase/blob/44ee50486b807e5d01a66380597d9a9ad84251ad/src/corelib/io/qfileinfo.cpp#L384

But this comes from the internal file engine which is all set up in the QFile() ctor before we have access.

askForAccess() also calls "exists()" you may rely on this or reuse th cached result in a QFileInfo object used earlier for finding the correct path. See:

mixxx/src/util/sandbox.cpp

Line 111 in d93a2fe

if (!pFileInfo->exists()) {

Btw: we prefer asignment initalizers when possible:
auto db = QFile(db_path); over QFile db(db_path);
because it does not look like a function call when you skim through the code.

[DidierMalenfant]

Make sure that exists() is only called once per possible path.

Are you ok with using std:optional internally to cache the found paths? They would be empty on init but filled in by the first query and returned without querying thereafter.

[daschuer]

I do not understand how an optional type will help but I have no objections if it does. This is anyway no bottleneck.

My idea was like that:

• Create a list of possible database locations.
• Use a FileInfo class to check in a loop if they exists. (Omit the extra check for existing directories)
• If you have one. pass that instance of FileInfo to the Sandbox
• If we have access create the QFile and open it.

The FileInfo object does the caching.

[DidierMalenfant]

I do not understand how an optional type will help but I have no objections if it does. This is anyway no bottleneck.

The optional would be to make the FileInfo creation lazy upon calling maybeDatabaseFilePath(). RythmBoxFeature stores an std::optional<FileInfo>, this allow to test if it's empty or not when calling maybeDatabaseFilePath(). If it is, we loop thru the paths and maybe find one that works. If it is not empty we just return that value without going thru the folders again.

It looks like we can store a default-constructed Fileinfo in case we go thru the paths and find nothing. We could push this even a bit further and return an optional for maybeDatabaseFilePath() which, if non-empty, is guaranteed to exist and has the sandbox access already approved on it. That way calling code doesn't have to do any tests that involve the file system just test the optional returned for non-empty.

Internally in RythmBoxFeature the empty optional means we haven't looked yet. A non-empty optional but default FileInfo means we looked and didn't find anything. And finally a valid FileInfo is just returned as it since it's been found before.

[daschuer]

Ah yes, I think I got it. You want even avoid double look ups later.

The idea to wrap FileInfo into an std::optional member will even avoid construction of default FileInfo() which involves memory allocation.

Calling exists() one FileInfo the first time fills cachedFlags: https://github.com/qt/qtbase/blob/5b48825fe84d5f9c87b9d0bb199aeb5c77bf0dd7/src/corelib/io/qfileinfo.cpp#L113
Subsequent calls will return the cache instead of doing disk access.

All this has probably a not notable performance impact. So decide wise how much effort you like to put into it. My original concern was just about the addition disc access, during one maybeDatabase... call.

[DidierMalenfant]

All this has probably a not notable performance impact. So decide wise how much effort you like to put into it. My original concern was just about the addition disc access, during one maybeDatabase... call.

I'm with you. It's not that much more code and I do like elegant and efficient solutions so I'll throw it in. Still getting my footing on how much to diverge from the code already in place sometimes :)