Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

ManageSpaceActivity created.

Open Noxor11 opened this issue 2 years ago • 9 comments

Pull Request template

Purpose / Description

Fix the issue #12366 creating the activity with a small warning stating the only way to manage data is deleting it.

Fixes

Fix the issue #12366.

Approach

Created a Kotlin class to show the activity after declaring it in the Android Manifest.

How Has This Been Tested?

Compiled and ran on my cellphone, using Android Studio's run configuration to directly run the Activity.

Checklist

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

EDIT: Now Checked.

1

2

Noxor11 avatar Sep 10 '22 18:09 Noxor11

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

welcome[bot] avatar Sep 10 '22 18:09 welcome[bot]

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,

github-actions[bot] avatar Sep 10 '22 18:09 github-actions[bot]

Hi,

FYI usually here, we amend commit and force push. No big deal since it’s going to squashed I assume. But when we actually use atomic commit, that helps ensure each commit has a single change.

Seems fine to me. Waiting for tests to run. It failed earlier but seems unrelated

Arthur-Milchior avatar Sep 11 '22 21:09 Arthur-Milchior

I realize this was adding as string. However, we hopefully will never release with only this message, the string will be replaced by buttons. So no need to do all the work to send it to translators.

So I added a commit that hard code the string in English.

Arthur-Milchior avatar Sep 17 '22 20:09 Arthur-Milchior

This currently impedes someone to clear the app data manually, besides uninstalling. I wouldn't merge this until a provisory button to clear the app data is on the activity

BrayanDSO avatar Sep 17 '22 20:09 BrayanDSO

On the one hand, I'm totally fine with not being able to delete without uninstalling. Either the data are on ~/AnkiDroid and the button don't do anything Or the user is on alpha using scoped storage, and there should be virtually nobody that is not a dev' doing it. And I don't know of anyone

On the other hand, I guess that starting to add buttons would be nice, since that's what the UI should looks like and what the the next step should be. However, asking for one or a few entries in the menu may be easy. Ensuring they are linked to correctly implemented actions is going to be really hard for a first PR request.

@Noxor11 Can you please let us know how you fill about:

  • adding element(s) in this activity that are clickable
  • ensuring that the "Delete collection" ask for confirmation and delete the folder containing the collection
  • or otherwise some other action mentioned in #12366 that you believe you can implement

Arthur-Milchior avatar Sep 17 '22 20:09 Arthur-Milchior

@Noxor11 Can you please let us know how you fill about:

  • adding element(s) in this activity that are clickable
  • ensuring that the "Delete collection" ask for confirmation and delete the folder containing the collection
  • or otherwise some other action mentioned in #12366 that you believe you can implement

Honestly I don't know the appropiate way of doing any of this in the app.

Noxor11 avatar Sep 17 '22 21:09 Noxor11

Not even the first step? Creating a list of actions in a menu is standard Android action. Opening a confirmation activity that can be confirmed or cancelled too. It's okay if it do nothing in a first commit, we can always call the function that deletes the collection later. You can also look at our code to see how some actions are done. Or come on discord and discuss with us to ask where to ask questions

Arthur-Milchior avatar Sep 18 '22 09:09 Arthur-Milchior

This does not seem to be actually pending merge, re-labeling

mikehardy avatar Sep 18 '22 12:09 mikehardy

Not even the first step? Creating a list of actions in a menu is standard Android action. Opening a confirmation activity that can be confirmed or cancelled too. It's okay if it do nothing in a first commit, we can always call the function that deletes the collection later. You can also look at our code to see how some actions are done. Or come on discord and discuss with us to ask where to ask questions

I managed to do it. The activity, along with the implemented "delete collection" button. If anything needs a change please let me know.

Noxor11 avatar Sep 28 '22 22:09 Noxor11

So I just pushed a new version. I hope back-end code is better. However, I admit I've been blocked by Front-End. I wished to used snackbar, as @BrayanDSO wanted, but it requires to have "An [Activity] that has a [CoordinatorLayout] with id root_layout.". And there is no coordinatorLayout here. Also, even trying to use a toast fail to show anything. (Even when directly called in initSubscreen, so it should not be a question of scope here). @Noxor11 , when you tried your function, where you able to see the toast?

Same problem with the progress bar, which requires a Fragment activity.

Arthur-Milchior avatar Oct 02 '22 19:10 Arthur-Milchior

I've just added number of back-up. However, #12562 also applies to it. We don't have something to directly delete extra back-ups. So still need to be done

Arthur-Milchior avatar Oct 03 '22 00:10 Arthur-Milchior

@Noxor11 , when you tried your function, where you able to see the toast?

I was able to see it after some seconds. The screen was laggy while deleting the files, but the toast did show up.

Sorry for the late response, my laptop's screen broke last monday, and I hadn't been able to check on this.

Noxor11 avatar Oct 03 '22 20:10 Noxor11

@oakkitten Thanks a lot. Applied or answered to all. In case it was not clear, we rarely ask specific people for review, unless we believe we actually must uses this very person feedback. Your review is very welcomed, and it's welcome anywhere. As I tried to explain on wiki and sometime on discord, anyone can review. One person with the reviewer status must give a review, but we want two reviews, the second review can be anyone. And that's what we use to decide who to offer the reviewer status to

Arthur-Milchior avatar Oct 09 '22 13:10 Arthur-Milchior

I've been tinkering with this, and I have a small question about finding unused media. The method that calculates unused media is attempting to normalize fields of cards that refer to media files, and the names of media files themselves. What is the problem that this solves? I am also not entirely sure if this solves the problem in a good way. If both normalized and non-normalized files exist, the non-normalized is silently deleted. Also, do we really at this point expect non-normalized card fields? In order to properly refactor this, or make a more suitable alternative, I want to have a better understanding of this. Any clues? @dae

oakkitten avatar Oct 27 '22 13:10 oakkitten

If the filenames on disk and in fields are not normalized, it can lead to copies being created with a different encoding when syncing across platforms due to the normalization macOS performs, or images failing to appear because the field encoding does not match the disk encoding. Anki ensures the correct encoding is used when writing files into the media folder, so "check media" should only perform changes when users manually write files into the media folder. And no, you can't rely on fields being in the correct encoding, since users can disable automatic normalization.

I would not waste time on refactoring the linked code, since the backend already provides its own implementation.

dae avatar Oct 28 '22 10:10 dae

I see. I made manage space activity start calculating unused media files when it is shown, so I'd prefer that opening it doesn't result in any changes in files or notes. Also, the legacy check() method seems to be performing normalization unconditionally. So I guess I'll create another method for calculating unused media files only, that returns a marker if media check is required. As I understand, the backend doesn't support this at the time being.

oakkitten avatar Oct 28 '22 15:10 oakkitten

I rebased this to main (Media.kt had a trivial conflict I fixed) and pushed it out (--force-with-lease) so CI would run and it was up to date - just trying to push it forward a bit

mikehardy avatar Oct 30 '22 19:10 mikehardy

I'll make another PR that will supersede this. (Because I can't add commits here.) Need a little time to polish it

oakkitten avatar Oct 30 '22 19:10 oakkitten

This was the foundation of the PR for manage space activity that just merged, thank you!

mikehardy avatar Dec 03 '22 13:12 mikehardy