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

feat: ✨ Finish event handler implementation #6

Merged
merged 43 commits into from
Sep 6, 2024
Merged

feat: ✨ Finish event handler implementation #6

merged 43 commits into from
Sep 6, 2024

Conversation

ion098
Copy link
Collaborator

@ion098 ion098 commented May 24, 2024 •
edited by github-actions bot
Loading

Finished implementation of various events handling functions, there are also some small QOL improvements for Button class.

Download the template for this pull request:

Note

This is auto generated from Add Template to Pull Request

curl -o [email protected]+da35fc.zip https://nightly.link/LemLib/Gamepad/actions/artifacts/1798420397.zip;
pros c fetch [email protected]+da35fc.zip;
pros c apply [email protected]+da35fc;
rm [email protected]+da35fc.zip;

@ion098 ion098 requested a review from Aang099 May 24, 2024 21:41
Aang099
Aang099 previously approved these changes Jun 3, 2024
View reviewed changes
Copy link
Member

@Aang099 Aang099 left a comment

Choose a reason for hiding this comment

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

Looks fine to me, though we definitely need to resolve those TODOs before release.

@ion098 ion098 requested a review from Aang099 June 5, 2024 19:18
@ion098 ion098 dismissed Aang099’s stale review June 25, 2024 17:51

(more changes since review)

@Aang099 Aang099 requested a review from SizzinSeal August 9, 2024 22:57
Aang099
Aang099 reviewed Aug 9, 2024
View reviewed changes
Copy link
Member

@Aang099 Aang099 left a comment

Choose a reason for hiding this comment

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

Please run clang-format, otherwise looks fine to me

SizzinSeal
SizzinSeal requested changes Aug 11, 2024
View reviewed changes
Copy link
Member

@SizzinSeal SizzinSeal left a comment

Choose a reason for hiding this comment

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

Needs some docs. Logic wise, I couldn't find any problems except for one place where an exception may be thrown. We don't want to use exceptions as they could crash whatever thread its running on, and we know our users won't implementing any error handling

include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/recursive_mutex.hpp Outdated Show resolved Hide resolved
include/gamepad/recursive_mutex.hpp Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
@ion098 ion098 requested review from Aang099 and SizzinSeal August 21, 2024 22:09
SizzinSeal
SizzinSeal requested changes Aug 22, 2024
View reviewed changes
Copy link
Member

@SizzinSeal SizzinSeal left a comment

Choose a reason for hiding this comment

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

Documentation needs better formatting

include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/controller.hpp Show resolved Hide resolved
include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/controller.hpp Show resolved Hide resolved
include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/controller.hpp Outdated Show resolved Hide resolved
Aang099
Aang099 reviewed Aug 22, 2024
View reviewed changes
Copy link
Member

@Aang099 Aang099 left a comment

Choose a reason for hiding this comment

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

Provided code is all tested, same comments as Liam. Documentation needs (mostly format) changes but should be fine to merge after

@ion098 ion098 requested review from SizzinSeal and Aang099 August 22, 2024 18:19
SizzinSeal
SizzinSeal requested changes Aug 22, 2024
View reviewed changes
Copy link
Member

@SizzinSeal SizzinSeal left a comment

Choose a reason for hiding this comment

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

Couldn't find any improvements that could be made to the logic, this review is just on style of the code and of the documentation

include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/controller.hpp Outdated Show resolved Hide resolved
include/gamepad/controller.hpp Show resolved Hide resolved
include/gamepad/controller.hpp Show resolved Hide resolved
include/gamepad/controller.hpp Outdated Show resolved Hide resolved
src/gamepad/controller.cpp Outdated Show resolved Hide resolved
src/gamepad/controller.cpp Outdated Show resolved Hide resolved
src/gamepad/controller.cpp Outdated Show resolved Hide resolved
@ion098 ion098 requested a review from SizzinSeal August 23, 2024 16:17
SizzinSeal
SizzinSeal approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@SizzinSeal SizzinSeal left a comment

Choose a reason for hiding this comment

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

LGTM

Aang099
Aang099 approved these changes Aug 24, 2024
View reviewed changes
Copy link
Member

@Aang099 Aang099 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ion098 ion098 changed the title Finish event handler implementation feat: ✨ Finish event handler implementation Sep 5, 2024
@ion098 ion098 merged commit 8280f2f into main Sep 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SizzinSeal SizzinSeal SizzinSeal approved these changes

@Aang099 Aang099 Aang099 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ion098 @Aang099 @SizzinSeal