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 Serato Metadata checkbox behaviour #11441

Closed
wants to merge 1 commit into from

Conversation

tech-bash
Copy link

Fix: #11226

Its working perfectly as wanted.

gsoc2

@uklotzde
Copy link
Contributor

uklotzde commented Apr 4, 2023

Closing PRs with unresolved review comments and opening new PRs instead is not helpful.

@daschuer
Copy link
Member

daschuer commented Apr 4, 2023

You need to start from the 2.4 branch to avoid rebasing.

@tech-bash
Copy link
Author

Closing PRs with unresolved review comments and opening new PRs instead is not helpful.

I know but sometimes it becomes quite messy, so I think of it as a fresh start. Sorry, will keep that mind for the future PRs.

@tech-bash
Copy link
Author

You need to start from the 2.4 branch to avoid rebasing.

Alright, so I need to change the base branch of this PR and rebase it?

@Aryan-Mishra24

This comment was marked as spam.

@tech-bash tech-bash changed the base branch from 2.3 to 2.4 April 4, 2023 10:21
@tech-bash
Copy link
Author

tech-bash commented Apr 4, 2023

Man it's so hard to make head or tails of what's going on here

I need to merge this commit onto a branch, @JoergAtGithub told me that its a 2.3 patch so initially I requested for 2.3 branch but now @daschuer wants 2.4 branch to be the base so I did the changes, but now I am not sure what to do next! I followed this.

@daschuer
Copy link
Member

daschuer commented Apr 4, 2023

@JoergAtGithub is correct, this is a patch that can go to 2.3.

@daschuer
Copy link
Member

daschuer commented Apr 4, 2023

You did your work however on the main branch. This will do the trick:
git rebase onto=upstream/2.3 HEAD~1
git push -f

@daschuer daschuer changed the base branch from 2.4 to 2.3 April 4, 2023 15:13
@Aryan-Mishra24

This comment was marked as spam.

@@ -554,4 +555,6 @@ void DlgPrefLibrary::slotSyncTrackMetadataToggled() {
if (isVisible() && checkBox_SyncTrackMetadata->isChecked()) {
mixxx::DlgTrackMetadataExport::showMessageBoxOncePerSession();
}
// If Track Metatdata checkbox is unchecked, ignore the state of Serato Metatdata checkbox.
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't make sense to repeat and rephrase what the code does as a comment. And it is also inconsistent to insert a comment only here but not at the other place. Please remove it.

@uklotzde
Copy link
Contributor

uklotzde commented Apr 4, 2023

Please get familiar with Git before opening any new PRs. Such a trivial fix is not worth all this noise and waste of time for everyone that is involved.

@Aryan-Mishra24

This comment was marked as off-topic.

@tech-bash
Copy link
Author

Please get familiar with Git before opening any new PRs. Such a trivial fix is not worth all this noise and waste of time for everyone that is involved.

Its not that I am not familiar with Git, its that I am trying to adapt Mixxx's coding style i know that in a few days I 'll be better, sorry for inconvenience!

@daschuer
Copy link
Member

daschuer commented Apr 5, 2023

Do you have git installed locally? On which OS are you?
Did you understand the commands from my post above?
#11441 (comment)

If you feel uncertain with git. I strongly recommend to do:
http://git-school.github.io/visualizing-git/
It is a nice game and you will learn git on the run.

@tech-bash
Copy link
Author

I am on Linux(Kubuntu), and I am travelling right now will push the changes after few hours, as I told ealier I am familiar with git, I understood the above command after the rebase there will be merge conflicts too that I will be needing to resolve and then I will push the updated changes, I am taking time because of my travelling issues.

@tech-bash
Copy link
Author

tech-bash commented Apr 5, 2023

Thank you for your concerns!

@ronso0
Copy link
Member

ronso0 commented Apr 6, 2023

Thank you. Please squash the two commits to get rid of the kdev4 file.

@tech-bash
Copy link
Author

tech-bash commented Apr 6, 2023

What worked for me was: git rebase --onto upstream/2.3 HEAD~1 and then git push --force, which is implicitly same as @daschuer gave in the above comments, Thank you!

@ronso0
Copy link
Member

ronso0 commented Apr 6, 2023

Okay. Squashing can be done when rebasing interactively (-i)
git rebase -i --onto upstream/2.3
then just follow the instructions.

@daschuer
Copy link
Member

daschuer commented May 6, 2023

@tech-bash
Before merge, we need your permission to distribute the changes with Mixxx.
Please sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
and comment here when done.

@daschuer
Copy link
Member

daschuer commented May 6, 2023

This is a last minute 2.3.5 candidate

@daschuer
Copy link
Member

I con confirm that @tech-bash has signed the contributor agreement.

Do you have interest to fix the CI so that we can include this in the next release?

@daschuer daschuer added this to the 2.3.6 milestone Jul 18, 2023
@Swiftb0y Swiftb0y mentioned this pull request Aug 2, 2023
daschuer added a commit that referenced this pull request Aug 6, 2023
@daschuer
Copy link
Member

daschuer commented Aug 6, 2023

Closed via #11782. Thank you.

@daschuer daschuer closed this Aug 6, 2023
@tech-bash tech-bash deleted the fix/serato branch February 27, 2024 07:48
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.

5 participants