-
-
Notifications
You must be signed in to change notification settings - Fork 714
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
fix: Update dialog shows dev version & loading gets stuck in certain circumstances #1792
Conversation
I think this PR needs a review by @TheAabedKhan. Would you review please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can increase the number of releases the API returns. And then optimize the code by:
- Filtering during initialization
- Linear iteration
- Early termination
- Reduced loop iterations
Can you test whether the suggested code fixes the issues you mentioned?
lib/services/github_api.dart
Outdated
final response = await _dio.get( | ||
'/repos/$repoName/releases', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final response = await _dio.get( | |
'/repos/$repoName/releases', | |
); | |
final response = await _dio.get( | |
'/repos/$repoName/releases?per_page=100', | |
); | |
final List<dynamic> releases = | |
response.data.where((release) => !release['prerelease']).toList(); | |
int updates = 0; | |
final String currentVersion = | |
await _managerAPI.getCurrentManagerVersion(); | |
while (releases[updates]['tag_name'] != currentVersion) { | |
updates++; | |
if (updates == releases.length) { | |
break; | |
} | |
} | |
for (int i = 1; i < updates; i++) { | |
releases[0].update( | |
'body', | |
(value) => | |
value + | |
'\n' + | |
'# ' + | |
releases[i]['tag_name'] + | |
'\n' + | |
releases[i]['body'], | |
); | |
} | |
return releases[0]; |
Thank you. However, ?per_page=100 caused bad performance when the changelogs are long. (even on release build, Snapdragon 845 phone) Also, I want to a question.
I think this code will also work: int updates = 0;
while (releases[updates]['tag_name'] != currentVersion) {
updates++;
if (updates == releases.length) {
break;
}
releases[0].update(
'body',
(value) =>
value +
'\n' +
'# ' +
releases[updates]['tag_name'] +
'\n' +
releases[updates]['body'],
);
} Is it really more efficient to first count the number with a while loop and then do a for loop rather than processing it in one iteration? |
Couldn't feel any difference on my device. However, you can try reducing the number until you are satisfied with the performance or remove the parameter if you like.
You're correct that in terms of pure iteration, it would be more efficient to process everything in one iteration rather than counting first and then iterating again. final StringBuffer releaseNotes = StringBuffer();
while (releases[updates]['tag_name'] != currentVersion) {
updates++;
if (updates == releases.length) {
break;
}
releaseNotes.writeln('# ${releases[updates]['tag_name']}');
releaseNotes.writeln(releases[updates]['body']);
}
releases[0].update(
'body',
(value) => value + '\n' + releaseNotes.toString(),
);
return releases[0]; |
It's more readable than two loops! Thank you for the advice. I just came up with an idea. This version string in Then, the method |
fix: Remove duplicated version line in changelogs feat: Allow update manager when failed to connect to github api
Changesupdate_confirmation_sheet.dart:
home_viewmodel.dart:
github_api.dart:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tag_name
should be included in the changelog as we did not use to include it within the body
prior to v1.19.0
. If we happen to not include the tag_name
within the body of changelog in the future, then users can't know which changelog belongs to which version.
tag_name within body |
no tag_name within body |
---|---|
Or
Whatever @oSumAtrIX says
Signed-off-by: validcube <pun.butrach@gmail.com>
Reviewed-by: Pun Butrach <pun.butrach@gmail.com> Script Execution UTC Time: null
Script Execution UTC Time: null Signed-off-by: validcube <pun.butrach@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥞 Approved, thank!
# [1.21.0-dev.4](v1.21.0-dev.3...v1.21.0-dev.4) (2024-06-24) ### Bug Fixes * Cache external API calls ([#1911](#1911)) ([2c3e2e6](2c3e2e6)) * Follow language update immediately ([#1944](#1944)) ([c13827e](c13827e)) * SecurityException when patching application ([#1856](#1856)) ([e0a6de2](e0a6de2)) * Update dialog shows dev version & loading gets stuck in certain circumstances ([#1792](#1792)) ([fc52560](fc52560)) ### Features * Add ability to set `null` in patch options ([#1947](#1947)) ([5c68d51](5c68d51))
# [1.21.0](v1.20.1...v1.21.0) (2024-07-29) ### Bug Fixes * Add missing import to patch options field ([d60f9aa](d60f9aa)) * Adjust scroll from clipping children form fields in `AlertDialog` from `showSourcesDialog` ([#1782](#1782)) ([bbeb836](bbeb836)) * Cache external API calls ([#1911](#1911)) ([2c3e2e6](2c3e2e6)) * Change problematic translation string ([6b03f3a](6b03f3a)) * Correct architecture to armeabi-v7a ([63c6412](63c6412)) * Download latest integrations non-pre-release ([4a72267](4a72267)) * Follow language update immediately ([#1944](#1944)) ([c13827e](c13827e)) * Follow system theme immediately ([#1942](#1942)) ([694f2a9](694f2a9)) * Handle selecting files and folders for patch options correctly ([#1941](#1941)) ([b26760b](b26760b)) * Increase dashboard RefreshIndicator edge offset ([#1859](#1859)) ([232b702](232b702)) * Patching Screen draw-behind Navigation Bar ([#1945](#1945)) ([f1b25d0](f1b25d0)) * Restore consistency with the app ([ea9654e](ea9654e)) * SecurityException when patching application ([#1856](#1856)) ([e0a6de2](e0a6de2)) * Select previously applied patches when loading patch selection ([#1865](#1865)) ([7ef8f04](7ef8f04)) * Unable to install application regardless of preference ([c7627ce](c7627ce)) * Unsupported patch toast says "patchItem.unsupportedPatchVersion" ([#2011](#2011)) ([3209c0e](3209c0e)) * Update dialog shows dev version & loading gets stuck in certain circumstances ([#1792](#1792)) ([fc52560](fc52560)) ### Features * Add ability to set `null` in patch options ([#1947](#1947)) ([5c68d51](5c68d51)) * Include primary architecture in external search ([#2068](#2068)) ([23690a9](23690a9)) * open browser when clicking on changelog link ([bc300d8](bc300d8)) * Save last patched app ([#1414](#1414)) ([7720408](7720408)) * Support patching on ARMv7a ([a766352](a766352))
This PR fix these
twothree issues related to the Manager update dialog.Issue 1
Dev version is shown in the "Show update" or "Show changelog" dialog when the current latest version is dev.
completes #1774
Cause
add49e1 only checks the past releases, so the latest dev version is still shown
Solution
Make
GitHubAPI.getLatestManagerVersion()
find the latest non-dev release and return it.Issue 2
If the Manager version is too old (more than 30 releases), the update dialog gets stuck loading.
Cause
The api (https://api.github.com/repos/ReVanced/revanced-manager/releases) returns only recent 30 releases.
However, the current implementation doesn't consider it.
If the api response doesn't include the current Manager version, a while statement in
GitHubAPI.getLatestManagerVersion()
( that searches current version) accesses out of range index and causes error.Solution
Check the list range
As of today, this issue affects all versions prior to 1.18.0.
Discord supporters should know this issue 😕Issue 3 (Added 2024/03/31)
In changelogs, version name line is duplicated
Cause
d414a91 changed changelog format to contain version name
Solution
Not to append version name when output changelogs