Skip to content

[GSoC] Implement the CSV importer#12105

Merged
mikehardy merged 9 commits into
ankidroid:mainfrom
BrayanDSO:csvImporter
Aug 25, 2022
Merged

[GSoC] Implement the CSV importer#12105
mikehardy merged 9 commits into
ankidroid:mainfrom
BrayanDSO:csvImporter

Conversation

@BrayanDSO

@BrayanDSO BrayanDSO commented Aug 19, 2022

Copy link
Copy Markdown
Member

Purpose / Description

On top of #12072

Fixes

Closes #6772
Closes #9437

Approach

This implements the base for using upstream's HTML/Js pages directly on AnkiDroid.

I've used krmanik#23 as base of this PR. I wouldn't have managed to do it without all the previous work of Mani and Damien, so I thank them very much.

As discussed on krmanik#23, to use an Anki HTML page, we should be able to intercept POST requests. After some proof of concepts made by Mani, the conclusion was that hosting a local server, like the desktop application, may be most suitable approach.

To create the local server, I've used the same approach of krmanik#23 and added NanoHTTPD, a java server library, as dependency. I don't have experience with any Java server libraries, so I can't tell with confidence if this is the best library, but it seemed solid to me and already had a working model on Mani's PR, so using it looked like the best choice for the moment.

Besides the server, this PR implements the base classes for creating AnkiPages. Each page can easily extend PageFragment and define what it needs. I've tried using the new Statistics, Card Info, Deck Options pages and they work well. Try https://github.com/BrayanDSO/Anki-Android/tree/csvImporterOtherPages if you want to test them (the deck options screen may need some extra work).

Finally, the CSV importer page and its endpoints are implemented and can be used with the new backend.

PENDING (for future PRs):

  • Close the Import dialog after the taps "OK" on the progress dialog
  • Inject CSS to match the themes' colors
  • Maybe hide the file path with CSS/Js, because it may be big and there isn't much horizontal space in phone screens on portrait mode (already on upstream)
  • Not for my GSoC project, but the other screens could be implemented too, as they are mostly done, so other people could test/work on them as well

How Has This Been Tested?

NOTICE: this needs an updated WebView (77+) to work correctly (see #6772 (comment) and the comments below for discussion), which shouldn't be a problem as the project's minSdk (21) supports it. But if you are testing on a emulator, be aware that the WebView shipped on some older SDK versions may be an old version, so either update the WebView or use a newer SDK.

NOTICE 2: there may be a flickering issue after remapping fields, but that was already solved on upstream (ankitects/anki#2005)

  1. Enable the new backend (Settings > Advanced)
  2. Deck picker > Three dot menu > Import > Text file
  3. Select a CSV file (I've tested with Anking, pretty heavy and popular deck)
    • Here's a mini version (23KB) if anyone wants to test it: MiniAnking.txt
    • And here's a slightly cut version (GitHub only allows files up to 25MB):
      Anking.txt
  4. Configure anything on the CSV importer
  5. Tap Import
Screen_Recording_20220821-073427_AnkiDroid.mp4

Checklist

Please, go through these checks before submitting the PR.

  • 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

@BrayanDSO BrayanDSO added the Blocked by dependency Currently blocked by some other dependent / related change label Aug 19, 2022
@BrayanDSO BrayanDSO force-pushed the csvImporter branch 3 times, most recently from 886bfe4 to e6ea5ff Compare August 19, 2022 18:49

@dae dae left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt
Comment thread AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt Outdated

@krmanik krmanik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we move last two commits to separate PR for better diff in this PR?

@BrayanDSO BrayanDSO marked this pull request as ready for review August 20, 2022 11:30
@BrayanDSO BrayanDSO added Needs Author Reply Waiting for a reply from the original author Review High Priority Request for high priority review labels Aug 20, 2022
@BrayanDSO

BrayanDSO commented Aug 20, 2022

Copy link
Copy Markdown
Member Author

Should we move last two commits to separate PR for better diff in this PR?

Good idea. Done it.

@BrayanDSO BrayanDSO removed the Blocked by dependency Currently blocked by some other dependent / related change label Aug 20, 2022
@mikehardy

Copy link
Copy Markdown
Member

Maintainer note (or, other maintainer note, since dae and krmanik are both here already): I was really curious about both the localhost server and the HTML/JS from the backend work and have also participated in PR 23 on krmanik's fork while this was developed.

TL;DR: localhost server really seems like the best approach after testing, and I'm super-excited to get HTML/JS support from the backend. This PR is pretty amazing in the forward progress it represents for AnkiDroid

@BrayanDSO BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Aug 21, 2022
@BrayanDSO

Copy link
Copy Markdown
Member Author

Updated the PR's description with a new video, now showing cancelling and undoing the import as well.

@BrayanDSO BrayanDSO force-pushed the csvImporter branch 2 times, most recently from 26cbe2d to eedf1a2 Compare August 21, 2022 11:35

@david-allison david-allison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The future is now! Incredible work

- Extract the try/catch logic from launchCatchingTask to `runCatchingTask`
- Create `withContextCatching` and `runBlockingCatching` methods
@BrayanDSO

Copy link
Copy Markdown
Member Author

Only squashed/simplified the last commits with the last push force

@dae dae left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work on adding undo!

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt Outdated
Comment thread AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt Outdated
@dae

dae commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

Maybe hide the file path with CSS/Js, because it may be big and there isn't much horizontal space in phone screens on portrait mode

Already coming: ankitects/anki@2e2fc79

- Remove `withContextCatching` (unused)
- Improve `runBlockingCatching` javadoc
@lukstbit lukstbit added the Needs Second Approval Has one approval, one more approval to merge label Aug 23, 2022
@david-allison david-allison mentioned this pull request Aug 23, 2022
8 tasks

@mikehardy mikehardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't have anything actually blocking this. I'm most happy about the monumental collaboration here on all levels

David spearheading then everyone else working to get Kotlin going
Damien and Mani's efforts to get the new version of the backend up and plumbing Anki JS through
Brayan, all the work here

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/pages/AnkiServer.kt
Comment thread AnkiDroid/src/main/java/com/ichi2/anki/pages/PagesActivity.kt Outdated
@BrayanDSO

This comment was marked as resolved.

So it's compatible with ipv6 too

@krmanik krmanik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have pulled the PR locally and tested it on an actual device. I have tested import feature using the following files.

  1. Anking.txt, provided by the Brayan
    File Size: 24 MB
    Lines in file: 20k
    Note Type: Basic
  2. Tatoeba Chinese English sentences pair
    File Size: 5 MB
    Lines in file: 63k
    Note Type: Basic
    I have interrupted import after 20k notes to test interrupt.

I am happy with the result of the import-csv feature. But the only issue I am seeing is that only the text file is selectable to import, other file (csv and tsv) is not selectable. I renamed files to test import.
So, may be follow up PR for making the csv and tsv selectable should be fine.

This is one of the best features coming to AnkiDroid. Thanks to @BrayanDSO.

@mikehardy

Copy link
Copy Markdown
Member

Let's merge this to unblock all the stuff piled up on it (multiple follow-ons, that is), and the csv file selection issue can be a follow-on as well

@mikehardy mikehardy merged commit ea20f01 into ankidroid:main Aug 25, 2022
@github-actions github-actions Bot added this to the 2.16 release milestone Aug 25, 2022
@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 Aug 25, 2022
@BrayanDSO

BrayanDSO commented Aug 25, 2022

Copy link
Copy Markdown
Member Author

I am happy with the result of the import-csv feature. But the only issue I am seeing is that only the text file is selectable to import, other file (csv and tsv) is not selectable.

lol. Learned something today. I'd thought that Android imports were based on MIME types, so if it works with .txt, renaming it to .csv shouldn't change anything, which means I'm probably missing something.

But that is easy to solve anyway

@BrayanDSO BrayanDSO deleted the csvImporter branch August 25, 2022 16:44
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.

Feature: Import CSV/TSV files

6 participants