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

Don't re-install apps that are already installed and disable auto-restore #719

Merged
merged 3 commits into from
Aug 21, 2024

Conversation

grote
Copy link
Collaborator

@grote grote commented Aug 15, 2024

This doesn't re-install apps that are already installed at the same or a newer version. Also, if auto-restore is enabled during install, it disables it and re-enables it when install is complete.

Part of #671

@grote grote force-pushed the only-install-if-not-installed branch from a6fff47 to 750c4b5 Compare August 15, 2024 21:13
Copy link
Collaborator

@t-m-w t-m-w left a comment

Choose a reason for hiding this comment

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

This works! I have a couple of thoughts, but I'm not sure if they even ought to be addressed:

  1. Apps that were already installed do not show app icons, just the generic icon. This looks a little odd alongside apps that were not already installed, as if maybe something went wrong.
  2. Apps that were already installed appear so abruptly - especially if all the apps were already installed - that, as above, it looks like maybe something went wrong.

The apps do have green checkmarks, which indicate everything should be fine, but I could see how a user might get a mixed message from the observed behavior.

Ideally, it could be communicated somehow that the app was already installed, but I don't think it's necessarily a blocker.

t-m-w
t-m-w previously approved these changes Aug 15, 2024
@grote
Copy link
Collaborator Author

grote commented Aug 16, 2024

I think at least the first point should get addressed. However, I can't reproduce it. See this video for how things look for me:

Screen_recording_20240816_094845.mp4

There's a bit of abruptness when all the apps suddenly show up, but doesn't look bad to me.

@grote grote force-pushed the only-install-if-not-installed branch from 750c4b5 to 4bf1265 Compare August 16, 2024 13:48
@grote grote changed the title Don't re-install apps that are already installed Don't re-install apps that are already installed and disable auto-restore Aug 16, 2024
@grote
Copy link
Collaborator Author

grote commented Aug 16, 2024

I added two commits related to disabling auto-restore during install to avoid restoring the same data twice for affected apps.

@t-m-w
Copy link
Collaborator

t-m-w commented Aug 21, 2024

I think at least the first point should get addressed. However, I can't reproduce it.

We spoke about this, and there were a few things that came to mind:

  1. My backup doesn't have APKs, so it couldn't pull icons or names from those.
  2. My backup likely doesn't have icons either, although I'm not really sure how to confirm that. The initial restore selection screen doesn't show either icons or names - is that confirmation enough, maybe?
Screenshot of initial restore selection screen

Shows a lack of icons and names (just shows package name) for 4 apps.

So assuming point 2 above is correct, then this is an edge case not worth worrying much about.

Copy link
Collaborator

@t-m-w t-m-w left a comment

Choose a reason for hiding this comment

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

From a user-facing perspective (no code review or adb, just what I can see happening), this all appears to work fine to me!

@t-m-w
Copy link
Collaborator

t-m-w commented Aug 21, 2024

2. My backup likely doesn't have icons either

I think this was all true. My backup was from a production build of CalyxOS, for which no stable version currently includes the change to include icons in the backups (should be in 5.10.x which is almost stable). So, it did not have icons or names stored.

I made another backup with the latest upstream code along with these changes, and the described/shown issue is not present. Icons and names are shown.

@grote grote merged commit bebb900 into seedvault-app:android14 Aug 21, 2024
1 check passed
@grote grote deleted the only-install-if-not-installed branch August 21, 2024 20:22
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