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: patch history #1414

Open
wants to merge 32 commits into
base: dev
Choose a base branch
from
Open

feat: patch history #1414

wants to merge 32 commits into from

Conversation

BenjaminHalko
Copy link
Sponsor Member

closes: #643

Adds patch history: It will keep the last 3 APKs of the patched packages. The user can then install or export it.

The UI could use a bit of work though. Right now if you click on the app in the history menu, it will let you install or export. If you open it in the installed app list you get the option to open / uninstall the app like before.

The reason for the 2 separate menus is for the case where someone patches the app, then closes manager without installing. It would be nice to be able to go back and install it. Thus it needs a separate entry from the version that is currently installed on the system.

The history is limited to the 3 most recent packages because it has to keep the patched apk for every entry which uses up space.

Example where Twitch was compiled but not installed, and where Repair Toolkit was compiled and installed:

Screen_Recording_20231021_131516_ReVanced.Manager.Debug.mp4

@Ushie
Copy link
Member

Ushie commented Oct 21, 2023

You may be interested in adding chips to switch between those cards, look at older versions of revanced manager before the removal of the "update" WIP card

@validcube
Copy link
Member

Now we need "delete stored APK" in settings!!!!

@oSumAtrIX
Copy link
Member

This feature requires enhanced transparency. Users should be able to create a backup of the patched app once the patching process is complete. This could lead to significant storage consumption for each patched app by default. A checkbox labelled "Save a copy" should be included within the installation dialogue, accompanied by the subtitle: "A copy of the patched app will be saved, which can be installed or exported in the future. You can manage these copies in the 'Patched apps' section located in the main menu of ReVanced Manager." As suggested by @validcube, it is advisable to provide the option to delete saved APKs, preferably within the same interface where users can install, export, or manage the patched app. A submenu displaying the list of saved patched apps, their patching date, and metadata, including applied patches, should be available for that.

If the user checks the checkmark to save the patched app, but a saved patched app of the same version is already present, give the user the option to overwrite any existing saved app.

The installation button in the patched app view has to show the same popup as the one after patching, as you may want to mount the app.

@BenjaminHalko
Copy link
Sponsor Member Author

BenjaminHalko commented Oct 22, 2023

This could lead to significant storage consumption for each patched app by default

I propose we only save the last 3 packages, plus there could be a button in the app history to delete it. This button could be beside the install and export button, on the same row.

A checkbox labelled "Save a copy" should be included within the installation dialogue, accompanied by the subtitle: "A copy of the patched app will be saved, which can be installed or exported in the future. You can manage these copies in the 'Patched apps' section located in the main menu of ReVanced Manager."

How about just a toggle in the settings menu. I feel like the patch history should be used for the event where a user forgets to either export or install an app. There have been a few times where I have forgot to export my apk and then needed to repatch it. But never an event where I wanted the manager to save it for me. If I needed it saved, I just exported the apk. If I had to click a checkbox every time I wanted the app to save the apk, that would defeat the point of having the app save a backup. If I remembered to click a checkbox then I will have remembered to export the apk.

The installation button in the patched app view has to show the same popup as the one after patching, as you may want to mount the app.

Already should do that, though I don't have root so I can't test that out.

@oSumAtrIX
Copy link
Member

I propose we only save the last 3 packages

How about using that as a maximum, but still inform the user about being able to overwrite the last saved patched app via a checkmark if a patched app is already saved? That way the user can replace their last saved patched app every time they successfully patch without having to manually clean up.

If I remembered to click a checkbox then I will have remembered to export the apk.

In this case, how about checking it by default. Having to visit the settings to toggle it, would be difficult right from the patch result screen.

@BenjaminHalko
Copy link
Sponsor Member Author

The "3 packages" limit would be 3 different packages. If you patch youtube it would replace youtube in the history automatically. If the youtube package was not in the history it would bump the oldest package off the history.

Although, thinking about it now, since the patch history is for "if the user forgets to preform an action once finished patching" it might be simpler to save ONLY the last patched apk. There isn't really a need to save more than that.

@BenjaminHalko BenjaminHalko marked this pull request as draft October 25, 2023 01:49
@oSumAtrIX
Copy link
Member

There isn't really a need to save more than that.

Yeah, 1 backup per app should be enough, saving the last patched app only (give the user the choice not to), should be good too.

# Conflicts:
#	lib/models/patch.dart
#	lib/services/patcher_api.dart
#	pubspec.yaml
@PalmDevs
Copy link
Member

@BenjaminHalko Can you upload a recording of what the feature looks like now? Or is the video in the PR description already up-to-date?

@BenjaminHalko
Copy link
Sponsor Member Author

Not shown in the video is the fact that only the last patched app gets saved. If there is already an app in the slot, it will get removed once a new app is done patching.

Screen_Recording_20231214_085636_ReVanced.Manager.Debug.mp4

assets/i18n/en_US.json Outdated Show resolved Hide resolved
assets/i18n/en_US.json Outdated Show resolved Hide resolved
Copy link
Member

@oSumAtrIX oSumAtrIX left a comment

Choose a reason for hiding this comment

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

I am not sure about this suggestion, but what "Last patched app" means can be confusing to the user. Is it the now-installed app? If you press delete, will it delete my currently installed patched app? Maybe add text to the app view that explains that it is a backup of the last patched application, which can be used to reinstall the patched app without having to patch again.

BenjaminHalko and others added 2 commits January 13, 2024 19:35
Co-authored-by: oSumAtrIX <johan.melkonyan1@web.de>
Co-authored-by: oSumAtrIX <johan.melkonyan1@web.de>
@BenjaminHalko
Copy link
Sponsor Member Author

Added this description:
Screenshot_20240305-084404_ReVanced Manager Debug

@validcube
Copy link
Member

Is there anything that's holding up this PR?

@BenjaminHalko
Copy link
Sponsor Member Author

I guess just approval.

@oSumAtrIX
Copy link
Member

#1414 (review)

Has this been resolved?

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.

None yet

8 participants