pine64 / OpenPineBuds

Community maintained firmware for PineBuds Pro
150 stars 19 forks source link

main: key_handler.cpp: Remove redundant checks #73

Closed Haxk20 closed 4 months ago

Haxk20 commented 1 year ago

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)

Ralim commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

Haxk20 commented 1 year ago

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 commented 1 year ago

I mean a block comment. Like:

/**
if (X ==  Y)
**/
Haxk20 commented 1 year ago

I mean a block comment. Like:

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

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

shymega commented 1 year ago

Thank you!

shymega commented 4 months ago

Implemented my request, and rebased.

This PR is now ready for merge.

shymega commented 4 months ago

@Ralim Can we get this merged once tests pass?