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

Ddj 200 support #3185

Merged
merged 13 commits into from Oct 20, 2020
Merged

Ddj 200 support #3185

merged 13 commits into from Oct 20, 2020

Conversation

fkbreitl
Copy link
Contributor

@fkbreitl fkbreitl commented Oct 18, 2020

Mappings of the Pioneer DDJ-200.

Superseedes #2377.

res/controllers/Pioneer-DDJ-200-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-200-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-200-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-200-scripts.js Outdated Show resolved Hide resolved
@Holzhaus Holzhaus changed the base branch from master to 2.2 October 18, 2020 16:09
@Holzhaus Holzhaus added this to the 2.2.5 milestone Oct 18, 2020
@Holzhaus
Copy link
Member

Posting this here for better visibility: #2377 (comment)

@Holzhaus Holzhaus marked this pull request as draft October 18, 2020 16:51
@Holzhaus
Copy link
Member

Please mark this as ready to review when the review comments and the removal of the offending commit has been taken care of and you added a PR for the mixxxdj/manual repo.

Copy link
Contributor Author

@fkbreitl fkbreitl left a comment

Choose a reason for hiding this comment

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

Sorry, I committed a new file. I didn't know I can simply accept them.
But they should all be accepted.

Copy link
Contributor Author

@fkbreitl fkbreitl left a comment

Choose a reason for hiding this comment

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

All accepted. But I could not delete the files due to:

$ git reset --hard 9a3742d~1 -- Pioneer\ DDJ-SB.midi.xml
fatal: Cannot do hard reset with paths.

@fkbreitl
Copy link
Contributor Author

fkbreitl commented Oct 18, 2020

Ok, I have done git rebase -i 9a3742d5e8c3cbb3d56bb8872e39b99275a6d437~1 and then I see this in emacs:

File Edit Options Buffers Tools Flymake Help                                   
noop

# Rebase 4d3d17d64a..4d3d17d64a onto 4d3d17d64a (1 command)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup <commit> = like "squash", but discard this commit's log message
# x, exec <command> = run command (the rest of the line) using shell
# b, break = stop here (continue rebase later with 'git rebase --continue')
# d, drop <commit> = remove commit
# l, label <label> = label current HEAD with a name
# t, reset <label> = reset HEAD to a label
# m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
# .       create a merge commit using the original merge commit's
# .       message (or the oneline, if no original merge commit was
# .       specified). Use -c <commit> to reword the commit message.
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.

What next?

@fkbreitl
Copy link
Contributor Author

Can I just send you the files? The overhead for 2 files is tremendous ....

@Holzhaus
Copy link
Member

Holzhaus commented Oct 18, 2020

Can I just send you the files? The overhead for 2 files is tremendous ....

It doesn't work like that. You need to remove that change from the git history.

Ok, I have done git rebase -i 9a3742d5e8c3cbb3d56bb8872e39b99275a6d437~1 and then I see this in emacs:

I cannot reproduce that. Are you sure you are on the ddj-200-support branch? If you run git show, the first line should look like this:

commit ddbedfb4d03670db20a60d9bc376f119dfd4c203 (HEAD -> ddj-200-support, origin/ddj-200-support)

If you now run the exact command you posted, you should see something like this:

pick 9a3742d5e8 make existing Pioneer filenames consistent
pick ab7c15954e add in xml and js file for ddj-200
pick 8dddda4313 remove unused parameters
pick fa8746892a remove noise descriptions
pick 762c675a43 improve and optimise script logic
pick 257f0b9acf give good descriptions
pick d23a5e7e4b add support for seeking
pick e0f277c1d7 add support for LEDs
pick a8f466d8eb get headphones master button working
pick b9abc33708 remove unused arguments
pick 1b51f1a11e Changes according to pre-commit hook
pick ddbedfb4d0 Change from code review by Holzhaus

And you need to change it to:

drop 9a3742d5e8 make existing Pioneer filenames consistent
pick ab7c15954e add in xml and js file for ddj-200
pick 8dddda4313 remove unused parameters
pick fa8746892a remove noise descriptions
pick 762c675a43 improve and optimise script logic
pick 257f0b9acf give good descriptions
pick d23a5e7e4b add support for seeking
pick e0f277c1d7 add support for LEDs
pick a8f466d8eb get headphones master button working
pick b9abc33708 remove unused arguments
pick 1b51f1a11e Changes according to pre-commit hook
pick ddbedfb4d0 Change from code review by Holzhaus

PS: When posting code, please use fenced code blocks by putting 3 backticks in before and after the code (or indent each line by 4 spaces).

PPS: Here's some background on rebasing: https://git-scm.com/book/en/v2/Git-Branching-Rebasing

@fkbreitl
Copy link
Contributor Author

This is what I get:

$ git show
commit 4d3d17d64a97ae3f8c28927af609a610bc3e79d4 (HEAD, tag: release-2.2.3, dan-giddins/2.2, ddj-200-support)
Merge: 017fdc4033 d4edbceca3
Author: Be <be@mixxx.org>
Date:   Tue Nov 26 18:02:42 2019 -0600

    Merge pull request #2369 from Be-ing/release_2.2.3
    
    Release 2.2.3

@Holzhaus
Copy link
Member

You probably ran git reset --hard 9a3742d5e8c3cbb3d56bb8872e39b99275a6d437~1 without the file path parameter (i.e. without appending -- path/to/file). Therefore, you have reset your entire working tree to the state before commit 9a3742d (you basically deleted you entire progress locally). Fortunately, you didn't push yet, so your progress is not lost. You can pull it from github into your local clone via:

git pull origin ddj-200-support

If that worked and git show shows the line I posted above, you can do the rebase.

@fkbreitl
Copy link
Contributor Author

Yes, I ran it without file name because it didn't work with -- hard.
Now I did git pull and reset again. Now after
git rebase -i 9a3742d5e8c3cbb3d56bb8872e39b99275a6d437~1 I see the emacs windows again.
What should I type in?

@Holzhaus
Copy link
Member

Now I did git pull and reset again.

You don't need to run git reset, just rebase directly after pull.

Now after
git rebase -i 9a3742d5e8c3cbb3d56bb8872e39b99275a6d437~1 I see the emacs windows again.
What should I type in?

As I posted above:

If you now run the exact command you posted, you should see something like this:

pick 9a3742d5e8 make existing Pioneer filenames consistent
pick ab7c15954e add in xml and js file for ddj-200
pick 8dddda4313 remove unused parameters
pick fa8746892a remove noise descriptions
pick 762c675a43 improve and optimise script logic
pick 257f0b9acf give good descriptions
pick d23a5e7e4b add support for seeking
pick e0f277c1d7 add support for LEDs
pick a8f466d8eb get headphones master button working
pick b9abc33708 remove unused arguments
pick 1b51f1a11e Changes according to pre-commit hook
pick ddbedfb4d0 Change from code review by Holzhaus

And you need to change it to:

drop 9a3742d5e8 make existing Pioneer filenames consistent
pick ab7c15954e add in xml and js file for ddj-200
pick 8dddda4313 remove unused parameters
pick fa8746892a remove noise descriptions
pick 762c675a43 improve and optimise script logic
pick 257f0b9acf give good descriptions
pick d23a5e7e4b add support for seeking
pick e0f277c1d7 add support for LEDs
pick a8f466d8eb get headphones master button working
pick b9abc33708 remove unused arguments
pick 1b51f1a11e Changes according to pre-commit hook
pick ddbedfb4d0 Change from code review by Holzhaus

@fkbreitl
Copy link
Contributor Author

As I said, the next thing I see is this emacs window asking me for commands.
Do I need to abort because of the reset and start over with pull again or can I continue?
Doesn't it make sense to paste in
drop 9a3742d5e8 make existing Pioneer filenames consistent and save it?
Maybe then git will do what we expect.

@Holzhaus
Copy link
Member

If you don't see the list of lines all starting with pick, you did something else between pulling and running git rebase. If you ran git reset --hard 9a3742d5e8c3cbb3d56bb8872e39b99275a6d437~1 again, you deleted all your local changes again.

Just run these commands and nothing in between:

$ git pull origin ddj-200-support
$ git show

Make sure the first line of the output begins with ddbedfb4d03670db20a60d9bc376f119dfd4c203.

If that is the case, run:

$ git rebase -i 9a3742d5e8c3cbb3d56bb8872e39b99275a6d437~1

Now your editor opens and you need to do the changes I explained above.

@Holzhaus
Copy link
Member

Ah, sorry, I didn't look correctly. git show shows the diff of the last commit, that's normal. What do you mean by "making the diff disappear"?

@fkbreitl
Copy link
Contributor Author

Ah ok. I also didn't notice. I am a subversion user and I tought it was a diff.
So then all code is pushed now?
Is my part done?

@Holzhaus
Copy link
Member

Holzhaus commented Oct 18, 2020

The code is pushed, yes.

Did you already create a documentation PR at the manual repo? I don't see it: https://github.com/mixxxdj/manual/pulls

@fkbreitl
Copy link
Contributor Author

Yes, 8 hours ago in #2377 I mentioned
Holzhaus/manual#2

@fkbreitl
Copy link
Contributor Author

Ah, you closed it. I was wrong. Sorry.

@fkbreitl
Copy link
Contributor Author

This is the new one: mixxxdj/manual#233

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Just some missing metadata, otherwise LGTM! Thank you.

<info>
<name>Pioneer DDJ-200</name>
<author>Dan Giddins, Frank Breitling</author>
<description>Pioneer DDJ-200 configuration for 2 decks.</description>
Copy link
Member

Choose a reason for hiding this comment

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

The description is a bit redundant. Also, please add a forum link and the basename of the manual rst file.

Suggested change
<description>Pioneer DDJ-200 configuration for 2 decks.</description>
<description>2-deck USB MIDI controller that can also be powered by an external battery.</description>
<forum>Forum URL goes here</forum>
<manual>pioneer_ddj_200</manual>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the general link to the forum?
I think the link to the manual could be useful.
However, I am not sure if this is online already.
What information does the manual tag provide?

Copy link
Member

@Holzhaus Holzhaus Oct 18, 2020

Choose a reason for hiding this comment

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

Do you mean the general link to the forum?

No, the link to the forum post about the mapping (in case users want to ask for help or something).

You could just use this one: https://mixxx.discourse.group/t/pioneer-ddj-200-mapping/18259
(I suggest to post a little update there when this has been merged).

I think the link to the manual could be useful.
However, I am not sure if this is online already.
What information does the manual tag provide?

You just need to specify the basename of the RST file in the manual (without the file extension). I already did that in the GitHub suggestion above, so just apply that change (but fill in the forum thread link first).

We don't use a full URL in the <manual> tag, because that way it will work with the offline manual viewer that we're planning to add, too (see #3047).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, thank you!!
Done!

@fkbreitl
Copy link
Contributor Author

Now for the 4-deck mapping can we add a second XML file or do you only allow one mapping per controller?

Copy link
Contributor Author

@fkbreitl fkbreitl left a comment

Choose a reason for hiding this comment

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

All done

@Holzhaus
Copy link
Member

Now for the 4-deck mapping can we add a second XML file or do you only allow one mapping per controller?

We only allow a single mapping file per controller. The 4-deck support could be merged into this one (but than should happen in a separate PR).

@Holzhaus Holzhaus marked this pull request as ready for review October 19, 2020 20:38
Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
Copy link
Contributor Author

@fkbreitl fkbreitl left a comment

Choose a reason for hiding this comment

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

Change in metadata

@fkbreitl
Copy link
Contributor Author

And when will this get into 2.3? Should I create a PR for this, too?
Is there any chance for a new build of 2.2? Where could I look for it?

Copy link
Member

@Holzhaus Holzhaus 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 very much.

@Holzhaus
Copy link
Member

CI fails are unrelated.

@Holzhaus Holzhaus merged commit c0cd88a into mixxxdj:2.2 Oct 20, 2020
@fkbreitl
Copy link
Contributor Author

fkbreitl commented Oct 20, 2020

Thank you for your help!

@Holzhaus
Copy link
Member

And when will this get into 2.3? Should I create a PR for this, too?

No, we regularly merge 2.2 into 2.3 and 2.3 into master.

Is there any chance for a new build of 2.2? Where could I look for it?

I think a 2.2.5 release is planned, but I can't give a date.

@fkbreitl
Copy link
Contributor Author

How can I continue the development in branch 2.2 now? I don't see the new files. I did

$git pull origin 2.2
$git checkout origin/2.2
warning: refname 'origin/2.2' is ambiguous.
Already on 'origin/2.2'
$ git show
commit 4707133fb6fdd27fa3147918b7cf8dec72b19e3d (HEAD -> origin/2.2, origin/2.2, remotes/origin/2.2)
Merge: c61be7981e 508e6d9ec6
Author: Daniel Schürmann <daschuer@mixxx.org>
Date:   Fri Oct 9 16:02:52 2020 +0200

    Merge pull request #3163 from ronso0/pref-controller-links-update
    
    Preferences: update some wiki & forums links

@Holzhaus
Copy link
Member

Holzhaus commented Oct 20, 2020

I suggest to read the documentation on remotes.

In a nutshell, your origin remote likely points at your fork of the mixxx repository, not the upstream mixxx repo. You can check with git remote -v:

$ git remote -v
origin	git@github.com:fkbreitl/mixxx.git (fetch)
origin	git@github.com:fkbreitl/mixxx.git (push)

I suggest to add the actual mixxx repo as a new upstream remote:

$ git remote add upstream git@github.com:mixxxdj/mixxx.git

Now fetch the new data from the remote:

$ git fetch upstream

You can start a new branch from the most recent commit in the 2.2 branch of the upstream remote like this:

$ git checkout -b ddj-200-fixes upstream/2.2

@fkbreitl
Copy link
Contributor Author

Thank you. I still have Dan's repo in there. Is this ok or should it be removed?

dan-giddins     https://github.com/dan-giddins/mixxx.git (fetch)
dan-giddins     https://github.com/dan-giddins/mixxx.git (push)
origin  https://github.com/fkbreitl/mixxx.git (fetch)
origin  https://github.com/fkbreitl/mixxx.git (push)
upstream        git@github.com:mixxxdj/mixxx.git (fetch)
upstream        git@github.com:mixxxdj/mixxx.git (push)

@Holzhaus
Copy link
Member

Holzhaus commented Oct 20, 2020

It doesn't hurt to have it in there, except that git fetch --all will take slightly longer if he pushes a lot of new branches to his repo.

@fkbreitl
Copy link
Contributor Author

Ok, and what is the push command?

@Holzhaus
Copy link
Member

Ok, and what is the push command?

git push <remote-you-want-push-to> <branch-you-want-to-push>

In my example above:

git push origin ddj-200-fixes

@fkbreitl
Copy link
Contributor Author

Great, thank you! See PR #3193

@uklotzde uklotzde modified the milestones: 2.2.5, 2.3.0 May 11, 2021
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.

None yet

3 participants