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

Disable some advanced settings if migrating storage #13329

Merged
merged 1 commit into from
Feb 25, 2023

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Feb 23, 2023

Pull Request template

Purpose / Description

Migrating storage is a high-risk operation as it involves copying the user's media

Disable:

  • AnkiDroid Directory
    • Corruption: Media from the one collection is moved to the other
  • V3 Migration
    • High Risk: Disabling defensively

Related

Approach

Grey the preference and set a summary

Screenshot 2023-02-24 at 19 58 39

How Has This Been Tested?

Manually: API 33 emulator

Learning

Unused n this PR

  • If a preference is not disabled, the click handler won't stop the change/dialog event
  • If disabled, the click handler doesn't fire

But we can handle the dialog event separately to catch this

On switch/checkbox preferences, all we need to do is override setOnPreferenceChangeListener AFTER it's been set conditionally

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

@david-allison david-allison added Review High Priority Request for high priority review Scoped storage labels Feb 23, 2023
@david-allison david-allison changed the title Disable some settings if migrating storage Disable some advanced settings if migrating storage Feb 23, 2023
@github-actions
Copy link
Contributor

Message to maintainers, this PR contains strings changes.

  1. Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
  2. Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

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.

LGTM - strings are clear right now, easy merge

@mikehardy mikehardy added the Needs Second Approval Has one approval, one more approval to merge label Feb 23, 2023
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Feb 24, 2023
@david-allison david-allison force-pushed the storage-block-functionality branch from 9d23340 to ece0e14 Compare February 24, 2023 20:00
@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Feb 24, 2023
@david-allison
Copy link
Member Author

replaced implementation with a summary change. Thanks to @BrayanDSO for the idea

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Feb 24, 2023
@david-allison david-allison force-pushed the storage-block-functionality branch from ece0e14 to 1b8ddad Compare February 24, 2023 21:15
Migrating storage is a high-risk operation as it involves copying
the user's media

Disable:
* AnkiDroid Directory
  * Corruption: Media from the one collection is moved to the other
* V3 Migration
  * High Risk: Disabling defensively

Grey the preference and set the summary

Issue 5304
Issue 13094
@david-allison david-allison force-pushed the storage-block-functionality branch from 1b8ddad to b111546 Compare February 24, 2023 23:04
@BrayanDSO BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Feb 24, 2023
@david-allison
Copy link
Member Author

david-allison commented Feb 25, 2023

Is this one good to go (with the string PR)?

@mikehardy
Copy link
Member

There's a fair chunk of strings activity (https://crowdin.com/project/ankidroid/activity-stream) since last build.
It is only an optimization but it is a useful one IMHO, better to clear them out.
I'll run strings now then hopefully (with a concerted push from everyone) we can merge all the strings PRs in a batch for optimal optimalness

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.

clean!
I just checked crowdin activity and there has been more from 2 people in between me cleaning strings and right now, but their translations all check out, so the strings PR will be easy

@mikehardy mikehardy merged commit adbba18 into ankidroid:main Feb 25, 2023
@github-actions github-actions bot added this to the 2.16 release milestone Feb 25, 2023
@github-actions github-actions bot removed Review High Priority Request for high priority review Needs Second Approval Has one approval, one more approval to merge labels Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

5 participants