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.

Sign up for GitHub

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

Jump to bottom

Raise Minimum API Level to Android 7.1 and Remove Legacy Contacts Sync Workaround #1090

Closed
wants to merge 9 commits into from
Closed

Raise Minimum API Level to Android 7.1 and Remove Legacy Contacts Sync Workaround #1090

wants to merge 9 commits into from

Conversation

ArnyminerZ
Copy link
Member

@ArnyminerZ ArnyminerZ commented Oct 21, 2024 •
edited
Loading

Purpose

Drop suppport for Android 7.0. Contacts sync requires a dirty workaround whose code is not actively maintained anymore and causes unnecessary complexity.

According to https://apilevels.com/ Android 7.0 usage is 97.2% Android 7.1 is 96.0%, so basically no one will notice.

Short description

  • Increase required API level to Android 7.1
  • Drop all the LocalAddressBook.verifyDirty workaround logic
  • SyncManager.prepare returns Unit now. Result not used anymore after getting rid of verifyDirty

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@ArnyminerZ ArnyminerZ linked an issue Oct 21, 2024 that may be closed by this pull request
Drop Android 7.0 contacts workaround, require Android 7.1 #1087
Closed
2 tasks
@ArnyminerZ ArnyminerZ changed the title 1087 drop android 70 contacts workaround require android 71 Drop Android 7.0 contacts workaround, require Android 7.1 Oct 21, 2024
@ArnyminerZ ArnyminerZ changed the title Drop Android 7.0 contacts workaround, require Android 7.1 Drop Android 7.0 support Oct 21, 2024
@ArnyminerZ ArnyminerZ self-assigned this Oct 21, 2024
@ArnyminerZ ArnyminerZ added the refactoring Internal improvement of existing functions label Oct 21, 2024
@ArnyminerZ ArnyminerZ marked this pull request as ready for review October 21, 2024 09:21
rfc2822
rfc2822 reviewed Oct 21, 2024
View reviewed changes
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Looks good, although we need a better (more positive) title for the release logs ;) Like "Drop Android 7 contacts workaround logic"

Also, there are methods in LocalContact (for the hashcodes) that should now also be obsolete. Please check.

@rfc2822 rfc2822 changed the title Drop Android 7.0 support Raise Minimum API Level to Android 7.1 and Remove Legacy Contacts Sync Workaround Oct 21, 2024
@rfc2822 rfc2822 mentioned this pull request Oct 22, 2024
Closed
4 tasks
rfc2822
rfc2822 previously approved these changes Oct 23, 2024
View reviewed changes
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Looks good! However I'll wait with merging because it increases the minimum SDK and if we need another 4.4.3.3 hotfix, I'd prefer to create it from the main branch without having an increased minimum SDK requirement.

@ArnyminerZ @rfc2822
Increased minSdk to 25
9e3245a 
Signed-off-by: Arnau Mora Gras <[email protected]>
@ArnyminerZ @rfc2822
Got rid of verifyDirty.
7b9d9dc 
Prepare now returns Unit

Signed-off-by: Arnau Mora Gras <[email protected]>
@ArnyminerZ @rfc2822
Got rid of unnecessary functions
bfd9f51 
Signed-off-by: Arnau Mora Gras <[email protected]>
@ArnyminerZ @rfc2822
Fixed missing removals
42fee31 
Signed-off-by: Arnau Mora Gras <[email protected]>
@rfc2822 rfc2822 force-pushed the 1087-drop-android-70-contacts-workaround-require-android-71 branch from a19cd65 to 42fee31 Compare October 23, 2024 10:32
@rfc2822
Copy link
Member

rfc2822 commented Oct 27, 2024

Can you please rebase? Since there we no further issues with 4.4.3.2 we can merge then. 👍🏻

@ArnyminerZ
Merge branch 'main-ose' into 1087-drop-android-70-contacts-workaround…
f5b8bed 
…-require-android-71

# Conflicts:
#	app/src/main/kotlin/at/bitfire/davdroid/sync/ContactsSyncManager.kt
@ArnyminerZ
Fixed invalid import
ea30587 
Signed-off-by: Arnau Mora Gras <[email protected]>
rfc2822
rfc2822 reviewed Oct 28, 2024
View reviewed changes
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

I noticed that we check for Android <O, which is Android 8 / API level 26.

So in the previous code, the workaround is also active for Android 7.1.

Can you please double-check that it's not required for Android 7.1 in the emulator?

  1. Create an account, sync contacts
  2. Fake-call a synced contact or write a fake SMS to a contact in the emulator
  3. Look whether the contact is dirty / uploaded again (which should not be the case).

In contrast, the same procedure (without the workaround, but on Android 7.0) should make the contact dirty and upload it without need.

@rfc2822 rfc2822 dismissed their stale review October 28, 2024 10:37

We forgot to check whether Android 7.1 is really OK without workaround (old code uses it until Android 8)

@rfc2822
Copy link
Member

rfc2822 commented Oct 28, 2024

These was a related behavior change in Android 8.0: https://developer.android.com/about/versions/oreo/android-8.0-changes#cpu

And in the old code we wrote Android <O and included 7.1 in the workaround, so I suspect that it's still necessary for 7.1.

But please check…

@ArnyminerZ
Copy link
Member Author

Look whether the contact is dirty / uploaded again (which should not be the case).

Yes, it's uploaded again on SDK 25

@ArnyminerZ
Copy link
Member Author

What should we do? Rollback some changes or increase the minSdk to Android 8...?

@rfc2822
Copy link
Member

rfc2822 commented Oct 31, 2024

Hm honestly I'd like to drop this code… then 4.4.3.2 would have been the latest release until Android 8.

What do you think @devvv4ever @sunkup?

@devvv4ever
Copy link
Member

devvv4ever commented Oct 31, 2024 •
edited
Loading

Would be ok. We then still support 6 years of Android releases, and people will still be able to load the older version anyway

@ArnyminerZ
Copy link
Member Author

Then increasing to 26...

@ArnyminerZ
Increase min SDK to 26
7874078 
Signed-off-by: Arnau Mora Gras <[email protected]>
@ArnyminerZ
Copy link
Member Author

I'll test again on 26 just to be sure, but it should work 🙄

@rfc2822
Copy link
Member

rfc2822 commented Oct 31, 2024 •
edited
Loading

Then we could remove other code that's only run on <O, too.

On the other side, it works (although specific code parts are unmaintained), and according to https://apilevels.com/, Android 8.0 currently has "only" a cumulative usage of 95.7%.

So that means 4.3% wouldn't get further updates, and as DAVx5 users are often tech-savvy and use custom ROMs etc, I bet we even have a higher percentage of old versions.

Really difficult decision …

@ArnyminerZ
Copy link
Member Author

I have just tried, and it works well on SDK 26

@ArnyminerZ
Copy link
Member Author

On the other side, it works (although specific code parts are unmaintained), and according to https://apilevels.com/, Android 8.0 currently has "only" a cumulative usage of 95.7%.

Yeah... That's also true. I mean, it doesn't do any bad to hold the code there, as long as it doesn't cause any issues

@rfc2822
Copy link
Member

rfc2822 commented Nov 1, 2024 •
edited
Loading

I mean we could also do the "proper" way: create a module (class) for handling the dirty check (ContactDirtyVerifier or something like that), and then only use it on Android 7.

If we then want to remove it one day, we only have to remove the class and the locations where it's called.

@rfc2822 rfc2822 mentioned this pull request Nov 2, 2024
Closed
@rfc2822
Copy link
Member

rfc2822 commented Nov 2, 2024

Closed in favor of #1118

@rfc2822 rfc2822 closed this Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfc2822 rfc2822 rfc2822 left review comments

Assignees

@ArnyminerZ ArnyminerZ

Labels
refactoring Internal improvement of existing functions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop Android 7.0 contacts workaround, require Android 7.1
3 participants
@ArnyminerZ @rfc2822 @devvv4ever