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

JS: Add badusb.altPrint() and badusb.altPrintln() to avoid layout issues on Windows targets #33

Merged
merged 20 commits into from
Mar 19, 2024

Conversation

oldip
Copy link
Contributor

@oldip oldip commented Mar 14, 2024

What's new

This PR implements the addition of Numpad keybindings in js_badusb.c, addressing the feature request (#30). The implementation allows users to use ALT+Numpad combinations for input, enhancing functionality for non-US input methods.

Implemented changes include:

  • Added keybindings for HID_KEYPAD_0 to HID_KEYPAD_9.
  • Updated the sendStringViaAltNumpad function to support these keybindings.

For the reviewer

This update is in response to the previously discussed feature request #30. The modifications are intended to improve the firmware's usability for non-US layouts by allowing ALT+Numpad combinations for text input.

  • I've uploaded the firmware with this patch to a device and verified its functionality.
  • I've confirmed the feature to be stable and the bug to be fixed with these changes.

Reference to the original feature request for more context:

Please review the changes and consider merging this PR to enhance the project's functionality. Thank you for your consideration.

@oldip
Copy link
Contributor Author

oldip commented Mar 18, 2024

Dear @Willy-JL , @Sil333033 , @HaxSam , and @MatthewKuKanich ,

I hope you're all doing well. I'm reaching out to kindly request your review on my pull request, #33: "Add numpad keybindings to improve non-US input methods". This contribution aims to enhance our beloved Momentum-Firmware project by introducing Numpad keybindings, a feature I believe many in our community have been looking forward to.

Having followed this project for some time, I've grown to admire the robustness and versatility it offers. It's been an inspiration to contribute and I'm excited about the possibility of adding to its functionality. This update, I believe, could make a significant difference for users with non-US input methods, and I'm eager for it to be reviewed by minds as capable as yours.

I've taken the liberty of addressing the initial feedback and ensuring that my contributions are in line with our project's standards and objectives. I've thoroughly tested the changes and I'm open to any further suggestions you might have to refine and improve this implementation.

Your insights and feedback would be immensely valuable to me and to the project. I understand the demands on your time and truly appreciate any guidance you can provide.

Thank you very much for your time and for the incredible work you've done with Momentum-Firmware. Looking forward to possibly hearing from you soon.

Warm regards,
Oldip

@Willy-JL
Copy link
Member

so, a few thoughts

while this may work, its not complete under a few aspects. it doesnt respect the same error checking that the exiting badusb.print functions have, like checking for badusb being setup and checking correctly the timeout value to be valid. rather, merging this new functionality into the existing print function (on the backend) would make more sense. this way, the error checking and looping is already done, just need to swap whether its pressing the keys normally, or doing numpad keys.

this also does not check if numlock is enabled. altstring only works while numlock is enabled, the badusb app checks for numlock state and enables numlock if needed.

please stick to a consistent namin convention, js_badusb_altPrint() is a little out of place...

while comments usually are usually good, there are points where its a bit out of place... some things are self explanatory but have multiple redundant comments, while others that would highly benefit from a clarification have nothing. i dont mean to be rude, but this feels AI generated. thats not to say its a problem, but some human intervention and review is always good to have, even more so in languages like C where the AI probably wont be aware of complex memory management (memory wasnt an issue here, but just keep that in mind)

there is also no example of this function being used, which would be beneficial since it would be hard to discover its existence without it

i'm taking the liberty of addressing these couple points, and will gladly merge this afterwards. thank you!

@Willy-JL Willy-JL merged commit 1a2c029 into Next-Flip:dev Mar 19, 2024
2 checks passed
@Willy-JL Willy-JL changed the title Add numpad keybindings to improve non-US input methods JS: Add badusb.altPrint() and badusb.altPrintln() to avoid layout issues on Windows targets Mar 19, 2024
@Willy-JL Willy-JL added the feature New feature or request label Mar 19, 2024
@oldip oldip deleted the feature/numpad-keybindings branch March 19, 2024 10:14
@oldip
Copy link
Contributor Author

oldip commented Mar 19, 2024

Thanks, @Willy-JL ! I'm deeply thankful for your review and the subsequent merge of my pull request. I genuinely appreciate the effort you've put into enhancing the contribution to meet the high standards of Momentum-Firmware.

Initially, I received some help from ChatGPT-4, but I encountered issues since the code didn't work as expected. This challenge led me to explore ducky_script.c for ducky_altchar and ducky_altstring methods, and I made an effort to adhere to the format used in js_badusb.c for consistency.

My background in coding is almost entirely at a beginner level in Java and Python, and I've had no formal training in C. Thus, this experience has been a steep learning curve for me. Your insights on the limitations of AI-generated code, especially regarding memory management and the importance of code review, have been incredibly helpful. I'll keep these in mind as I move forward.

I must express my admiration for you and your team's work on Momentum-Firmware. Transitioning from xfw to Momentum-Firmware was an easy decision, amazed by the stability and functionality your firmware offers. Your profile, showcasing your projects and achievements at such a young age, is truly inspiring. It's an honor to have my contribution considered and adopted by someone I hold in high regard.

Thank you once again for the opportunity to contribute to such an impactful project. The acceptance of my suggestions and the welcoming approach to contributions from individuals with different levels of expertise is very encouraging. I look forward to continuing to learn from this experience and contributing to the future of Momentum-Firmware in any way I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants