build: bump sdkMinVersion to 28 (Android 9)#4885
Conversation
a73fb88 to
ec425b2
Compare
Signed-off-by: Jorge Aguado Recio <jaguado@izertis.com>
Signed-off-by: Jorge Aguado Recio <jaguado@izertis.com>
Signed-off-by: Jorge Aguado Recio <jaguado@izertis.com>
ec425b2 to
7aedbb4
Compare
|
Blocked till 4.8.2 is out |
DeepDiver1975
left a comment
There was a problem hiding this comment.
Reviewed as maintainer. The min-SDK bump itself is clean and consistent, and CI is green — nice work. A few non-blocking observations, mostly about the "remove obsolete SDK version checks" acceptance criterion from #4880.
Looks good
build.gradle:sdkMinVersion = 24 -> 28is the single source of truth; all modules (owncloudApp,owncloudData,owncloudDomain,owncloudComLibrary,owncloudTestUtil) referencesdkMinVersion, so the bump propagates consistently.- The removed guards are all genuinely redundant at minSdk 28 (API 28 = Android P):
Build.VERSION_CODES.O(26),.P(28), and the< Pdexopener check are now always-true / always-false, so the simplifications are correct and behaviour-preserving. - Robolectric
@Config(sdk = [..O]) -> [..P]and theDexOpenerunconditional install are appropriate. - Changelog:
changelog/unreleased/4885is present and matchesTEMPLATE.mdand the repo's PR-number naming convention. (Optional nit: you could also add thehttps://github.com/owncloud/android/issues/4880URL line so the fragment links back to the issue.)
Leftover obsolete SDK checks the PR missed (the issue AC asks to "remove obsolete SDK version checks" — these three are now dead/redundant at minSdk 28 and were not caught):
owncloudApp/.../ui/preview/PreviewTextFragment.kt:248—setRolesAccessibilityToMenuItems()still wraps its body inif (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O). This is the exact same accessibility pattern you unwrapped inFileDetailsFragment,MainFileListFragment,PreviewAudioFragment,PreviewImageFragment,PreviewVideoActivity,FileDisplayActivity,FileFragmentandDrawerActivity. This one appears to have been overlooked — worth unwrapping it too for consistency.owncloudApp/.../utils/NotificationUtils.kt:93—if (SDK_INT < Build.VERSION_CODES.O && timeOut \!= null)is now unreachable dead code (a workaround for API < 26 that can never run). Safe to drop theSDK_INT < Opart.owncloudApp/.../extensions/ContextExt.kt:27—@RequiresApi(Build.VERSION_CODES.O)oncreateNotificationChannelis now redundant (minSdk 28 >= 26), especially since this PR removed the< Oearly-return inMainApp.createNotificationChannels().
None of these are blocking or functional regressions — they're dead-code / consistency cleanups that fall within this PR's stated scope. Recommend folding them in (at minimum #1) before merge so the "remove obsolete checks" AC is fully satisfied. The support-policy decision (dropping Android 7/8 / API 24–27) is a product call and is already covered by #4880.
Related Issues
App: #4880
ReleaseNotesViewModel.ktcreating a newReleaseNote()with String resources (if required)QA