Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

(fix) relink directory / migrate Linux/macOS <--> Windows #12878

Merged
merged 1 commit into from Apr 15, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Feb 26, 2024

The issue:

When trying to relink a music directory with a database created in another OS (actually only Windows <--> Linux/macOS),
directory relocation will fail. This is due to the fact that mixxx::FileInfo (QFileInfo), which is part of the roundtrip
DirectoryDAO -> Library preferences -> ... -> DirectoryDAO, does unexpected things (no explicit documentation?) if the path string it's constructed from doesn't match the current OS' path schema:

  • Windows -> Linux:
    C:\some\path is considered relative and the the current working dir path is prepended
    /mixxx/build/dir/C:/some/path
  • Linux -> Windows
    same issue, /home/Music is missing "[drive letter]:" prefix and becomes C:/home/Music (not found etc.)

For both directions it fails with this DEBUG_ASSERT in DirectoryDAO::relocateDirectory and relocation will fail silently.
See #12715

For both directions I also hit this assertion in mixxx::Fileinfo), probably when trying to load cover arts.

(note #12436 which adds some feedback to directory operations)


Simple fix:

avoid the string -> fileInfo -> string roundtrip and simply use QStrings for the entire display/relocation flow.
Just the 'oldDirectory' test needs to be fixed.
Or, does it? Actually it's now the same path returned by DirectoryDAO::getRootDirStrings(), so if it was changed on the roundtrip the query will fail 馃し

Note for testing with a db from another OS:

You need to build with DEBUG_ASSERTIONS_FATAL=OFF, otherwise you'll hit them in fileinfo.h when the library tries to display cover art.
This also prevents building helpful migration tests.

@@ -144,7 +168,7 @@ DirectoryDAO::RemoveResult DirectoryDAO::removeDirectory(
QList<RelocatedTrack> DirectoryDAO::relocateDirectory(
const QString& oldDirectory,
const QString& newDirectory) const {
DEBUG_ASSERT(oldDirectory == mixxx::FileInfo(oldDirectory).location());
// DEBUG_ASSERT(oldDirectory == mixxx::FileInfo(oldDirectory).location());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we accomlish the same validation, just without QFileInfo?
IIUC this guarantees that the string and location (string) are identical, if not we can check if it's only the prefix that differs and move on in that case.

Anyone has a smart QString trick handy for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, do we really need an extensive check here?
It's now the same path returned by DirectoryDAO::getRootDirStrings(), so if it was changed on the roundtrip the query will fail.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have omitted the rather complex check (with no actual benefit except debug output) and added a comment instead.
Any ideas to improve the situation are welcome.

@ronso0 ronso0 changed the title DEBUG relink directory / migrate Linux -> Windows (fix) relink directory / migrate Linux/macOS <--> Windows Feb 27, 2024
@ronso0 ronso0 changed the base branch from main to 2.4 February 27, 2024 02:07
@ronso0
Copy link
Member Author

ronso0 commented Feb 27, 2024

(updated the description with more findings)

@ronso0 ronso0 added this to the 2.4.1 milestone Mar 4, 2024
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Good catch.

@daschuer daschuer merged commit 646b19d into mixxxdj:2.4 Apr 15, 2024
14 checks passed
@ronso0 ronso0 deleted the relink-win-linux branch April 15, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants