[Android 14] Update compileSdkVersion to 34 #740
Conversation
There was a problem hiding this comment.
LGTM! 💯
I have left few questions (❓) and a couple of suggestions (💡) comments for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.
| @@ -405,8 +405,8 @@ private class FlingGestureListener extends SimpleOnGestureListener { | |||
| private static final int SWIPE_MIN_DISTANCE = 120; | |||
| private static final int SWIPE_THRESHOLD_VELOCITY = 200; | |||
|
|
|||
| @Override public boolean onFling(MotionEvent e1, MotionEvent e2, float velocityX, float velocityY) { | |||
| if (e1 != null && e2 != null) { | |||
| @Override public boolean onFling(MotionEvent e1, @NonNull MotionEvent e2, float velocityX, float velocityY) { | |||
There was a problem hiding this comment.
Suggestion (💡): Since you added the missing @NonNull nullability annotation for e2, I suggested adding the @Nullable for e1 as well, just to make it match the super method's signature and making it explicit why e1 needs to be checked.
There was a problem hiding this comment.
That makes sense. Not sure why I didn't add it. Adding now.
| @@ -2292,7 +2292,9 @@ abstract class ComposeLoopFrameActivity : AppCompatActivity(), OnStoryFrameSelec | |||
| } | |||
|
|
|||
| private inner class FlingGestureListener : GestureDetector.SimpleOnGestureListener() { | |||
| override fun onFling(e1: MotionEvent, e2: MotionEvent, velocityX: Float, velocityY: Float): Boolean { | |||
| override fun onFling(e1: MotionEvent?, e2: MotionEvent, velocityX: Float, velocityY: Float): Boolean { | |||
| if (e1 == null) return super.onFling(null, e2, velocityX, velocityY) | |||
There was a problem hiding this comment.
Question (❓): This still ends-up in a valid application flow, right? 🤔
There was a problem hiding this comment.
Based on the logic here, we don't really know what to do if e1 is null, and the fallback is to just call the super anyway, so I kept calling the super implementation anyway.
I just realized though that when this fling is properly captured, the code is currently crashing because of a bug in a different part of the code (not related to this compileSdkVersion update). I will open a different PR to fix it.
| seekBar.progress = (fontSizeSp - TEXT_SIZE_SLIDER_MIN_VALUE) / TEXT_SIZE_SLIDER_STEP | ||
| val fontSizeSp = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE) { | ||
| // this takes into account text specifics (such as font size multipliers in system settings) | ||
| TypedValue.deriveDimension( |
There was a problem hiding this comment.
- Praise (❤️): Nice find and thanks for enhancing this
update()function. - Suggestion (💡): I would have recommended making this change in a separate commit and explaining the how & why in that commit's description. Maybe doing so would have made the extra comment included there obsolete, but to be honest, on this I am not 100% sure on that and whether I would have kept that comment around anyway... 🤷
- Question (❓): Was this change indeed fixing compilation errors, thus part of this commit, or was that change something totally different? As I understand, only adding the
@Suppress("DEPRECATION")was needed to fix compilation errors, right? 🤔
There was a problem hiding this comment.
I would have recommended making this change in a separate commit and explaining the how & why in that commit's description.
That is a good point, I will rewrite the commit history to add this change in a separate commit. I will likely keep the comment though.
As I understand, only adding the
@Suppress("DEPRECATION")was needed to fix compilation errors, right?
Well, adding the @Suppress("DEPRECATION") does indeed get rid of compilation errors, but doesn't really "fix" them in the sense of favoring the new API when possible, so I would consider this enhancement as part of the compileSdk changes. Wdyt? As you said in #2, I will likely move this enhancement to a separate commit, while keeping the @Suppress as part of the compileSdkcommit.
The new deriveDimension API introduced in Android 14 takes into account text specifics, such as font size multipliers set by the user in the system settings. The enhancement here uses this API when running on Android 14 while using the old (and now deprecated) scaledDensity calculation.
thomashorta
force-pushed
the
update-compile-sdk-version-34
branch
from
0e21f06 to
b388ab5
Compare
Parent: wordpress-mobile/WordPress-Android#19106
Changes were needed in the code to fix compilation issues.
No new lint warnings/errors were raised.
Note: This doesn't deal with partial media access on the app module.