Skip to content

(fix) Track file export: various fixes - #13610

Merged
daschuer merged 4 commits into
mixxxdj:2.4from
ronso0:track-export-skip-fix
Aug 31, 2024
Merged

(fix) Track file export: various fixes#13610
daschuer merged 4 commits into
mixxxdj:2.4from
ronso0:track-export-skip-fix

Conversation

@ronso0

@ronso0 ronso0 commented Aug 29, 2024

Copy link
Copy Markdown
Member
  1. The overwrite dialog had confusing/duplicate buttons,and clicking Skip would cancel the dialog
    before:
    export-dlg-before

    now:
    export-dlg-now

  2. Crate: use the right-clicked carte (no the selected crate, which might be null)

  3. abort track export if no tracks are selected (empty crate/playlist), previously the progress dialog would be empty and remain open forever

@ronso0 ronso0 added this to the 2.5.0 milestone Aug 29, 2024
@ronso0
ronso0 force-pushed the track-export-skip-fix branch from 37520b3 to c858211 Compare August 29, 2024 13:52
@ronso0
ronso0 changed the base branch from 2.5 to 2.4 August 29, 2024 13:53
@ronso0 ronso0 modified the milestones: 2.5.0, 2.4.2 Aug 29, 2024
@daschuer

daschuer commented Aug 30, 2024

Copy link
Copy Markdown
Member

In the corresponding Nemo File Manager dialog we have checkbox for all files and only three buttons.
But that would be not a bugfix PR. It uses "Replace" instead of "Overwrite" which sounds less techy.

@daschuer daschuer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After fixing the build it works here. I will file a feature request for the checkbox.

}

void CrateFeature::slotExportTrackFiles() {
m_lastRightClickedIndex = index;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How does this work? What is index?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

umm 🤷
I made the fix in 2.5, build, then rebased onto 2.4 but didn't build again, so my guess is this is a wronlgy resolved conflict.
Interesting that CI can build it.

@ronso0
ronso0 force-pushed the track-export-skip-fix branch from c858211 to 9848391 Compare August 30, 2024 14:41
@ronso0
ronso0 marked this pull request as draft August 30, 2024 15:40
@ronso0
ronso0 force-pushed the track-export-skip-fix branch from 9848391 to 2d4dfe3 Compare August 30, 2024 15:54
@ronso0
ronso0 marked this pull request as ready for review August 30, 2024 15:54
Comment thread src/library/export/trackexportdlg.cpp Outdated
question_box.setDefaultButton(pSkip);

question_box.exec();
auto pBtn = question_box.clickedButton();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

clang-tidy is suggesting:

Suggested change
auto pBtn = question_box.clickedButton();
auto* pBtn = question_box.clickedButton();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

@ronso0
ronso0 force-pushed the track-export-skip-fix branch from 2d4dfe3 to 66ec8b4 Compare August 30, 2024 17:28
Comment thread src/library/export/trackexportworker.cpp Outdated
@ronso0
ronso0 force-pushed the track-export-skip-fix branch from 66ec8b4 to ece92f1 Compare August 31, 2024 12:48

@Swiftb0y Swiftb0y left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. wdyt @daschuer?

@daschuer daschuer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works and LGTM. Thank you.

@daschuer
daschuer merged commit d9704db into mixxxdj:2.4 Aug 31, 2024
@ronso0
ronso0 deleted the track-export-skip-fix branch September 21, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants