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

Add fsrs version to debug info #17304

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

lukstbit
Copy link
Member

@lukstbit lukstbit commented Oct 24, 2024

Purpose / Description

Adds the fsrs version to the debug info. In a separate commit I also added the version to the about settings screen so it can be removed if not wanted(or maybe combine the backend and fsrs info):
s1

Fixes

Towards #17236

How Has This Been Tested?

Looked at the debug info, looked at the settings screen

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@lukstbit lukstbit added the Review High Priority Request for high priority review label Oct 24, 2024
@lukstbit lukstbit added this to the 2.19 release milestone Oct 24, 2024
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

The FSRS version is technically correct (in that it's the library version) but not what we should display to the user.

A user would expect this version to be "FSRS 4.5", and not displaying this will lead to confusion.

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Requested change:

I feel hardcoding a mapping from 'internal' to 'user-facing' version is the most effective way to quickly solve this

  • Hardcode a mapping from 0.6.4 to 4.5
  • Add a unit test which will break if the BuildConfig changes from 0.6.4
  • In a future version, we expect to move this to the backend

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

  • capitalization nits

AnkiDroid/src/main/res/layout/about_layout.xml Outdated Show resolved Hide resolved
@lukstbit lukstbit force-pushed the chore_addFsrsVersionInDebugInfo branch from 29ac8da to 0d716ec Compare October 24, 2024 11:10
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Cheers!

Once this is merged, we should add an issue to expose this version in the backend

That will remove the need to perform additional code changes when bumping the backend version

@david-allison david-allison added the Needs Second Approval Has one approval, one more approval to merge label Oct 24, 2024
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

With apologies for the back & forth. After a discussion with Voczi, Jarrett Ye & Expertium, there's consensus for displaying the library version.

Voczi
user facing version covers a larger amount of changes in fsrs at a time, library version covers smaller amount of changes at a time. thats how i understood it

Expertium
[FSRS library version], since it's more informative
There are minor changes within major FSRS versions

Jarrett
I prefer lib version
https://github.com/open-spaced-repetition/fsrs-rs/releases
You can see all changes introduced by any version.

https://discord.com/channels/368267295601983490/701922522836369498/1298985695645270097

I personally disagree here (I believe displaying the user-facing version is better), but it's a quick change to make to improve things if necessary

The FSRS devs will feel the support burden from this change more than AnkiDroid, and it's only fair to them to help them with user support as much as possible

@lukstbit lukstbit force-pushed the chore_addFsrsVersionInDebugInfo branch from 0d716ec to e9efd84 Compare October 24, 2024 15:17
@lukstbit
Copy link
Member Author

lukstbit commented Oct 24, 2024

I've reverted the changes to the initial state, this includes showing only the library version in the debug info.

I personally disagree here (I believe displaying the user-facing version is better)

I also think the user facing version is better. Looking through discussions on ankiweb's forums the users refer to FSRS in terms of 4.5, 5 and not library version which is mostly a dev thing. Even if there are issues with FSRS, we ask for debug info which would include the precise library version anyway.

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I'm also going with it as the stated preference of the devs following my general "implementor's choice" rule, which is even stronger if it's "people doing support's choice". We can add a mapping later if we really need it

@mikehardy mikehardy added this pull request to the merge queue Oct 25, 2024
@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Review High Priority Request for high priority review Needs Second Approval Has one approval, one more approval to merge labels Oct 25, 2024
Merged via the queue into ankidroid:main with commit f72114b Oct 25, 2024
9 checks passed
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Oct 25, 2024
@lukstbit lukstbit deleted the chore_addFsrsVersionInDebugInfo branch October 25, 2024 05:37
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.

3 participants