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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(fix) Track menu: avoid crash and UX issues with track nullptr #13685

Merged
merged 6 commits into from
Sep 27, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Sep 22, 2024

This is just a quick fix to avoid a crash when trying to open the track menu or Track Properties with Ctrl+P
when the selection contains a duplicate track. (a track that references the same file as another track)
Note: fixin the UX inconsistencies (see below) is a lot more work, probably requires some not so trivial refactoring.

No idea how I got that duplicate file, but it's awful show stopper.
The general question is how to provide helpful info to the user so they can resolve the situation themselves (without having to edit the database manually).

The menu and Track Properties can now be opened (Track Props is empty) but at least it doesn't crash anymore (in release builds / with DEBUG_ASSERTIONS_FATAL = OFF).

Note for testing:
In case you manage to create a duplicate track, you need to disable this DEBUG_ASSERT in TrackDAO inorder to proceed

DEBUG_ASSERT(!trackId.isValid() || trackId == cacheResolver.getTrackRef().getId());

FWIW I couldn't duplicate a track with DB Browser for SQlite, it prevents that:
Error changing data: UNIQUE constraint failed: track_locations.location


Uff, the UX when encountering such a duplicate is very inconsistent, and I think a 'normal' user (DEBUG_ASSERTIONS_FATAL = OFF) does not get enough information to resolve the situation:

  1. Track Properties can be opened, but it's empty
  2. when opening Track Properties for another track in the table, then hit a dupe track when using Next/Prev, the fields are cleared but the window title is not
  3. MusicBrainz dialog can be opened but displays ~20% progress and "Tags can be applied"
  4. otoh, the track menu can be opened and most (all?) actions work was confused due to ¹
    disabled: Find On Web
    works: Add to Auto DJ, Add to Playlist/Crate (though sidebar items are not set bold)
    not working: BPM, Color, ...
    Search Related:
    • is disabled when hovering/selecting it, cursor is stuck there, menu can only be closed by pressing Esc twice
    • remains disabled for regular tracks afterwards (wrong order of populate and setEnabled())
  5. when loading the track by dragging it to a deck (loaded by ref/url), the 'original' track¹ is loaded
  6. otoh neither double-click in Tracks nor the keyboard shortcuts Shift+Left/Right work (loaded via TrackPointer, but WTrackTableView::loadSelectedTrackToGroup aborts due to nullptr)
  7. track color is displayed in Tracks but in decks it's the color of the 'original' track?

¹ GlobalTrackCache/TrackDAO throw the warning for the newer track.
No, it depends on which track has been cached first. So the dupe can be different in each session.

@daschuer
Copy link
Member

does pTrack = getFirstTrackPointer(); return null?
I have not yet understand how duplicates are related. But that change is very much required.

@ronso0
Copy link
Member Author

ronso0 commented Sep 22, 2024

TrackDAO returns nullptr when it (actually GlobalTrackCacheResolver) encounters a duplicate.

@ronso0
Copy link
Member Author

ronso0 commented Sep 23, 2024

I updated the description with more findings wrt UX inconsistency.

I'll make some small UX fixes soonish.

@ronso0 ronso0 force-pushed the duplicate-track-location branch from be3ce61 to 7cbc4bd Compare September 23, 2024 13:35
@github-actions github-actions bot added the ui label Sep 23, 2024
@ronso0
Copy link
Member Author

ronso0 commented Sep 23, 2024

Some fixes...

If desired, I can move the DlgTagFetcher fix to a separate PR.

@ronso0 ronso0 changed the title (fix) Track menu: avoid crash with track nullptr (fix) Track menu: avoid crash and UX issues with track nullptr Sep 23, 2024
@ronso0 ronso0 marked this pull request as ready for review September 23, 2024 13:36
@ronso0 ronso0 added this to the 2.4.2 milestone Sep 24, 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. Thank you. What is your plan with the TODOs?

@@ -849,6 +849,7 @@ void WTrackMenu::updateMenus() {
if (featureIsEnabled(Feature::LoadTo)) {
int iNumDecks = static_cast<int>(m_pNumDecks.get());
m_pDeckMenu->clear();
// TODO Only enable for single track?
Copy link
Member

Choose a reason for hiding this comment

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

Yes

@@ -964,6 +965,7 @@ void WTrackMenu::updateMenus() {
bool anyBpmLocked;
bool anyBpmNotLocked;
std::tie(anyBpmLocked, anyBpmNotLocked) = getTrackBpmLockStates();
// TODO Disable menu if anyBpmLocked == anyBpmNotLocked ??
Copy link
Member

Choose a reason for hiding this comment

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

Reflecting the state with the menu would be nice.
I can imagine a use case where you want clear all BPM values of not locked tracks. Therefor a feature might suit that is enabled when at least one track has no BPM locked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, the TODO text is not clear. What I meant was: disable the menu if both are false which happens if getTrackBpmLockStates() found no valid track.

But actually, if users run into null tracks unnoticed, those tracks won't be processed and the log should contain a warning, which IMO is more helpful to discover null tracks than greying out the menu.

I'll remove the TODO

@ronso0 ronso0 force-pushed the duplicate-track-location branch from 0edd51a to 580a8fc Compare September 26, 2024 10:51
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, Thank you.

@daschuer daschuer merged commit 19c0e66 into mixxxdj:2.4 Sep 27, 2024
14 checks passed
@ronso0 ronso0 deleted the duplicate-track-location branch September 27, 2024 14:00
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.

2 participants