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.

Sign up for GitHub

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

Jump to bottom

Modernize and document common-hid-packet-parser.js #4718

Merged
merged 65 commits into from
Nov 20, 2022
Merged

Modernize and document common-hid-packet-parser.js #4718

merged 65 commits into from
Nov 20, 2022

Conversation

JoergAtGithub
Copy link
Member

@JoergAtGithub JoergAtGithub commented Apr 7, 2022 •
edited
Loading

@JoergAtGithub
Adapted eslint configuration for common-hid-packet-parser.js
95cc223 
Applied eslint changes for common-hid-packet-parser.js
Swiftb0y
Swiftb0y reviewed Apr 7, 2022
View reviewed changes
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

I don't know how much work you want to put into this. A lot of things could be improved, but I don't want to force you to do too much work. So what scope did you intend for this PR? Just make the new ESLint config pass, improve docs or even modernize large parts the library?

res/controllers/common-hid-packet-parser.js Outdated Show resolved Hide resolved
@JoergAtGithub
Copy link
Member Author

So what scope did you intend for this PR?

No large changes, only cleanup and one small functional extension to initialize data.

@JoergAtGithub JoergAtGithub force-pushed the hid-parser-update branch 2 times, most recently from 2122906 to 030f286 Compare April 29, 2022 19:50
@JoergAtGithub
Use ES6 class syntax
597d1f2 
Some types documented
@JoergAtGithub JoergAtGithub changed the title WIP: Modernize common-hid-packet-parser.js Modernize common-hid-packet-parser.js Apr 29, 2022
@JoergAtGithub JoergAtGithub changed the title Modernize common-hid-packet-parser.js Modernize and document common-hid-packet-parser.js Apr 30, 2022
@JoergAtGithub JoergAtGithub marked this pull request as ready for review April 30, 2022 09:00
@JoergAtGithub JoergAtGithub requested a review from Swiftb0y April 30, 2022 10:40
@Swiftb0y Swiftb0y self-assigned this Apr 30, 2022
@JoergAtGithub
Added missing type for bit to JSDoc
b890d51 
Removed one duplicate line
@daschuer
Copy link
Member

daschuer commented Jul 4, 2022

I have left some comments. Not sure what will fit in the PR or should be postponed.

@JoergAtGithub
Replaced "Record." by "Object.", because Record is TypeScript syntax …
af39091 
…and not valid JSDoc.

--
Unfortunately I had to replace the typedef packetItemId, because the JSDoc Object. syntax for records requires type string as index.
--
For type compatibility with TypeScript (and TypeScript based tools like VS Code), the keyword Object in JSDoc must be upper case. To prevent such tools reporting false positive errors, the setting of eslint-plugin-jsdoc (this case is described here: https://github.com/gajus/eslint-plugin-jsdoc/blob/master/README.md#why-not-capital-case-everything)
@JoergAtGithub
Clarify, that "play" is just an example
64251b2
@JoergAtGithub
Use JSDoc statements for deprecated info of processDelta
9a872e6
@JoergAtGithub
Deprecated unused HIDController.OutputUpdateInterval
651089a
@JoergAtGithub
Used @default syntax - According the JSDoc spec, the value is optiona…
96f1052 
…l, I added it nevertheless, because VS Code doesn't print it otherwise
@JoergAtGithub
Replaced all strings with embedded expressions by template strings
f54a65a
@JoergAtGithub
Corrected type of packetField.pack
057ad16 
Added type for HIDPacket.packSizes
Added missing information about endianess of pack types
@JoergAtGithub
Added missing type/type cast
661ddaa
@JoergAtGithub
Added missing @type to HIDPacket.groups
42923dc
@JoergAtGithub
This was the only place where the keyword initializePacketData exist …
87269ee 
…in the Mixxx codebase, and the comment that it don't work is there since the beginning of the Mixxx Git history.
@JoergAtGithub
Tried to explain, what this.timers = {}; is used for.
ea794af
@JoergAtGithub
Copy link
Member Author

@Swiftb0y I addressed now all open review comments. I would be glad, if you could confirm, that I did this in your sense.

@Swiftb0y
Copy link
Member

Yes, thank you for your effort. Unfortunately, as I pointed out, I only reviewed the first third. I don't know when or even if I'll get to the rest sometime.

@JoergAtGithub
Copy link
Member Author

Yes, I fully understand this. Therefore I requested no re-review from you.
But if you aggree, that my changes address your change requests, it would be nice, if you could reset the Changes Requested state.

@Swiftb0y Swiftb0y dismissed their stale review October 11, 2022 22:54

first third lgtm, rest still needs review.

daschuer
daschuer approved these changes Nov 15, 2022
View reviewed changes
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.

I did another review round with my limited JS knowledge.
And I did not spot obvious issues.
Given that this a PR just started for documentation purpose, it is already mature.
Thank you for all the work.
Can you confirm that this is ready for merge and has been tested with HID Hardware?

@Swiftb0y and @ywwg Did you had a chance to test this on your hardware?
If you don't have objections, I will merge this in a few days.
Thank you.

@JoergAtGithub
Copy link
Member Author

@daschuer Thank you very much for reviewing this!
From my point of view, it's ready to merge! But I've only one HID controller (Traktor Z2 with the mapping from #3038) available for test.
Practical testing with other HID mappings makes definitally sense. To get maximum feedback from users with other controllers, this PR should be merged before the next Beta test cycle.

@Swiftb0y
Copy link
Member

@Swiftb0y and @ywwg Did you had a chance to test this on your hardware?

I don't have any HID hardware or mapping to test this with. I was only reviewing the code because I have a somewhat determined opinion on how we should handle typing in our JS libraries.

Swiftb0y
Swiftb0y requested changes Nov 15, 2022
View reviewed changes
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Allow me another couple days to review the rest of the code.

Swiftb0y
Swiftb0y requested changes Nov 16, 2022
View reviewed changes
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

little nitpicks. I highly appreciate all the effort put into this code, yet I still don't understand the library even the slightest. In the end I'd call for the deprecation of this library and would favor a smaller library based on TypedArrays which integrates into componentsJS. Until then, I highly appreciate all the work you put spend so far. LGTM apart from my two nitpicks.

res/controllers/common-hid-packet-parser.js Outdated Show resolved Hide resolved
res/controllers/common-hid-packet-parser.js Show resolved Hide resolved
@JoergAtGithub JoergAtGithub mentioned this pull request Nov 19, 2022
Open
@JoergAtGithub
Copy link
Member Author

In the end I'd call for the deprecation of this library and would favor a smaller library based on TypedArrays which integrates into componentsJS.

A rewrite from scratch makes sense, but before starting with this task, I would like to improve the C++/JavaScript HID API. Currently all the semantic information, which HID provides about the individual controls of a device, isn't accessible in the JS mapping code.

Swiftb0y
Swiftb0y approved these changes Nov 20, 2022
View reviewed changes
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thank you. Sorry for taking so long.

@Swiftb0y Swiftb0y merged commit 8affbbb into mixxxdj:main Nov 20, 2022
@JoergAtGithub
Copy link
Member Author

Thank you for the detailed review!

@JoergAtGithub JoergAtGithub deleted the hid-parser-update branch November 20, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Swiftb0y Swiftb0y Swiftb0y approved these changes

@daschuer daschuer daschuer approved these changes

Assignees

@Swiftb0y Swiftb0y

Labels
code quality controller mappings needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JoergAtGithub @daschuer @Swiftb0y