feat: import CSV UI by david-allison · Pull Request #9437 · ankidroid/Anki-Android

david-allison · 2021-08-22T20:25:24Z

Pull Request template

Purpose / Description

We want to allow the user to import CSVs

Fixes

Fixes #6772

Approach

We already have the python code converted. We use a fragment-based approach for the UI

Host Fragment
- Load File
- Options
  - Delimiter Dialog
  - Mapping (CSV -> Field) Fragment
- Load/Display Log

⚠️ We use Kotlin coroutines for async, mostly as I'd like to try them out as a new async mechanism, I'm very open to changing this if it's not ideal

Parcelize has been added to reduce the code needed to create bundles

How Has This Been Tested?

Some unit tests, tested on my phone

⚠️ Large feature, likely needs more integration tests on the progress/end state

Checklist

You have not changed whitespace unnecessarily (it makes diffs hard to read)
You have a descriptive commit message with a short title (first line, max 50 chars).
Your code follows the style of the project (e.g. never omit braces in if statements)
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
- ❌See below

Screenshots

Accessibility failures

david-allison · 2021-08-22T21:27:28Z

Contains commits from #9423, best to get that in first

mikehardy · 2021-08-22T21:46:30Z

#9423 is in!

david-allison · 2021-08-22T22:35:11Z

I expect this'll pass CI now. Will remove the draft if it does

Review questions:

should we backup before import? Will this potentially make restoration less likely (if a user messes up, I feel they're likely to try again a few times)
How do we we want to handle the accessibility issues?

david-allison · 2021-08-23T00:13:23Z

+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
+        super.onViewCreated(view, savedInstanceState)
+
+        downloadPercentage = view.findViewById(R.id.import_progress_text)


How would other reviewers feel about using a DI framework to reduce code here?

I had to Google it, and I see no issue here.
I mean, I'm never totally fan of adding state, but I'm not fan of huge method either

david-allison · 2021-08-23T00:13:57Z

+            AnkiDroidApp.sendExceptionReport(ex, "import failed")
+            onException(ex)
+        }
+        GlobalScope.launch(Dispatchers.Main + exceptionHandler) {


@Arthur-Milchior pinging for your opinion on me testing a new async mechanism, and thoughts on whether this API is too far from what we're currently using

I don't know the first thing about GlobalScope.launch, so hard to tell.
My main concern with switching with a new async mechanism is that all async code currently assume that a single background thread runs at one point in time (appart from synchronization thread). In particular, it means that there is never two activities touching the database at the same time.
I suppose that "performImport" will write to the database.
I can't imagine any actual trouble, but even if it works as expected, still not a great idea to break this invariant until we know how we want async to evolve.

Plus, I'd assume it make testing it more complex as you can't use the same tooling to move this method to frontend

Agreed in general that testing a new async exec mechanism as a sort of one-off mixed in another PR doesn't seem like the right spot.

david-allison · 2021-08-23T00:15:16Z

+        const val RESULT_BUNDLE_OPTIONS = "importOptions"
+
+        /**
+         *


missing comment

usage: `collection.models[noteTypeId]!!`

Handles a user selecting a file For use in the CSV Import (issue 6772)

Handles obtaining a single character (and allows tab input) For use in the CSV Import (issue 6772)

Handles the UI of mapping from CSV to Note fields/tags For use in the CSV Import (issue 6772)

Also made NoteImporter/TextImporter mutable to handle this as is in the Python Handles the UI of all options when mapping from CSV to Note fields/tags For use in the CSV Import (issue 6772)

For CSV import: Issue 6772

Performs import, shows progress and results For use in the CSV Import (issue 6772)

Ties all the fragments together and responsible for workflow Related: 6772

Adds activity, linked to host fragment and implemented as menu in DeckPicker Fixes 6772

mindinsomnia · 2021-08-30T02:54:06Z

UI Mockup for CSV Import Workflow:

Rationale:

Menu -> Import Cards: Clearly labelled now so users should be able to tell easily from that menu item that if they desire to import some cards, to follow that pathway.

Import Cards screen: Clearly explains the process they are about to begin, both informs the user what type of files are supported for importing, and allows the user to choose the format to import and begin a process. Followed by a file selection screen which should be self explanatory at that point.

Select Deck: Button to popup what I suspect would be some kind of chooser dialog for choosing a deck, displaying selected deck gives the user a chance to preview that choice before they continue to avoid mistakes.

CSV Data: Preview the data to ensure it's being formatted nicely before continuing, chance to choose one of several common CSV delimiters if the current automatically selected delimiter is resulting in incorrect formatting. Preview does not need to list all data, even the first 100 or so rows would be sufficient.

Import Options: All the other decisions and choices out of the way, the user can focus on how the data is mapped, and confirm the operation to begin importing.

Summary Page: Neatly displays results of import process.

david-allison · 2021-08-31T02:03:36Z

Soft-blocked on #9466

mikehardy · 2021-08-31T18:22:56Z

mostly just noting soft-block PR was merged, and I took a moment to briefly scan comments and perform Seagull Management duties (swoop in, squawk, leave droppings, swoop out hahah)

github-actions · 2021-10-30T19:36:50Z

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

github-actions · 2021-12-29T22:38:04Z

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

david-allison · 2021-12-29T22:56:12Z

Not going to get to this for a while - scoped storage needs to happen and working on that is my prioirty.

github-actions · 2022-02-27T23:36:52Z

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

david-allison · 2022-08-23T21:27:41Z

Superseded by #12105

david-allison added Needs Review and removed Needs Review labels Aug 22, 2021

david-allison marked this pull request as draft August 22, 2021 21:03

david-allison force-pushed the 6772-import branch from fe873f5 to 1d48c6b Compare August 22, 2021 21:18

david-allison added the Blocked by dependency Currently blocked by some other dependent / related change label Aug 22, 2021

david-allison added the Needs Review label Aug 22, 2021

david-allison force-pushed the 6772-import branch from 1d48c6b to d3dd359 Compare August 22, 2021 22:17

david-allison removed the Blocked by dependency Currently blocked by some other dependent / related change label Aug 22, 2021

david-allison force-pushed the 6772-import branch from d3dd359 to 3221a42 Compare August 22, 2021 22:33

david-allison marked this pull request as ready for review August 22, 2021 23:09

david-allison commented Aug 23, 2021

View reviewed changes

david-allison force-pushed the 6772-import branch from 3221a42 to f3e2792 Compare August 24, 2021 04:21

david-allison mentioned this pull request Aug 24, 2021

Csv import: Refactorings & CsvMapping class #9445

Merged

7 tasks

david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Aug 24, 2021

david-allison added 4 commits August 25, 2021 00:06

allow indexed access to note types

8bd5bd8

usage: `collection.models[noteTypeId]!!`

feat: Text Importer file selection fragment

f117602

Handles a user selecting a file For use in the CSV Import (issue 6772)

feat: Text Importer field delimiter fragment

cb1ec2b

Handles obtaining a single character (and allows tab input) For use in the CSV Import (issue 6772)

feat: Text Importer field mapping fragment

ef48b59

Handles the UI of mapping from CSV to Note fields/tags For use in the CSV Import (issue 6772)

david-allison force-pushed the 6772-import branch from f3e2792 to 180459f Compare August 24, 2021 23:11

david-allison added 5 commits August 25, 2021 00:26

feat: Text Importer option setting fragment

6043871

Also made NoteImporter/TextImporter mutable to handle this as is in the Python Handles the UI of all options when mapping from CSV to Note fields/tags For use in the CSV Import (issue 6772)

note import: add progress logging

b34a373

For CSV import: Issue 6772

feat: Text Importer importing/progress fragment

d3cba0e

Performs import, shows progress and results For use in the CSV Import (issue 6772)

feat: Text Importer host fragment

c238523

Ties all the fragments together and responsible for workflow Related: 6772

feat: CSV Text Importer

12a233a

Adds activity, linked to host fragment and implemented as menu in DeckPicker Fixes 6772

david-allison force-pushed the 6772-import branch from 180459f to 12a233a Compare August 24, 2021 23:26

david-allison removed the Needs Author Reply Waiting for a reply from the original author label Aug 24, 2021

david-allison added the Needs Review label Aug 24, 2021

This comment has been minimized.

Sign in to view

david-allison added the Needs Author Reply Waiting for a reply from the original author label Aug 30, 2021

david-allison self-assigned this Aug 30, 2021

david-allison added Blocked by dependency Currently blocked by some other dependent / related change and removed Needs Review labels Aug 31, 2021

ankidroid deleted a comment from Arthur-Milchior Aug 31, 2021

david-allison removed the Blocked by dependency Currently blocked by some other dependent / related change label Aug 31, 2021

github-actions Bot added the Stale label Oct 30, 2021

david-allison removed the Stale label Oct 30, 2021

github-actions Bot added the Stale label Dec 29, 2021

david-allison added Needs a new dev For PR that were good start but the original dev' left. Any dev can take over and finish it. and removed Stale labels Dec 29, 2021

github-actions Bot added the Stale label Feb 27, 2022

krmanik added Keep Open avoids the stale bot and removed Stale labels Feb 28, 2022

david-allison mentioned this pull request Apr 6, 2022

Feature: Import CSV/TSV files #6772

Closed

BrayanDSO mentioned this pull request Aug 19, 2022

[GSoC] Implement the CSV importer #12105

Merged

7 tasks

david-allison removed Keep Open avoids the stale bot Needs a new dev For PR that were good start but the original dev' left. Any dev can take over and finish it. Needs Author Reply Waiting for a reply from the original author labels Aug 23, 2022

david-allison closed this Aug 23, 2022

Uh oh!

Conversation

david-allison commented Aug 22, 2021

Pull Request template

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

Uh oh!

david-allison commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikehardy commented Aug 22, 2021

Uh oh!

david-allison commented Aug 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-allison Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

david-allison Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

Arthur-Milchior Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

mikehardy Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

david-allison Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

mindinsomnia commented Aug 30, 2021

Uh oh!

david-allison commented Aug 31, 2021

Uh oh!

mikehardy commented Aug 31, 2021

Uh oh!

github-actions Bot commented Oct 30, 2021

Uh oh!

github-actions Bot commented Dec 29, 2021

Uh oh!

david-allison commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 27, 2022

Uh oh!

david-allison commented Aug 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

david-allison commented Aug 22, 2021 •

edited

Loading

david-allison Aug 23, 2021 •

edited

Loading

david-allison commented Dec 29, 2021 •

edited

Loading