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

main: key_handler.cpp: Remove redundant checks #73

Merged
merged 2 commits into from
Jul 20, 2024
Merged

Conversation

Haxk20
Copy link
Contributor

@Haxk20 Haxk20 commented Nov 4, 2023

Hello,

I read trough few files and this one stood out mostly due to redundancy of the if checks.

The if checks would be useful if we did something different for single tap in single or tws mode but we currently use single tap in both modes for pause or play. So no need for the checks.

In quad tap we dont have any action for right or left earbud (I think when we add passtrough mode this could be a good place to enable it) so for now we can simply remove the checks :)

All in all its just a tiny cleanup of the file :)

(This has been tested and all works as expected on my pinebuds)

@shymega shymega requested a review from Ralim November 4, 2023 20:55
@shymega shymega added the enhancement New feature or request label Nov 4, 2023
@Ralim
Copy link
Collaborator

Ralim commented Nov 5, 2023

The intention here was that all tap callbacks follow a similar pattern and it makes it easy for one to adjust if they wanted. I was highly suspect they impact would either compiler optimise out or at the least be exceptionally little impact?

@Haxk20
Copy link
Contributor Author

Haxk20 commented Nov 5, 2023

The intention here was that all tap callbacks follow a similar pattern and it makes it easy for one to adjust if they wanted. I was highly suspect they impact would either compiler optimise out or at the least be exceptionally little impact?

Compiler would not optimize them out due to the trace macro. Same goes for single tap. Compiler would most likely make the function call outside of the if statements and just do the trace macro inside the if statement.

Their impact is of course tiny but I dont see a reason to have redundant if statements just so that the user, developer making the adjustment doesnt need to copy paste it from other tap callbacks.

But if you feel like having them in is more important then please close the PR :)

@shymega
Copy link
Collaborator

shymega commented Nov 5, 2023

I think I'd rather keep these conditionals in - but I'm open to commenting on them out.

This makes the conditionals visible to future developers, in terms of style. I don't think the PR needs to be closed, just rethought.

@shymega shymega self-assigned this Nov 5, 2023
@Haxk20
Copy link
Contributor Author

Haxk20 commented Nov 9, 2023

I think I'd rather keep these conditionals in - but I'm open to commenting on them out.

This makes the conditionals visible to future developers, in terms of style. I don't think the PR needs to be closed, just rethought.

Im for sure open to ideas :)

Wdym by commenting on them out ? As in just leave the code there but actually have it as comments ?

@shymega
Copy link
Collaborator

shymega commented Nov 10, 2023

I mean a block comment. Like:

/**
if (X ==  Y)
**/

@Haxk20
Copy link
Contributor Author

Haxk20 commented Nov 11, 2023

I mean a block comment. Like:

/**
if (X ==  Y)
**/

That could be a good solution indeed. I will update the commit tomorrow

@shymega
Copy link
Collaborator

shymega commented Nov 11, 2023

Thank you!

Haxk20 and others added 2 commits July 19, 2024 19:55
In single tap event there is no need at all to check whether we are in single bud mode or if the event came from left or right earbud in TWS mode. The event is the same.

For quad tap we do nothing at all in TWS mode for now. So remove the checks for right or left earbud.
@shymega
Copy link
Collaborator

shymega commented Jul 19, 2024

Implemented my request, and rebased.

This PR is now ready for merge.

@shymega
Copy link
Collaborator

shymega commented Jul 19, 2024

@Ralim Can we get this merged once tests pass?

@Ralim Ralim merged commit 6c1f50f into pine64:main Jul 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants