Replace MDir with mixxx::FileInfo/FileAccess #3744

uklotzde · 2021-03-23T20:38:02Z

Next step towards replacing TrackFile and a more cohesive handling of sandboxing:

mixxx::FileAccess = mixxx::FileInfo + SecurityTokenPointer
Improve management of library root directories
- Overhaul DirectoryDAO
- Move access to DirectoryDAO from Library into TrackCollection which owns DirectoryDAO
- Improve detection of parent/child directories based on location paths provided by mixxx::FileInfo
Remove MDir

The next PR will improve sandboxing based on this foundation and finally remove TrackFile.

Unfortunately I am unable to split these changes into even smaller change sets afterwards. There is also some overlap between the current and the following changes. Just trying to keep it as small as possible after holding those PRs back for such a long time. For all new code I try to submit tiny PRs, one at a time.

src/util/fileaccess.h

daschuer

I have test tested this on Ubuntu and it still works. Thank you.

How do want to proceeded with the MacOS tests. Can we take the risk an and merge it without?

daschuer · 2021-03-23T21:00:03Z

src/library/browse/browsethread.h

@@ -49,7 +49,7 @@ class BrowseThread : public QThread {

    // You must hold m_path_mutex to touch m_path or m_model_observer
    QMutex m_path_mutex;
-    MDir m_path;
+    mixxx::FileAccess m_path;


This looks confusing, because one may think this refrences a file, but it is a directory. Do you have an idea to improve the naming?

QFileInfo also could reference everything file/dir/symlink, we follow it. This naming is at least consistent.

Yes, I know. Unfortunately this makes the usage code harder to read.
Can we rename m_pPath to something more specific, to improve the situation?

Do we use the security tokens for single files? Looking into the implementation, there are different call stacks for files and directories. Maybe we can inherit a file and a dir version and accept some code duplications for some extra safety

What are your future plans with this?

This is nothing that blocks this PR ...

I have just noticed this. The special QDir overloads in Sandbox are not needed an can be removed. We can switch entirely to mixxx::FileInfo. Only the browse view seems to be affected.

Inheriting does not make sense for this use case. You can only know at runtime if a file system reference is a file, directory, or symlink.

The equivalent of QFileInfo in Rust would be std::path::Path/std::path::PathBuf.

Just noticed that Sandbox handles directories differently. Converting back to draft until I have checked that nothing got mixed up here. Thanks for reviewing so far.

src/library/browse/foldertreemodel.cpp

daschuer · 2021-03-23T22:10:07Z

src/library/export/engineprimeexportjob.cpp

-                                  ->getDirectoryDAO()
-                                  .getDirs();
-        for (const auto& dir : dirs) {
+        const auto dirInfos = m_pTrackCollectionManager->internalCollection()


Suggested change

const auto dirInfos = m_pTrackCollectionManager->internalCollection()

const QList<mixxx::FileInfo> dirInfos = m_pTrackCollectionManager->internalCollection()

We don`t have a DirInfos class or such. That's why I think we we need to be here more verbose here.

Did you check consider this comment?

I have explicitly added the type for the loop variable (even though I don't consider it as relevant and everything was declared as auto before). It doesn't matter that the container is a QList, we just iterate through it.

uklotzde · 2021-03-25T22:22:46Z

@Holzhaus I needed platform-specific defines in tests: bf44876

uklotzde · 2021-03-25T22:24:36Z

Now with tests for directory links (not on Windows) and explicit distinction of directory vs. file sandboxing. Just to be sure that we don't break anything. The code in Sandbox still contains some redundancies and I am not entirely sure that it is correct. But that didn't change and everything should work as before.

Holzhaus

Works fine for me, tests pass.

src/util/fileaccess.h

Holzhaus · 2021-03-29T17:04:42Z

@daschuer Can I hit merge or are you planning to review this again?

daschuer · 2021-03-29T17:26:43Z

If you have reviewed and tested it, it should be sufficient, however a test on a mac would be nice.
Or did we test this already on a mac?

Holzhaus · 2021-03-29T17:27:45Z

No, maybe @Be-ing can help out.

daschuer · 2021-03-29T18:31:08Z

src/library/browse/browsethread.h

@@ -49,7 +49,7 @@ class BrowseThread : public QThread {

    // You must hold m_path_mutex to touch m_path or m_model_observer
    QMutex m_path_mutex;
-    MDir m_path;
+    mixxx::FileAccess m_path;


How about this:

Suggested change

mixxx::FileAccess m_path;

mixxx::FileAccess m_dir;

I don't want to introduce many unrelated changes just by renaming things. And I especially try to avoid touching this browse stuff.

Holzhaus

Works well, thank you!

github-actions bot added build code quality library ui labels Mar 23, 2021

Replace MDir with mixxx::FileInfo/FileAccess

dcc6f9e

Holzhaus reviewed Mar 24, 2021

View reviewed changes

src/util/fileaccess.h Show resolved Hide resolved

FileAccess: Add documentation

b6e4a2c

daschuer requested changes Mar 24, 2021

View reviewed changes

uklotzde marked this pull request as draft March 25, 2021 10:02

uklotzde changed the title ~~Replace MDir with mixxx::FileInfo/FileAccess~~ [WiP] Replace MDir with mixxx::FileInfo/FileAccess Mar 25, 2021

uklotzde added 4 commits March 25, 2021 21:10

Replace auto with explicit type

764c285

Explicitly distinguish between file/dir security tokens

9c47831

CMake: Public platform defines

bf44876

DirectoryDAO: Test that symbolic links are handled correctly

fd83957

uklotzde changed the title ~~[WiP] Replace MDir with mixxx::FileInfo/FileAccess~~ Replace MDir with mixxx::FileInfo/FileAccess Mar 25, 2021

uklotzde marked this pull request as ready for review March 25, 2021 22:25

Holzhaus reviewed Mar 27, 2021

View reviewed changes

src/util/fileaccess.h Outdated Show resolved Hide resolved

Fix typo

a7725cf

Holzhaus requested a review from daschuer March 28, 2021 10:28

daschuer reviewed Mar 29, 2021

View reviewed changes

daschuer approved these changes Mar 29, 2021

View reviewed changes

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

feb5ac4

Holzhaus approved these changes Apr 2, 2021

View reviewed changes

Holzhaus merged commit fb747b6 into mixxxdj:main Apr 2, 2021

uklotzde deleted the fileaccess branch April 2, 2021 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace MDir with mixxx::FileInfo/FileAccess #3744

Replace MDir with mixxx::FileInfo/FileAccess #3744

uklotzde commented Mar 23, 2021 •

edited

Loading

daschuer left a comment

daschuer Mar 23, 2021

uklotzde Mar 25, 2021

daschuer Mar 25, 2021

daschuer Mar 25, 2021 •

edited

Loading

uklotzde Mar 25, 2021

uklotzde Mar 25, 2021

uklotzde Mar 25, 2021

daschuer Mar 23, 2021

daschuer Mar 29, 2021

uklotzde Mar 29, 2021

uklotzde Mar 29, 2021

uklotzde commented Mar 25, 2021

uklotzde commented Mar 25, 2021

Holzhaus left a comment

Holzhaus commented Mar 29, 2021

daschuer commented Mar 29, 2021

Holzhaus commented Mar 29, 2021

daschuer Mar 29, 2021

uklotzde Mar 29, 2021

Holzhaus left a comment

	const auto dirInfos = m_pTrackCollectionManager->internalCollection()
	const QList<mixxx::FileInfo> dirInfos = m_pTrackCollectionManager->internalCollection()

Replace MDir with mixxx::FileInfo/FileAccess #3744

Replace MDir with mixxx::FileInfo/FileAccess #3744

Conversation

uklotzde commented Mar 23, 2021 • edited Loading

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

daschuer Mar 25, 2021 • edited Loading

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

uklotzde commented Mar 25, 2021

uklotzde commented Mar 25, 2021

Holzhaus left a comment

Choose a reason for hiding this comment

Holzhaus commented Mar 29, 2021

daschuer commented Mar 29, 2021

Holzhaus commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus left a comment

Choose a reason for hiding this comment

uklotzde commented Mar 23, 2021 •

edited

Loading

daschuer Mar 25, 2021 •

edited

Loading