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

Update denon_mc7000.rst #484

Merged
merged 4 commits into from
Feb 22, 2023
Merged

Update denon_mc7000.rst #484

merged 4 commits into from
Feb 22, 2023

Conversation

holopansel
Copy link
Contributor

I also created the two new svg files already linked in the document
denon_mc7000_velocity_sampler_mode
denon_mc7000_pitch_play_mode
.

@holopansel
Copy link
Contributor Author

holopansel commented May 13, 2022

This update belongs to mixxxdj/mixxx#4729

@@ -301,7 +304,8 @@ This mode lets you jump a number of beats while pushing a pad button once.
Sampler Mode (pink LED)
-----------------------

8 samplers can be triggered from either Deck.
8 samplers can be triggered from each Deck if MC7000.SamplerQty is set to 32. Deck 1 will play sampler 1 to 8, Deck 2 will play sampler 9 to 16, Deck 3 will play sampler 17 to 24 and Deck 4 will play sampler 25 to 32.
Copy link
Contributor

@uklotzde uklotzde May 13, 2022

Choose a reason for hiding this comment

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

Most users will simply use the default mapping without any customization. The manual should primarily describe this and simply refer to the script if users intend to customize it.

As a regular user without any ambitions to program JavaScript I find the references to internals of the script more confusing than helpful.

TL;DR IMHO it would be sufficient to describe only the default behavior in the manual and document the options for customization in the script code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set the default to 16 samplers in the script now, so the user does not need to take any action before using, this works with the default skin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment for 32 samplers is now in the script code.

Apart from that the behavior is equal to Sampler Mode.

8 samplers can be triggered from each Deck if MC7000.SamplerQty is set to 32. Deck 1 will play sampler 1 to 8, Deck 2 will play sampler 9 to 16, Deck 3 will play sampler 17 to 24 and Deck 4 will play sampler 25 to 32.
If MC7000.SamplerQty is set to 16, Deck 1 and Deck 3 will play the samplers 1 to 8 and Deck 2 and Deck 4 will play the samplers 9 to 16.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to omit the description of the non-default behavior from the 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.

16 samplers will be the standard and therefore all 32 sampler information be removed from the manual.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

I just left some thoughts about the intended audience of the manual. The existing MC 7000 page is overambitious in this respect and confuses regular users with extra information that is useless for them. Isolating the developer documentation in .. hint:: sections seems reasonable. Sometimes it is not clear what the default setting actually is if I select the controller in the preferences, knowing absolutely nothing about mapping scripts.

The SVG illustrations as a bonus are nice.

@uklotzde
Copy link
Contributor

Please check the failing pre-commit action that complains about some formatting issues. The failing link checks are probably unrelated, ignore them.

@holopansel
Copy link
Contributor Author

holopansel commented May 20, 2022

Thank you so far uklotzde,
I am going to continue here after swiftb0y approved the corresponding merge request. Otherwise there is a manual available for features that are not.

@Swiftb0y
Copy link
Member

that serato flip logo might be trademarked. I think we should remove it just in case.

@ronso0
Copy link
Member

ronso0 commented Feb 13, 2023

Just a hint:
the svg paths/objects should be filled (white I guess) so they are always readable, regardless of the browser used.
This is how it looks like with a dark theme:
image

@holopansel
Copy link
Contributor Author

Thanks Ronso,
will change that.

@JoergAtGithub
Copy link
Member

Pre-Commit is broken now. You can fix this by using pre-commit.patch artifact from the last CI build:
git apply pre-commit.patch
or you install pre-commit locally on your system: https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking

@holopansel
Copy link
Contributor Author

that serato flip logo might be trademarked. I think we should remove it just in case.

uploaded new files.

@holopansel
Copy link
Contributor Author

before I uploaded the files, the background was set to ff (white), after applying the pre-commit patch file it is back to 00. So there is still the dark theme problem. Any ideas how to solve this?

@ronso0
Copy link
Member

ronso0 commented Feb 15, 2023

could you please attach an example SVG (before) that got reverted by pre-commit?

@holopansel
Copy link
Contributor Author

found a solution, will update again.

@ronso0
Copy link
Member

ronso0 commented Feb 16, 2023

I don't see a white background in that image. At least not in the num pad rows

@ronso0
Copy link
Member

ronso0 commented Feb 16, 2023

I think we should squash all commits when you're done. There's a lot of back and forth it seems.

@holopansel
Copy link
Contributor Author

on my browser with dark theme it looks like this:
manual

@ronso0
Copy link
Member

ronso0 commented Feb 16, 2023

I don't know which svg editor you used, but if I download that file you posted last and put a gradient in the layer below it look s like this:
image

@holopansel
Copy link
Contributor Author

holopansel commented Feb 17, 2023

The file used was already outdated by the time you used it, therefore I deleted the post. In the beginning I modified the files using inkscape, all later versions where I posted that I found a solution were modified using a text editor.
So please look at the complete manual https://github.com/holopansel/manual/blob/patch-1/source/hardware/controllers/denon_mc7000.rst with all updated files and let me know if this looks ok on your browser now. Thanks

@ronso0
Copy link
Member

ronso0 commented Feb 17, 2023

I checked the netlify preview and set a dark background. Graphics look good now, thanks.
https://deploy-preview-484--mixxx-manual.netlify.app/hardware/controllers/denon_mc7000.html

I just noticed the controller overvew could be cropped, don't know if you like to squeeze that in as well
https://deploy-preview-484--mixxx-manual.netlify.app/_images/denon_mc7000.svg

@ronso0
Copy link
Member

ronso0 commented Feb 17, 2023

@holopansel Do you work with git locally or with the Github web interface?
I'm asking because these are 22 commits by now and I always appreciate having a somewhat clean git history with meaningful commit messages. Locally, rebasing this branch is easy, I don't know if/how that works with the web interface.

Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

The added documentation of the new features itself is correct!

@holopansel
Copy link
Contributor Author

The many commits result from using the web interface.

@ronso0
Copy link
Member

ronso0 commented Feb 17, 2023

@JoergAtGithub If you agree I'd squash this into 2-3 commits and push to @holopansel's repo?

@JoergAtGithub
Copy link
Member

Yes, I agree!

@holopansel
Copy link
Contributor Author

would also be ok for me.

@JoergAtGithub JoergAtGithub added this to the 2.3.4 milestone Feb 20, 2023
.. hint::
To use default Mixxx behaviour and allow playing multiple samplers at the same time you can set the user variable ``MC7000.prevSamplerStop`` inside the :ref:`JavaScript file<denon_mc7000_uservariables>` to ``false``.

Pitch Mode (green LED)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Pitch Mode (green LED)
Pitch Play Mode (green LED)

I'll fix it.

@ronso0
Copy link
Member

ronso0 commented Feb 22, 2023

@JoergAtGithub wanna take a final look before merge?

@JoergAtGithub
Copy link
Member

LGTM!
The broken links are real, but unrelated to this PR.

@JoergAtGithub JoergAtGithub merged commit 7a750c1 into mixxxdj:2.3 Feb 22, 2023
@JoergAtGithub
Copy link
Member

Thank you!

@holopansel
Copy link
Contributor Author

Thank you all.

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