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

Use range-limiting methods. #8651

Merged
merged 3 commits into from Jul 20, 2022

Conversation

Isira-Seneviratne
Copy link
Member

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Use range-limiting methods, such as MathUtils.clamp() and Kotlin's coerceIn().

Fixes the following issue(s)

  • Fixes #

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@AudricV AudricV added the codequality Improvements to the codebase to improve the code quality label Jul 18, 2022
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Nice catch ;-)

@Isira-Seneviratne Isira-Seneviratne force-pushed the Use_limiting_methods branch 3 times, most recently from d26d7e3 to c34da64 Compare July 18, 2022 23:44
Isira-Seneviratne and others added 2 commits July 20, 2022 05:36
Co-authored-by: Stypox <stypox@pm.me>
@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Again some code style issues I fixed myself, but this is ready (I tested, for example, the popup and found no issues)

Comment on lines 83 to 87
// Use 1/4 of the width for the preview
final int newWidth = MathUtils.clamp(Math.round(baseViewWidthSupplier.getAsInt() / 4f),
DeviceUtils.dpToPx(10, context),
// Scaling more than that factor looks really pixelated -> max
Math.round(srcWidth * 2.5f));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Use 1/4 of the width for the preview
final int newWidth = MathUtils.clamp(Math.round(baseViewWidthSupplier.getAsInt() / 4f),
DeviceUtils.dpToPx(10, context),
// Scaling more than that factor looks really pixelated -> max
Math.round(srcWidth * 2.5f));
final int newWidth = MathUtils.clamp(
// Use 1/4 of the width for the preview
Math.round(baseViewWidthSupplier.getAsInt() / 4f),
// But have a min width of 10dp
DeviceUtils.dpToPx(10, context),
// And scaling more than that factor looks really pixelated -> max
Math.round(srcWidth * 2.5f));

Comment on lines 21 to 23
public int interpolateOutOfBoundsScroll(@NonNull final RecyclerView recyclerView,
final int viewSize, final int viewSizeOutOfBounds,
final int totalSize, final long msSinceStartScroll) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public int interpolateOutOfBoundsScroll(@NonNull final RecyclerView recyclerView,
final int viewSize, final int viewSizeOutOfBounds,
final int totalSize, final long msSinceStartScroll) {
public int interpolateOutOfBoundsScroll(@NonNull final RecyclerView recyclerView,
final int viewSize,
final int viewSizeOutOfBounds,
final int totalSize,
final long msSinceStartScroll) {

@Stypox Stypox merged commit cacce6d into TeamNewPipe:dev Jul 20, 2022
@Isira-Seneviratne Isira-Seneviratne deleted the Use_limiting_methods branch July 20, 2022 23:42
@Stypox
Copy link
Member

Stypox commented Aug 4, 2022

I got a crash while testing #8656 related to this PR:

  • start playing in a popup in portrait
  • rotate the phone to landscape
  • hold the popup to have it enlarged
  • try to move the popup
java.lang.IllegalArgumentException: Cannot coerce value to an empty range: maximum -175.0 is less than minimum 0.0.
        at kotlin.ranges.RangesKt___RangesKt.coerceIn(_Ranges.kt:1131)
        at org.schabi.newpipe.player.gesture.PopupPlayerGestureListener.onScroll(PopupPlayerGestureListener.kt:241)
        at android.view.GestureDetector.onTouchEvent(GestureDetector.java:704)
        at org.schabi.newpipe.player.gesture.BasePlayerGestureListener.onTouch(BasePlayerGestureListener.kt:27)
        at org.schabi.newpipe.player.gesture.PopupPlayerGestureListener.onTouch(PopupPlayerGestureListener.kt:34)
        at android.view.View.dispatchTouchEvent(View.java:14536)
        at ...

So this PR introduced possible crashes. Could you revert all usages of range-limiting methods in cases where the two extrema are not surely one smaller than the other?

@Isira-Seneviratne
Copy link
Member Author

I got a crash while testing #8656 related to this PR:

  • start playing in a popup in portrait
  • rotate the phone to landscape
  • hold the popup to have it enlarged
  • try to move the popup
java.lang.IllegalArgumentException: Cannot coerce value to an empty range: maximum -175.0 is less than minimum 0.0.
        at kotlin.ranges.RangesKt___RangesKt.coerceIn(_Ranges.kt:1131)
        at org.schabi.newpipe.player.gesture.PopupPlayerGestureListener.onScroll(PopupPlayerGestureListener.kt:241)
        at android.view.GestureDetector.onTouchEvent(GestureDetector.java:704)
        at org.schabi.newpipe.player.gesture.BasePlayerGestureListener.onTouch(BasePlayerGestureListener.kt:27)
        at org.schabi.newpipe.player.gesture.PopupPlayerGestureListener.onTouch(PopupPlayerGestureListener.kt:34)
        at android.view.View.dispatchTouchEvent(View.java:14536)
        at ...

So this PR introduced possible crashes. Could you revert all usages of range-limiting methods in cases where the two extrema are not surely one smaller than the other?

Sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants