Minimixxx controller config

[ywwg]

There is currently no documentation for this config, but I will work on that. Seeing as I am the only person on Earth with this controller that seems ok for now :)

[Holzhaus]

Seeing as I am the only person on Earth with this controller that seems ok for now :)

If there is zero chance that anyone else has this controller, does it really make sense to add it to the mixxx source tree and ship it to every single user?

Why not just keep it in your personal mixxx config dir? Or are you planning to start mass-production soon? 🙂

[Swiftb0y]

I don't think it makes sense to add this to the repo as well. Since nobody is going to use this code, its only purpose would be to serve as a best-practice example to beginners. However, glossing over the code for a minute, I see a lot of issues that would need to be ironed out to the point where I'd consider rewriting the mapping entirely.
Please do not misinterpret this as a personal attack, I just don't think it makes sense to merge this in its current state.

[ronso0]

Maybe mentioning it in the changelog draws attention to it.
But maybe a blog post would be better? "open source hardware & software"

[Be-ing]

I agree, it doesn't make much sense to ship this with Mixxx if the device isn't readily commercially available. If you want to post it on the forum and tell users how they can order their own controller like yours that would be cool though.

[Holzhaus]

Why not use an enum and a switch statement instead of string comparison?

[ywwg]

oh hah I didn't know js could do that. I'll fix

[ywwg]

done

[Swiftb0y]

I'm also not sure what advantage I would gain by doing all that work -- the config works quite nicely.

Not much, I'm just personally worried about increased maintenance work with this extra code. If you assure that you will maintain this mapping I don't really have a problem with your custom library, though in the future, I'd prefer if we tried to reiterate on the existing design instead of inventing the wheel from scratch. I'm aware that the current approach to switching functionality within a component is not ideal.

[Be-ing]

I share @Swiftb0y's concern about writing an ad hoc JavaScript library for this one mapping that serves the same purpose of the library we already have; this seems like a not-invented-here issue. You don't have to rewrite a fully working PR from scratch, but I took a brief look at the code but quickly got discouraged from reviewing it. I don't want to bother learning a completely different system for every mapping and I think it's asking a lot of reviewers to do so. If you have an issue with the Components library, we can discuss that, especially as we are making backwards incompatible changes in #4171.

[ywwg]

yeah next time I would try using the existing lib. And yes, I will maintain the mapping.

[ywwg]

documentation in progress: mixxxdj/manual#456

[Swiftb0y]

Please take care of the pre-commit errors before I'll start to review (silence them if absolutely necessary).

[ywwg]

@Swiftb0y errors fixed

[Swiftb0y]

Thank you. I'll review shortly, feel free to ping if I forget to do it in the next 7 days.

[Swiftb0y]

Code looks good. Had some minor nitpicks but nothing worth leaving a comment for.
However, I'd propose to add a disclaimer at the top though to discourage beginners to copy paste this code into their own mappings. Most of the code relies on the architecture of the mapping with your custom library, so users would need to copy that library as well, which we don't want since we agreed that it would not be maintainable to introduce another library we have to support. Is that ok for you?

[ywwg]

yeah no problem -- I can add a comment that people should use the component library if they are looking for copypasta.

[Swiftb0y]

Thanks

[Holzhaus]

@Swiftb0y we should not merge new mappings without also merging the corresponding manual PR (mixxxdj/manual#456 in this case).

[Swiftb0y]

I'm sorry, forgot about that. Doesn't make much sense to revert the PR now, right?

[Holzhaus]

I'm sorry, forgot about that. Doesn't make much sense to revert the PR now, right?

No, let's just try to remember it next time.😅

[daschuer]

@Swiftb0y: Please create a merge commit next time. Else it is hard to track changes among all the "Merge pull request #4521 from ... " messages. Also the individual commit messages are loosing the context.
In case you like to eliminate faulty commits, rebase the branch before merge.

[Swiftb0y]

We need to write down proper guidelines for this. This has been more than once that I merged too soon or merged in the wrong mode. As a result, I would simply not press merge anymore if I have to take the blame for each little mistake.

[Holzhaus]

I think the "let's merge mappings when both documentation and code are ready" is known. You forgot it this time, no big issue, just try to remember it next time ;-)

Regarding merge commit vs. squash: I disagree with @daschuer and I think squash+merge was the right call, the commit messages were not that helpful anyway.

[Swiftb0y]

I think the "let's merge mappings when both documentation and code are ready" is known.

Yes that is known, though writing it down into a checklist would still make sense so I can take a look before merging to make sure I didn't forget anything.

[daschuer]

@Holzhaus In this case it is not worth to discuss it. But let's clarify our approach for the future:

Please have a look at the commit message here:
c0679a2

The original commit messages are still there, but useless because they are decoupled from the original commit.

The other issue is the produced git history:
I am currently crawling down the commit history from merge commit to merge commit by searching for:
"Merge pull request #4545" to write the CHANGELOG (still 190 Merges left :-/ )
I need to be careful not to skip the squashed merges. If we now consider sqash merges with edited commit message we will have hard times to follow the original main branch and not branch into a PR.
A main branch with only merge commits is a better alternative here.

That are the reasons for our rule in https://github.com/mixxxdj/mixxx/blob/main/CONTRIBUTING.md
Merge PRs using a merge , to keep the original commits valid. Keep the default commit message "Merge pull request ..." with the reference to the pull request. In case where the PR contains broken (non-building) commits, back-and-forth commits or commits without a meaningful commit message that are not worth keeping, ask the author to squash the commits before merge or use squash-and-merge if the author is not fluent in git.

In this case the commit messages where not so much cluttered to be useless so I would have preferred to just merge them as they are. If you really want to go the extra mile for a cleaner history, which is IMHO not worth the effort.
You could have ask @ywwg to clean up the history for more reasonable commit messages and then merge.

In general, I prefer to put important info as comment into the code, this is a better invest of time.

Does this sound reasonable? Do we need an adjustment of our rules?

[Holzhaus]

The original commit messages are still there, but useless because they are decoupled from the original commit.

I usually edit the commit message for the squash commit to something like "Add MiniMixxx controller mapping" 😉

I am currently crawling down the commit history from merge commit to merge commit

Why? For the changelog we have the "changelog" label now. PRs without that label should not be mentioned in the changelog.

In this case the commit messages where not so much cluttered to be useless so I would have preferred to just merge them as they are. If you really want to go the extra mile for a cleaner history, which is IMHO not worth the effort.
You could have ask @ywwg to clean up the history for more reasonable commit messages and then merge.

In this case we could do that, but most mapping contributors are not fluent in git, and IMHO the bar to get a mapping g merged is already pretty high. We try to have good code/documentation quality without scaring away contributors. For that reason I think squash+merge is preferable over asking mapping contributors to do squash/rebase manually.

In general, I prefer to put important info as comment into the code, this is a better invest of time.

Yes.

[daschuer]

Ok, fine, we are basically on the same line.

So the mayor difference between our opinion is when the squash+merge is preferred.

In terms of the lowest bar, I prefer just merge. There is no harm with an extra commit like "MiniMixxx: cleanup" or such.
If you insist to squash, you need to write a new commit message that describes the new commit. If we want to keep the single commits messages in a list like it was done here, we should also keep the commits.

Can we agree to that?

Regarding our Changelog lable, I do not really trust it. Can this replace a systematic approach looking at all PRs?
Even though if we do remember all the time to set the flag, you do not see the final picture when writing the CHANGELOG. you need to decide which one is worth to not and which not.

For now I continue for 2.4 with the style already started: https://github.com/mixxxdj/mixxx/blob/main/CHANGELOG.md

Is this OK?