fix: Update dialog shows dev version & loading gets stuck in certain circumstances

[kitadai31]

This PR fix these two three 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

[kitadai31]

I think this PR needs a review by @TheAabedKhan. Would you review please?

[TheAabedKhan]

We can increase the number of releases the API returns. And then optimize the code by:

1. Filtering during initialization
2. Linear iteration
3. Early termination
4. Reduced loop iterations

Can you test whether the suggested code fixes the issues you mentioned?

[TheAabedKhan]

Suggested change

	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];

[kitadai31]

Thank you.
I checked that it fixes the issues.

However, ?per_page=100 caused bad performance when the changelogs are long. (even on release build, Snapdragon 845 phone)
It took 1.5 seconds to open the changelog and scroll is very laggy.
Therefore, I think it would be reasonable to limit it to default 30 releases.

Also, I want to a question.
I'm interested in this.

2. Linear iteration
3. Early termination
4. Reduced loop iterations

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?
(Sorry for the off-topic general computer science question.)

[TheAabedKhan]

It took 1.5 seconds to open the changelog and scroll is very laggy.

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.

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?

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.
Also, while we are on it, how about accumulating release notes separately before updating the body of the first release? This will reduce the number of update operations on the body of the first release.

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];

[kitadai31]

It's more readable than two loops! Thank you for the advice.

I just came up with an idea.

This version string in UpdateConfirmationSheet can be obtained from a field HomeViewModel.latestManagerVersion instead of GitHubAPI.getLatestManagerReleases().
This value is more accurate than get from https://api.github.com/repos/ReVanced/revanced-manager/releases?per_page=XXX, because the possibility exists that all releases from this API are pre-release.
(lack of my test cases before making a PR)

Then, the method Future<Map<String, dynamic>?> getLatestManagerRelease(String repoName) don't have to provide the latest version string, so it can be more simple and less-responsibility method like Future<String?> getManagerChangelogs()
I will do this tomorrow.

[kitadai31]

Changes

update_confirmation_sheet.dart:

• Change version string reference from snapshot.data!['tag_name'] to model.latestPatchesVersion
• Use FutureBuilder only for the changelog part
  • This allows users to update manager even when failed to connect GitHub API

home_viewmodel.dart:

• Make String? _latestManagerVersion and String? _latestPatchesVersion instance variables public in order to be used in update_confirmation_sheet
• Remove initializing with empty strings because I want to guarantee that the variable holds the latest version string
• Change methods which used in update_confirmation_sheet

github_api.dart:

• Future<Map<String, dynamic>?> getLatestManagerRelease(String repoName) now returns the changelogs only and renamed to Future<String?> getManagerChangelogs()
• Set to ?per_page=50
• Don't append the version name header while generating changelogs (Refer to the added issue 3)

[TheAabedKhan]

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

[validcube]

🥞 Approved, thank!