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

Revive #11441 #11782

Merged
merged 1 commit into from
Aug 6, 2023
Merged

Revive #11441 #11782

merged 1 commit into from
Aug 6, 2023

Conversation

Swiftb0y
Copy link
Member

@Swiftb0y Swiftb0y commented Aug 2, 2023

Fixes #11226, Supersedes #11441

I added another little feature where it disables the serato metadata export when the general metadata export is also disabled. I'm not sure if enforcing that in the UI is the best solution, suggestions welcome.

@github-actions github-actions bot added the ui label Aug 2, 2023
@Swiftb0y Swiftb0y changed the title feat/gh11226 Revive #11441 Aug 2, 2023
@daschuer
Copy link
Member

daschuer commented Aug 2, 2023

This can be also fixed in 2.3. Did you consider this?

I think the logic is already implemented in the back-end that the "SeratoMetadataExport" config is only used if "SyncTrackMetadataExport" is enabled. This is a visual fix at the right place IMHO.

In addition, this PR resets the value of "SeratoMetadataExport". So that after cycling "SyncTrackMetadataExport", "SeratoMetadataExport" is off.

Bug or feature?

@Swiftb0y
Copy link
Member Author

Swiftb0y commented Aug 2, 2023

In addition, this PR resets the value of "SeratoMetadataExport". So that after cycling "SyncTrackMetadataExport", "SeratoMetadataExport" is off.

Right, I wanted to show that disabling metadata export will disable serato export as well. Just making the checkbox unclickable was not enough of an indication for me since it still looked like it was enabled, just not changeable by the user.

Bug or feature?

Not sure either, I'd consider this "minor visual polishing". I don't care what branch it goes into, I just wanted to close the abandoned PR this is based on. What branch would you like this to be targeted at?

@daschuer
Copy link
Member

daschuer commented Aug 2, 2023

I would prefer to merge this to 2.3. if this is not to much hassle for you.

Just graying out the checkbox does not work. It need to be grayed out and unchecked.
The question is if we want to restore the old state after enabling metadata export.
I think we have the option "checked"/"unchecked"/"old state".

From UX perspective, it should be the "old state" to decouple dependencies, but if we have good reasons for a difference choice it is also fine.

This will make the behavior clear visually that serato metadata
can only be exported if the general metadata export is also
enabled.
@Swiftb0y Swiftb0y changed the base branch from 2.4 to 2.3 August 3, 2023 11:01
@Swiftb0y
Copy link
Member Author

Swiftb0y commented Aug 3, 2023

done, but expect merge conflicts when merging to 2.4.

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 8b5cdfb into mixxxdj:2.3 Aug 6, 2023
1 check passed
@daschuer daschuer added this to the 2.3.6 milestone Aug 6, 2023
@Swiftb0y
Copy link
Member Author

Swiftb0y commented Aug 6, 2023

Thank you

@Swiftb0y Swiftb0y deleted the fix/gh11226 branch August 6, 2023 11:46
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.

2 participants