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 debug info to patching screen #1176

Closed
4 tasks done
taku-nm opened this issue Aug 28, 2023 · 14 comments · Fixed by #1299
Closed
4 tasks done

feat: add debug info to patching screen #1176

taku-nm opened this issue Aug 28, 2023 · 14 comments · Fixed by #1299
Assignees
Labels
Feature request Requesting a new feature that's not implemented yet

Comments

@taku-nm
Copy link
Contributor

taku-nm commented Aug 28, 2023

Type

Functionality

Issue

An astounding amount of users simply posts a screenshot of the patching screen saying "Aborted". When they're feeling gracious, they might add some info like "latest version" themselves, but often the user is unaware that there is a newer version available.

So the core issue is manager only showing the errors the patcher throws, without adding any debug info.
Sometimes its helpful, sometimes it isn't.

Feature

Display additional debug info.

this should include:

  • Manager Version
  • Package name of app selected
  • Version of app selected
  • Number of patches selected
  • Default patches?
  • APK loaded from storage or app list?
  • Split APK for not?
  • Android Version

in order to minimize the screenspace required for that info I'd suggest packing it all into a short string:

1.9.3:com.google.android.youtube:18.23.35:52:Y:S:NS:11

This way there is some flexibility as to where to include that information.
It's important that the debug information is on display regardless of where the user might've scrolled to in the log.

Poorly photoshopped example:
aborted
(android version not pictured as I thought about this after the fact)

Additionally, one might drop the information on "Number of patches selected" and "Default patches" in favour of an integer that is computed based on the selected patches. Similar to discords bot permissions integer.

Motivation

Efficiency and speed.
Having this debug information would immediately outline the source of the error in the overwhelming majority of cases that land in the support channel.
This reduces additional questioning and guidance, especially when the user for example doesn't know what versions he's using.

It gives all supporters a fairly detailed insight into what the user is doing (wrong).

Additional context

One might also consider a pop-up that notifies the user to either send a screenshot or to click [this] button to export the log to ask for further assistance. In the later case one should attach the debug string at the beginning of the exported logfile.

Acknowledgements

  • I have searched the existing issues and this is a new and no duplicate or related to another open issue.
  • I have written a short but informative title.
  • I filled out all of the requested information in this issue properly.
  • The issue is related solely to the ReVanced Manager
@taku-nm taku-nm added the Feature request Requesting a new feature that's not implemented yet label Aug 28, 2023
@validcube
Copy link
Member

validcube commented Sep 7, 2023

This feature request looks interesting, I think the information could be outputed to the installer log instead of the app bar, would that looks fine enough?

@validcube validcube self-assigned this Sep 7, 2023
@Ushie
Copy link
Member

Ushie commented Sep 7, 2023

Yes, in that case the question comes towards where exactly? At the end of the patching process? At the top? if it's at the top, it wont be captured in screenshots of the log because of logs that come after it, if its at the end it might not be visible because error logs are long and the relevant part would be in the middle

@taku-nm
Copy link
Contributor Author

taku-nm commented Sep 7, 2023

installer log instead of the app bar

While that would probably work in most cases, the intention of putting that into that specific bar, is to ensure that it is visible in every screenshot. The installer log can be scrolled up and down, bypassing that matter.
+What ushie just said

@Ushie
Copy link
Member

Ushie commented Sep 7, 2023

It wouldn't fit in app bar though

@taku-nm
Copy link
Contributor Author

taku-nm commented Sep 7, 2023

It wouldn't fit in app bar though

Why not? my poorly photoshopped example would be enough, and it really does not need to be big

@validcube
Copy link
Member

validcube commented Sep 7, 2023

It wouldn't fit in app bar though

Why not? my poorly photoshopped example would be enough, and it really does not need to be big

I think this would violate the Material 3 guidelines for top app bar. I think @PalmDevs would have better suggestion for what to do.

@taku-nm
Copy link
Contributor Author

taku-nm commented Sep 7, 2023

Could one alternatively consider having the code displayed below the error log, in a separate box?
But lets see what Palm comes up with

@PalmDevs
Copy link
Member

PalmDevs commented Sep 7, 2023

I think instead of trying to add information into an already cramped screen, we should just add relevant information into the logs instead and tell the user to share them when they get support.

@taku-nm
Copy link
Contributor Author

taku-nm commented Sep 7, 2023

tell the user to share them when they get support

That only partially solves the issue this is intended to solve, as the point of it is to reduce the back and worth in the first place. The debug data must be included in the most commonly and intuitively shared medium, which is the screenshot of the Aborted screen.

If that requires, violating some specific design spec, I would like to point to a great video by software UI designer tantacrul in which he outlines that breaking a design might in some cases be a necessity. Similarly to WP:IAR

@PalmDevs
Copy link
Member

PalmDevs commented Sep 8, 2023

That only partially solves the issue this is intended to solve, as the point of it is to reduce the back and worth in the first place. The debug data must be included in the most commonly and intuitively shared medium, which is the screenshot of the Aborted screen.

If that requires, violating some specific design spec, I would like to point to a great video by software UI designer tantacrul in which he outlines that breaking a design might in some cases be a necessity. Similarly to WP:IAR

This solution also only fixes the branches of the issue. The whole issue stems from the fact the Flutter Manager overall is really confusing, which can (and have already) lead to user errors. This would be fixed in the Compose rewrite.

For now, if it's required, put the information in the bottom of the logs.

@BenjaminHalko BenjaminHalko self-assigned this Sep 21, 2023
@taku-nm
Copy link
Contributor Author

taku-nm commented Sep 21, 2023

upon further discussion here is a good solution:

Instead of printing the debug string in the log there should be a pop-up that opens as soon as an exception is being thrown. The pop-up should include the debug string, the main error, a button to copy the string + logs to clipboard and a hint as to what to do with it.
"Share this message with the support channel" for example.

@oSumAtrIX
Copy link
Member

. The pop-up should include the debug string, the main error

In most cases, this exceeds the max height of a small screen and the more info the popup has, the more the user will be inclined to screenshot it and call it a day instead of pressing the copy button that includes all necessary information.

@BenjaminHalko BenjaminHalko linked a pull request Sep 22, 2023 that will close this issue
@Ushie Ushie closed this as completed Oct 13, 2023
@taku-nm
Copy link
Contributor Author

taku-nm commented Nov 25, 2023

After a while of seeing this feature deployed, it has become apparent that the discord message limit has become the main barrier for most users.
The message limit is easily reached by the fact that Apps like YouTube have a lot of patches.
I'd suggest trimming anything that has been included by default to remove overall clutter and making it easier to share the logs.
Any patch that has options should be included in the logs as well.

Conceivably one might exclude all statements following the regex .* succeeded but this might go against attempts of removing hard-coded variables.

Checklist

  • Trim patches included by default
  • Include note wherever something has been trimmed
  • Include any patch with options

@oSumAtrIX
Copy link
Member

Please open a new issue, this feature has nothing to do with your new one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Requesting a new feature that's not implemented yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants