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

ANDROID-15230 Fix suppressed long method #391

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

jeprubio
Copy link
Contributor

@jeprubio jeprubio commented Sep 25, 2024

🥅 What's the goal?

Fix suppress LongMethod

🚧 How do we do it?

  • Remove the suppressions of LongMethod.
  • Added some private methods to make the previous long methods smaller.
  • Scoutting: Removing the annotation ExperimentalPagerApi which was then suppressed in the same file with the OptIn.

🧪 How can I test this?

  • Video checking the updated code of lists and progress buttons:
UpdatedCode.mov

@Telefonica Telefonica deleted a comment from github-actions bot Sep 26, 2024
@Telefonica Telefonica deleted a comment from github-actions bot Sep 26, 2024
Copy link

📱 New catalog for testing generated: Download

setVisibilityAndColors()
}

fun getText(): CharSequence =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved the public methods on top of the private ones I've created

Comment on lines +70 to +77
setupAttributes(attrs, defStyleAttr)
setupViewProperties()
setupButtonNormal()
setupButtonLoading()
setupProgressBar()
setButtonBackground()
addViews()
setVisibilityAndColors()
Copy link
Contributor Author

@jeprubio jeprubio Sep 26, 2024

Choose a reason for hiding this comment

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

Code split in private methods

@@ -156,8 +154,6 @@ fun CarouselPagerIndicator(
indicatorShape = indicatorShape)
}

@ExperimentalPagerApi
Copy link
Contributor Author

@jeprubio jeprubio Sep 26, 2024

Choose a reason for hiding this comment

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

These Experimental annotations are wrong, they were hidden by the OptIn() on the top but they shouldn't have been there. This one should have been an OptIn() one.

@jeprubio jeprubio marked this pull request as ready for review September 26, 2024 08:15
@jeprubio jeprubio requested review from a team, DevPabloGarcia and yamal-alm and removed request for a team September 26, 2024 08:15
MotionEvent.ACTION_UP, MotionEvent.ACTION_CANCEL -> {
buttonNormal.isPressed = false
buttonLoading.isPressed = false
view.performClick()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this line needed? was it being handled in other side?

Copy link
Contributor Author

@jeprubio jeprubio Sep 26, 2024

Choose a reason for hiding this comment

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

Detekt marks it as error if view.performClick() is not invoked on a setOnTouchListener lambda

onTouch lambda should call View#performClick when a click is detected

Explanation for issues of type "ClickableViewAccessibility":
If a View that overrides onTouchEvent or uses an OnTouchListener does not also implement performClick and call it when clicks are detected, the View may not handle accessibility actions properly. Logic handling the click actions should ideally be placed in View#performClick as some accessibility services invoke performClick when a click action should occur.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried about any unexpected behavior and if it wasn't added before for a reason.. but I guess the button is still working as before right?

Copy link
Contributor Author

@jeprubio jeprubio Sep 26, 2024

Choose a reason for hiding this comment

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

Yup, I made a video in the pr description which checks the 2 updated elements (the progress button and the list in compose)

Copy link
Contributor

@yamal-alm yamal-alm left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

@DevPabloGarcia DevPabloGarcia left a comment

Choose a reason for hiding this comment

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

Thanks!

@jeprubio jeprubio merged commit 54074d1 into main Sep 26, 2024
5 checks passed
@jeprubio jeprubio deleted the feature/ANDROID-15230-FixSuppressedLongMethod branch September 26, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants