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.

Sign up for GitHub

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

Jump to bottom

feat(new reviewer): join "Hide 'answer buttons'" prefs #17824

Closed
wants to merge 1 commit into from
Closed

feat(new reviewer): join "Hide 'answer buttons'" prefs #17824

wants to merge 1 commit into from

Conversation

BrayanDSO
Copy link
Member

Purpose / Description

Hide 'easy' and 'again' and Hide answer buttons are being joined into a single ListPreference called Show answer buttons with the options:

  • All
  • Good and Again
  • None

Approach

Blocked by #17816

Remove the old preferences and add a new one

How Has This Been Tested?

join.webm

Checklist

Please, go through these checks before submitting the PR.

  • 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

@BrayanDSO BrayanDSO added the Blocked by dependency Currently blocked by some other dependent / related change label Jan 15, 2025
@github-actions GitHub Actions
Copy link
Contributor

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

@BrayanDSO BrayanDSO force-pushed the show-answer-buttons branch 3 times, most recently from 52acbb1 to 83762f3 Compare January 16, 2025 00:07
@david-allison
Copy link
Member

david-allison commented Jan 16, 2025 •
edited
Loading

I strongly want to see '2 button' mode to be sold as an 'addon' to users so:

  • We're not pressuring upstream to implement it as a feature before it's ready
    • 'Copy as Markdown' got proposed a few days ago due to our implementation, it's a useful feature for user support, but I wouldn't have wanted upstream to treat it as a priority.
  • We're making a commitment to addons, (again, likely before they're upstream compatible)

I think this PR will make the above harder to accomplish

@BrayanDSO
Copy link
Member Author

We're not pressuring upstream to implement it as a feature before it's ready

I don't see it as pressure from our side. AnkiDroid can put the answer buttons at the top of the screen and that doesn't make anybody to want to pressure Anki to implement it. What does that is the feature being useful, which is why there's already a lot of people asking for that in the forums, even before this was added into the new reviewer. Also, the feature is still the same, just condensed into another preference. If you want to avoid pressure, remove it.

What I want is to improve the user's experience. If they get a positive impact out of it and request it in Anki, that's a consequence of being a good thing. And if they don't want to hear their users and that kind of "pressure"... Well, it's their problem.

We're making a commitment to addons

I don't think that this interferes with moving the option to addons at all. Removing ˋGood and Againˋ from the options is the same of removing ˋHide 'Hard' and 'Easy' buttonsˋ. ˋShow answer buttons > All/Noneˋ still make sense as a setting, or it can be transformed into a SwitchPreference in just 2 minutes.

@BrayanDSO BrayanDSO force-pushed the show-answer-buttons branch from 83762f3 to 6f69e48 Compare January 16, 2025 10:28
@BrayanDSO BrayanDSO added Needs Review and removed Blocked by dependency Currently blocked by some other dependent / related change Has Conflicts labels Jan 16, 2025
@BrayanDSO
feat: join "Hide 'answer buttons'" prefs
6f69e48 
`Hide 'easy' and 'again'` and `Hide answer buttons` are being joined into a single ListPreference called `Show answer buttons` with the options:
- All
- Good and Again
- None
@BrayanDSO
Copy link
Member Author

'2 button' mode to be sold as an 'addon' to users

And I really don't know how do you want to sell something as an addon if addons don't exist yet. When they do, make it an addon, then warn them about the move. While they don't, it doesn't make sense to an user to see something labeled as that

@david-allison
Copy link
Member

I don't see it as pressure from our side. AnkiDroid can put the answer buttons at the top of the screen and that doesn't make anybody to want to pressure Anki to implement it. What does that is the feature being useful, which is why there's already a lot of people asking for that in the forums, even before this was added into the new reviewer. Also, the feature is still the same, just condensed into another preference. If you want to avoid pressure, remove it.

It's pressure from our side to change one of the core aspects of the ecosystem. I strongly want to avoid that until we have our house in order.

There's hundreds of pages of debate on the forums. This is going to cause lasting issues and eat up developer time however we skin it, and (as much as I want this feature), I don't want it to be fuel for the fire.

  • See https://redirect.github.com/ankitects/anki/pull/3719 for a recent example of AnkiDroid functionality being brought upstream. That was fairly non-controversial and took up time. Even under the best of circumstances, 'pass/fail' will take much longer, and I'd rather upstream development effort went to whatever they decide their roadmap is, and not use AnkiDroid to push a path for them. Is 'Copy as Markdown' useful? Of course! But I'd rather see FSRS become the default, unify the note editor, and spend a ton of time simplifying the deck options

A few of my comments on the merge of the feature, just to confirm that I'm committed to the addons approach.

  • We shouldn't diverge from Anki Desktop long term (besides mobile/Android-only features)
    • When we do diverge short-term this should be with a plan to move back to a consistent state
    • It should be obvious to users that these are divergences, and not supported to the same degree as compatible functionality

In this case, we should:

  • Agree to reimplement this as an add-on long-term (which can affect the Reviewer Chrome)
  • Move this to an "addons" menu in the settings to note that this isn't supported as core functionality, and that there may be changes

I'd block the general feature release on moving it to an addons menu, to shield ourselves/upstream from the impact of divergence (and string changes/instability). I'm happy to work on this.

And I really don't know how do you want to sell something as an addon if addons don't exist yet. When they do, make it an addon, then warn them about the move. While they don't, it doesn't make sense to an user to see something labeled as that

  • It's a commitment to having addons in the future (hopefully the version after the new reviewer is live).
    • And hopefully a few people will be excited by it. I am
  • It means we don't need to update the feature location
  • It's allows us to test the addons UI
  • It sets expectations that there will be breakages when we move it from an extension to AnkiDroid to a JS Addon
  • We're not pressuring upstream with incompatible functionality
    • Anki Desktop has it as an addon, we have it as an addon. No concerns

====

I don't think that this interferes with moving the option to addons at all. Removing ˋGood and Againˋ from the options is the same of removing ˋHide 'Hard' and 'Easy' buttonsˋ. ˋShow answer buttons > All/Noneˋ still make sense as a setting, or it can be transformed into a SwitchPreference in just 2 minutes.

Ok, I'm sold

david-allison
david-allison requested changes Jan 17, 2025
View reviewed changes
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.

I'm not immediately seeing that it'd be trivial to split this over two screens, BUT it's feasible

LGTM once the string is moved

AnkiDroid/src/main/res/values/10-preferences.xml
<string name="hide_hard_and_easy" maxLength="41">Hide ‘Hard’ and ‘Easy’ buttons</string>
<string name="show_answer_buttons" maxLength="41">Show answer buttons</string>
<string name="show_answer_buttons_all" maxLength="41" comment="Option to show all answer buttons">All</string>
<string name="show_answer_buttons_good_and_again" maxLength="41" comment="Option to show only the ‘Good’ and ‘Again’ answer buttons">‘Good’ and ‘Again’</string>
Copy link
Member

Choose a reason for hiding this comment

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

Time to kick off addons, his should be in 21-addons.xml [CrowdIn syncing infra optional]

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 17, 2025
@BrayanDSO
Copy link
Member Author

eat up developer time

All of the intermediate steps here are eating more IMO.

I still don't see it as pressure, nor why someone should feel pressured in an open source free app (although some people do).

That's a lot of churn and discussion for something that could be simple.

I'll leave that for anyone motivated enough for that.

@BrayanDSO BrayanDSO closed this Jan 17, 2025
@BrayanDSO BrayanDSO deleted the show-answer-buttons branch January 17, 2025 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-allison david-allison david-allison requested changes

Assignees
No one assigned
Labels
Has Conflicts Needs Author Reply Waiting for a reply from the original author Needs Review Strings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BrayanDSO @david-allison