Recording: fix table refresh issues #4648

ronso0 · 2022-01-25T02:19:05Z

I noticed that the track view selection and position was reset when I

was browsing Tracks without having activated any other sidebar item earlier (= no stored state of the track view)
toggled recording
then tried to move vertically in the tracks view with arrow keys or [Library] controls

In the end this is a fixup for #4177, as well as some tweaks I discovered while trying to find the cause for the issue.
Selection bug fixed by
26a5cea
26a5cea

ronso0 · 2022-01-25T02:20:18Z

still WIP until I verified all commits build

ronso0 · 2022-01-26T11:48:39Z

still WIP until I verified all commits build

All good. Ready for review.

src/library/recording/dlgrecording.cpp

uklotzde · 2022-01-30T15:53:03Z

src/library/recording/dlgrecording.cpp

+    saveCurrentViewState();
+    QString recordingDir = m_pRecordingManager->getRecordingDir();
+    // Hack: append "/" so the model stores the complete path the model state key
+    m_browseModel.setPath(mixxx::FileAccess(mixxx::FileInfo(


BrowseTabelModel::setPath() should be responsible for normalizing the path as needed. Otherwise this only works for DlgRecording, but not for anyone else.

BrowseTabelModel::setPath() gets /path/to/some/dir/ from the BrowseFeature and this works for both the model key as well as passing the path to BrwoseThread. Besides, "anyone else" is fictional since no other feature uses the BrowseTableModel.
The actual "issue" is that we store /path/to/recording/directory in ConfigKey(RECORDING_PREF_KEY, "Directory") without trailing separator (and I didn't want to touch how RecordingManager handles this)
So, IMO this 'hack' for the exceptional DlgRecording is okay IMO, compared to complicating the code elsewhere (and I thought about how to to do this safely and with little effort more than twice).

Qt normally doe not use a trailing separator for directories. I have not yet understand why this makes a difference here. Can you add another sentence that explains what happens otherwise?
How about assert that case inside setPath?

The "issue" is that the browseModel does send paths with trailing /
See the commit message of e79a977

The existing code that works perfectly fine for BrowseModel which I will not touch:

mixxx/src/library/browse/browsetablemodel.cpp

Lines 212 to 219 in f455527

void BrowseTableModel::setPath(mixxx::FileAccess path) {

if (path.info().hasLocation()) {

m_currentDirectory = path.info().locationPath();

} else {

m_currentDirectory = QString();

}

m_pBrowseThread->executePopulation(std::move(path), this);

}

Model view state keys are stored without trailing /

/any/path/passed/from/BrowseModel/ has trailing / and becomes /any/path/passed/from/BrowseModel
which is fine because any accidentally passed /path/to/MUSIC/file.ext would properly be stripped to /path/to/MUSIC

Without trailing / the rec path from the config/home/me/Music/Recordings would become /home/me/Music

Comments about the current code should be added to the code instead of hiding this information in a commit message.

Okay, I'll add those to browsetablemodel.cpp
Is the issue resolved for you then?

FWIW the / is appended in BrowseFeature, even though it is not needed to populate the track view (the Rec view was obviously working fine without /)
See

mixxx/src/library/browse/browsefeature.cpp

Lines 420 to 425 in b648a88

// We here create new items for the sidebar models

// Once the items are added to the TreeItemModel,

// the models takes ownership of them and ensures their deletion

TreeItem* folder = new TreeItem(

one.fileName(),

QVariant(one.absoluteFilePath() + QStringLiteral("/")));

Maybe it's needed for FolderTreeModel::hasChildren
or Mixxx::FileInfo is Dir (used by Library > addDir)
idk, and I won't touch it.

There is also the issue that BrowseTableModel::setPath(mixxx::FileAccess path) is either badly named or broken.
Because it expects a "file" location.

I think this will solve the issue by its roots:

void BrowseTableModel::setPath(mixxx::FileAccess path) { if (path.info().hasLocation()) { if (path.info().isFile()) { m_currentDirectory = path.info().locationPath(); } else { m_currentDirectory = path.info().location(); } } else { m_currentDirectory = QString(); } m_pBrowseThread->executePopulation(std::move(path), this); }

src/library/recording/dlgrecording.cpp

daschuer · 2022-01-30T21:06:21Z

src/library/recording/dlgrecording.cpp

+    saveCurrentViewState();
+    QString recordingDir = m_pRecordingManager->getRecordingDir();
+    // Hack: append "/" so the model stores the complete path the model state key
+    m_browseModel.setPath(mixxx::FileAccess(mixxx::FileInfo(


Qt normally doe not use a trailing separator for directories. I have not yet understand why this makes a difference here. Can you add another sentence that explains what happens otherwise?
How about assert that case inside setPath?

ronso0 · 2022-01-30T23:15:24Z

As usual:
I will --fixup small review issues and squash as soon as the review is considered.
Thus let me know when you consider your review done, so we don't have --fixups in main.
Thanks!

uklotzde · 2022-02-01T10:01:26Z

src/library/recording/dlgrecording.cpp

+    saveCurrentViewState();
+    QString recordingDir = m_pRecordingManager->getRecordingDir();
+    // Hack: append "/" so the model stores the complete path the model state key
+    m_browseModel.setPath(mixxx::FileAccess(mixxx::FileInfo(


Please note that neither mixxx::FileInfo nor QFileInfo promise to keep a trailing slashes.

It looks like the current mechanism for storing paths as model keys has serious shortcomings and these hacks may stop working at any time.

The issue is slightly different anyway. See #4648 (comment)

These checks is exactly what I wanted to avoid.

Back to square one:
Browse passes all paths with trailing / (manually appended, for no obvious reason).
Removing the / was actually just a cosmetic issue for the model key string and path.info().locationPath() a simple way to

check if path "is valid" and

remove /

Rec is the only feature that was passing a fixed path without trailing slash.
My change to append / there, too, is only working around .locationPath() chopping the last dir part.
We may simply check for path.info().isDir() and use the submitted path then.

daschuer · 2022-02-01T09:27:09Z

src/library/recording/dlgrecording.cpp

-    m_browseModel.setPath(mixxx::FileAccess(mixxx::FileInfo(m_recordingDir)));
+    saveCurrentViewState();
+    QString recordingDir = m_pRecordingManager->getRecordingDir();
+    // Hack: append "/" so the model stores the complete path the model state key


So it is actually not a hack but a requirement. How about this:

Suggested change

// Hack: append "/" so the model stores the complete path the model state key

// Append "/" to avoid the folder name is treated as a file name in BrowseTableModel::setPath()

daschuer · 2022-02-01T10:40:47Z

src/library/recording/dlgrecording.cpp

+    saveCurrentViewState();
+    QString recordingDir = m_pRecordingManager->getRecordingDir();
+    // Hack: append "/" so the model stores the complete path the model state key
+    m_browseModel.setPath(mixxx::FileAccess(mixxx::FileInfo(


There is also the issue that BrowseTableModel::setPath(mixxx::FileAccess path) is either badly named or broken.
Because it expects a "file" location.

I think this will solve the issue by its roots:

void BrowseTableModel::setPath(mixxx::FileAccess path) { if (path.info().hasLocation()) { if (path.info().isFile()) { m_currentDirectory = path.info().locationPath(); } else { m_currentDirectory = path.info().location(); } } else { m_currentDirectory = QString(); } m_pBrowseThread->executePopulation(std::move(path), this); }

ronso0 · 2022-02-02T00:45:47Z

Since locationPath() was chosen only to remove the trailing / from the paths passed by the BrowseFeature to have a 'clean' model key, and working around the sideeffect caused by the Rec path (no trailing /)...
I changed setPath() to use the entire path location (with /) for the model key and avoid the Rec issue in the first place:

void BrowseTableModel::setPath(mixxx::FileAccess path) {
    if (path.info().hasLocation() && path.info().isDir()) {
        m_currentDirectory = path.info().location();
    } else {
        m_currentDirectory = QString();
    }
    m_pBrowseThread->executePopulation(std::move(path), this);
}

Given @uklotzde's approval the other commits are fine.
@daschuer let me know if you go along with the current solution, then I will squash and we can merge.

daschuer · 2022-02-02T06:20:59Z

Thank you!

in case the recording path was changed in preferences

This is already triggered by RecordingFeature::activate

:setPath gets passed /any/file/path/. For the model view key we use locationPath() to chop the last / and everything after. Though, in the config we store /path/to/recording and this is used to call setPath(path), thus append / to get the correct model key.

No need to read the recording dir from config, check/create the dir and print "Recordings folder set to..." for every 1 MB written to disk.

ronso0 · 2022-02-02T12:59:33Z

Squashed and commit message adjusted.
Ready for merge.

daschuer · 2022-02-02T13:40:47Z

src/library/browse/browsetablemodel.cpp

-    if (path.info().hasLocation()) {
-        m_currentDirectory = path.info().locationPath();
+    if (path.info().hasLocation() && path.info().isDir()) {
+        m_currentDirectory = path.info().location();


It turns out that location() does not strip a trailing slash. Is this new behaviour desired?
I have hacked up a test to verify this:
https://github.com/daschuer/mixxx/blob/2114f3e34122b195bde868e2a3355f9626324bf6/src/test/fileinfo_test.cpp#L62

Yes, intended:

Since locationPath() was chosen only to remove the trailing / from the paths passed by the BrowseFeature to have a 'clean' model key, and working around the sideeffect caused by the Rec path (no trailing /)...
I changed setPath() to use the entire path location (with /) for the model key and avoid the Rec issue in the first place:

daschuer

LGTM

github-actions bot added the library label Jan 25, 2022

ronso0 force-pushed the record-table-refresh branch from edb6559 to f05c355 Compare January 26, 2022 00:28

ronso0 added the skins label Jan 26, 2022

ronso0 requested a review from uklotzde January 26, 2022 11:49

ronso0 added this to the 2.4.0 milestone Jan 26, 2022

ronso0 changed the title ~~Record table refresh~~ Recording: fix table refresh issues Jan 26, 2022

ronso0 marked this pull request as ready for review January 26, 2022 17:20

uklotzde reviewed Jan 29, 2022

View reviewed changes

src/library/recording/dlgrecording.cpp Outdated Show resolved Hide resolved

ronso0 force-pushed the record-table-refresh branch from e9edfe4 to 8f28523 Compare January 29, 2022 21:13

uklotzde suggested changes Jan 30, 2022

View reviewed changes

daschuer requested changes Jan 30, 2022

View reviewed changes

uklotzde reviewed Feb 1, 2022

View reviewed changes

uklotzde approved these changes Feb 1, 2022

View reviewed changes

daschuer requested changes Feb 1, 2022

View reviewed changes

daschuer approved these changes Feb 2, 2022

View reviewed changes

ronso0 added 8 commits February 2, 2022 13:46

DlgRecording: always use fresh path to update the table view

a3bc43d

in case the recording path was changed in preferences

DlgRecording: remove redundant model update

0a87463

This is already triggered by RecordingFeature::activate

DlgRecording: save view state before updating the model

5193644

restore view only when inserting rows into current Browse/Rec view

2a4aabc

RecordingManager::getFreeSpace: use stored dir string

9451c66

No need to read the recording dir from config, check/create the dir and print "Recordings folder set to..." for every 1 MB written to disk.

RecordingManager: style cleanup

9d9d24e

Browse: improve debug output for added tracks

cf62a0d

ronso0 force-pushed the record-table-refresh branch from 954ed93 to cf62a0d Compare February 2, 2022 12:58

daschuer requested changes Feb 2, 2022

View reviewed changes

daschuer approved these changes Feb 2, 2022

View reviewed changes

daschuer merged commit a348844 into mixxxdj:main Feb 2, 2022

ronso0 deleted the record-table-refresh branch February 2, 2022 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recording: fix table refresh issues #4648

Recording: fix table refresh issues #4648

ronso0 commented Jan 25, 2022 •

edited

Loading

ronso0 commented Jan 25, 2022

ronso0 commented Jan 26, 2022

uklotzde Jan 30, 2022

ronso0 Jan 30, 2022

daschuer Jan 30, 2022

ronso0 Jan 30, 2022 •

edited

Loading

uklotzde Jan 30, 2022

ronso0 Jan 30, 2022

ronso0 Feb 1, 2022

ronso0 Feb 1, 2022 •

edited

Loading

daschuer Feb 1, 2022 •

edited

Loading

daschuer Jan 30, 2022

ronso0 commented Jan 30, 2022

uklotzde Feb 1, 2022

daschuer Feb 1, 2022

ronso0 Feb 1, 2022

daschuer Feb 1, 2022

daschuer Feb 1, 2022 •

edited

Loading

ronso0 commented Feb 2, 2022

daschuer commented Feb 2, 2022

ronso0 commented Feb 2, 2022

daschuer Feb 2, 2022

ronso0 Feb 2, 2022

daschuer left a comment

	void BrowseTableModel::setPath(mixxx::FileAccess path) {
	if (path.info().hasLocation()) {
	m_currentDirectory = path.info().locationPath();
	} else {
	m_currentDirectory = QString();
	}
	m_pBrowseThread->executePopulation(std::move(path), this);
	}

	// We here create new items for the sidebar models
	// Once the items are added to the TreeItemModel,
	// the models takes ownership of them and ensures their deletion
	TreeItem* folder = new TreeItem(
	one.fileName(),
	QVariant(one.absoluteFilePath() + QStringLiteral("/")));

	// Hack: append "/" so the model stores the complete path the model state key
	// Append "/" to avoid the folder name is treated as a file name in BrowseTableModel::setPath()

Recording: fix table refresh issues #4648

Recording: fix table refresh issues #4648

Conversation

ronso0 commented Jan 25, 2022 • edited Loading

ronso0 commented Jan 25, 2022

ronso0 commented Jan 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 Jan 30, 2022 • 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

ronso0 Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

daschuer Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Jan 30, 2022

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 Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

ronso0 commented Feb 2, 2022

daschuer commented Feb 2, 2022

ronso0 commented Feb 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Jan 25, 2022 •

edited

Loading

ronso0 Jan 30, 2022 •

edited

Loading

ronso0 Feb 1, 2022 •

edited

Loading

daschuer Feb 1, 2022 •

edited

Loading

daschuer Feb 1, 2022 •

edited

Loading