Add mixxx::FileInfo for file system references #3741

uklotzde · 2021-03-22T07:30:03Z

Add a wrapper mixxx::FileInfo that acts as a shim for QFileInfo with a reduced API and Mixxx-specific extensions. It introduces the concept of a "location" for permanent, normalized file/directory/symlink references that are used in many places in Mixxx. Supposed to replace both TrackFile and MDir. Please refer to the code for more detailed documentation.

This is only the first PR in a row. If you want to see this class in action please check #2656.

daschuer

I have left some comments.
I have not yet get the point where it helps in:
#2656
Can you print me to a place?

daschuer · 2021-03-22T07:57:17Z

src/util/fileinfo.h

+namespace mixxx {
+
+/// Helper functions and extensions for QFileInfo using Mixxx terminology.
+namespace fileinfo {


Having a namespace in addition to a class would be a new concept in Mixxx, and will probably hide the function from intellisense feature of the IDE.

I would prefere to make them static class members.

How would you then name those static functions to resolve the naming conflicts?

We can have a static and a non static function with the same name, because the parameters are different.

QSting location = AbsoluteFileInfo::location(file);

Looks Ok to me.

The purpose of the assert inside would be obvious. No need to a extra comment.

The property of being not relative is not a class invariant. Instead this must be determined at runtime whenver you try to obtain a location string.

How could it happen that it changes after the QFileObject is constructed?
I am unsure how this relates to my interface proposal, because it just showsa rename and a move into the class namespace.

Please note that only some mixxx::FileInfo instances will be converted into location strings. This must only be checked when you try to obtain a location, not in the constructor.

I have left some comments.
I have not yet get the point where it helps in:
#2656
Can you print me to a place?

The subsequent commits in #2656 will make use of this class.

src/util/fileinfo.h

daschuer · 2021-03-22T10:28:56Z

Ok, I think I have got it. You want a class that is not a relative File.

Why not just assert this at the constructor and all other non const members.
The class can be called AbsoluteFileInfo to make this clear.

uklotzde · 2021-03-22T11:44:38Z

I

Ok, I think I have got it. You want a class that is not a relative File.

Why not just assert this at the constructor and all other non const members.
The class can be called AbsoluteFileInfo to make this clear.

No. I want a class that reflects the application-level concepts and allows to detect unintended usage, both at compile-time (limited API) and runtime (debug assertions).

daschuer

Thank you. We are getting close.
Here some more comments.

src/util/fileinfo.h

daschuer · 2021-03-22T13:51:20Z

src/util/fileinfo.h

+    /// once it has been set, even after the corresponding file has
+    /// been deleted! A non-empty canonical location is immutable.
+    /// See also: FileInfoTest
+    QString freshCanonicalLocation();


Is the immutable issue a Mixxx or Qt issue?
Because of the immutable nature the name is misleading. Do we need the function at all?
Maybe something like
ResolveUnresolved()

I consider QFileInfo a dangerous and confusing class, mainly caused by the (optional, but default) caching behind the scenes that causes side-effects.

This function has been adopted from TrackFile. It is needed to ensure that previously unavailable locations become accessible when mounting volumes after starting Mixxx. A refresh is only triggered if the first attempt to obtain the canonical location results in an empty string. In this case and if cashing is enabled we need to check the file system again, to ensure that the file is really absent.

The comment already explains the situation in detail and is now verified by an extensive test. I am not investing more time here.

Ok, than this is only a naming issue.
Here:

mixxx/src/track/trackref.h

Line 22 in 1ca77e0

auto canonicalLocation = fileInfo.freshCanonicalLocation();

all comments here are out of sight and you have placed another comment to explain the function again.

I think the function should be called like:

canonicalLocationFromCashOrRetryLookUp()
or
canonicalLocationRefresIfEmpty()

less exact but more catchy:

originalCanonicalLocation()

or we may split it up in too calls()

fileinfo.resolveUnresolved()
return fileinfo.canonicaLocation()

We remove the function her and inline it.

src/util/fileinfo.h

uklotzde · 2021-03-22T18:51:34Z

The next 2 commits will remove MDir and wrap mixxx::FileInfo + SecurityTokenPointer into mixxx::FileAccess. Removing TrackFile will happen after this is done.

daschuer · 2021-03-22T23:34:40Z

src/util/fileinfo.h

+    /// once it has been set, even after the corresponding file has
+    /// been deleted! A non-empty canonical location is immutable.
+    /// See also: FileInfoTest
+    QString freshCanonicalLocation();


Ok, than this is only a naming issue.
Here:

mixxx/src/track/trackref.h

Line 22 in 1ca77e0

auto canonicalLocation = fileInfo.freshCanonicalLocation();

all comments here are out of sight and you have placed another comment to explain the function again.

I think the function should be called like:

canonicalLocationFromCashOrRetryLookUp()
or
canonicalLocationRefresIfEmpty()

less exact but more catchy:

originalCanonicalLocation()

or we may split it up in too calls()

fileinfo.resolveUnresolved()
return fileinfo.canonicaLocation()

We remove the function her and inline it.

src/util/fileinfo.h

uklotzde · 2021-03-23T08:13:36Z

I am not able to change

mixxx/src/track/trackref.h

Line 22 in 1ca77e0

auto canonicalLocation = fileInfo.freshCanonicalLocation();

until TrackFile has been replaced by mixxx::FileInfo. The legacy class remains as is. I have prepared a commit that will follow in a later PR.

daschuer

Thank you. Nice result :-)

daschuer · 2021-03-23T08:51:06Z

Can this be merged after CI has finished?

uklotzde · 2021-03-23T09:03:41Z

Can this be merged after CI has finished?

Yes. Please wait for CI, I haven't built this locally yet.

daschuer · 2021-03-23T13:00:13Z

All Green. Thank you.

Add mixxx::FileInfo

b50e6ea

uklotzde added the code quality label Mar 22, 2021

uklotzde added this to the 2.4.0 milestone Mar 22, 2021

github-actions bot added the build label Mar 22, 2021

daschuer reviewed Mar 22, 2021

View reviewed changes

uklotzde added 4 commits March 22, 2021 13:34

FileInfo: Delete unused qHash() function

5e2eab0

FileInfo: Replace free functions with static member functions

86b78eb

FileInfo: Add notes about the class design

becb328

Merge branch 'main' of git@github.com:mixxxdj/mixxx.git into fileinfo

284cede

daschuer requested changes Mar 22, 2021

View reviewed changes

uklotzde added 3 commits March 22, 2021 19:07

FileInfo: Delete section comments

1e8ab79

FileInfo: Fix hasLocation() and test assumptions

2d3ee02

FileInfo: Amend documentation of freshCanonicalLocation()

9c7264c

daschuer requested changes Mar 22, 2021

View reviewed changes

FileInfo: Rename member function

13f5884

daschuer approved these changes Mar 23, 2021

View reviewed changes

daschuer merged commit e1f8fea into mixxxdj:main Mar 23, 2021

uklotzde deleted the fileinfo branch March 23, 2021 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mixxx::FileInfo for file system references #3741

Add mixxx::FileInfo for file system references #3741

uklotzde commented Mar 22, 2021

daschuer left a comment

daschuer Mar 22, 2021

uklotzde Mar 22, 2021

daschuer Mar 22, 2021

daschuer Mar 22, 2021

uklotzde Mar 22, 2021

daschuer Mar 22, 2021

uklotzde Mar 22, 2021

uklotzde Mar 22, 2021

daschuer commented Mar 22, 2021

uklotzde commented Mar 22, 2021

daschuer left a comment

daschuer Mar 22, 2021

uklotzde Mar 22, 2021

uklotzde Mar 22, 2021 •

edited

Loading

daschuer Mar 22, 2021

uklotzde commented Mar 22, 2021

daschuer Mar 22, 2021

uklotzde commented Mar 23, 2021

daschuer left a comment

daschuer commented Mar 23, 2021

uklotzde commented Mar 23, 2021

daschuer commented Mar 23, 2021

Add mixxx::FileInfo for file system references #3741

Add mixxx::FileInfo for file system references #3741

Conversation

uklotzde commented Mar 22, 2021

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Mar 22, 2021

uklotzde commented Mar 22, 2021

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Mar 22, 2021

Choose a reason for hiding this comment

uklotzde commented Mar 23, 2021

daschuer left a comment

Choose a reason for hiding this comment

daschuer commented Mar 23, 2021

uklotzde commented Mar 23, 2021

daschuer commented Mar 23, 2021

uklotzde Mar 22, 2021 •

edited

Loading