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

feat: Add installer status dialog #1473

Merged
merged 36 commits into from
Aug 29, 2024
Merged

Conversation

oSumAtrIX
Copy link
Member

@oSumAtrIX oSumAtrIX commented Nov 8, 2023

This PR shows a native dialogue after the installation finishes. It can handle all status flags. Business logic may move.

Todo

  • Handle errors such as:
    • Installing regularly while a mount installation is currently active (In this case, uninstall the mount installation)
    • Mount errors such as wrong base APK or no base APK present (In this case, show an appropriate dialog)

Review

Warning

On installation, the install button is not greyed out, and no progress is visible, so it appears as if nothing happened.

@oSumAtrIX oSumAtrIX linked an issue Nov 8, 2023 that may be closed by this pull request
4 tasks
@oSumAtrIX oSumAtrIX self-assigned this Nov 8, 2023
@TheAabedKhan TheAabedKhan added the ReVanced Manager Compose Regarding the Compose rewrite of ReVanced Manager label Nov 8, 2023
Copy link
Contributor

@KobeW50 KobeW50 left a comment

Choose a reason for hiding this comment

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

Suggestion for the "invalid" error description

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@oSumAtrIX oSumAtrIX marked this pull request as ready for review November 10, 2023 00:49
Copy link
Contributor

@KobeW50 KobeW50 left a comment

Choose a reason for hiding this comment

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

String changes in error dialog

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@KobeW50
Copy link
Contributor

KobeW50 commented Nov 14, 2023

Is this also for when you update the Manager?

@CnC-Robert
Copy link
Member

Is this also for when you update the Manager?

No it's only for installing patched apps

@Ushie
Copy link
Member

Ushie commented Nov 17, 2023

ReVanced Manager Updates will also use this, but this PR focuses on patched apps

@oSumAtrIX
Copy link
Member Author

@ Axelen123 Currently, the extra status message is not used, but I don't know which extra status messages exist so that I can implement them. For example, if an existing installation is present, but the patched app mismatches signatures, then the dialogue will say:

image

This flag is returned from installation, and there is no way to differentiate between an APK being invalid, for example, or a signature mismatch.

Furthermore, when uninstalling, the dialogue may also be shown. But once again, there is no way to differentiate between installations, uninstallations or the status message, so the dialogue may say that the installation has failed while the uninstallation failed.

@oSumAtrIX
Copy link
Member Author

msedge_XcwCJ412Tb.mp4

(The recording is cut off at the end, it shows the success dialogue when installation succeeded)

@Axelen123
Copy link
Member

@ Axelen123 Currently, the extra status message is not used, but I don't know which extra status messages exist so that I can implement them. For example, if an existing installation is present, but the patched app mismatches signatures, then the dialogue will say:

image

This flag is returned from installation, and there is no way to differentiate between an APK being invalid, for example, or a signature mismatch.

Not sure why Android says the app is invalid. Android should give you CONFLICT on signature mismatch. What is the content of the EXTRA_STATUS_MESSAGE here?

Furthermore, when uninstalling, the dialogue may also be shown. But once again, there is no way to differentiate between installations, uninstallations or the status message, so the dialogue may say that the installation has failed while the uninstallation failed.

Isn't it possible to make a separate dialog or enum for uninstalls?

@oSumAtrIX
Copy link
Member Author

Isn't it possible to make a separate dialog or enum for uninstalls?

Yes, but ideally we would have the existing Flags enum have variants of the install and uninstall strings.

@Ushie
Copy link
Member

Ushie commented Dec 10, 2023

Will root install be handled in this?

@oSumAtrIX
Copy link
Member Author

@Axelen123 I have updated the PR description with up to date information.

Copy link
Member

@Axelen123 Axelen123 left a comment

Choose a reason for hiding this comment

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

Code looks good to me. The PR can be merged after dealing with the final comment.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@oSumAtrIX
Copy link
Member Author

Not sure, if caused by this PR but I tried patching YT via the asset produced by a workflow run on this PR, and after clicking on install, it mentioned an installation conflict and blocked installation. Saving the APK file was also disabled. ReVanced Manager Flutter did install the patched app

@Axelen123
Copy link
Member

Looks like I missed that. This PR does introduce a check, but it does not account for patches that change the package name.

@oSumAtrIX
Copy link
Member Author

The issue stems from the fact that the dialog uses the input apk, but since we install the output apk, the output should be used

@Axelen123 Axelen123 merged commit d201bdc into compose-dev Aug 29, 2024
2 checks passed
@Axelen123 Axelen123 deleted the feat/installer-dialog branch August 29, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReVanced Manager Compose Regarding the Compose rewrite of ReVanced Manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: handle install failure
9 participants