Skip to content

Csv import: Refactorings & CsvMapping class#9445

Merged
mikehardy merged 18 commits into
ankidroid:masterfrom
david-allison:6772-import-refactor
Aug 24, 2021
Merged

Csv import: Refactorings & CsvMapping class#9445
mikehardy merged 18 commits into
ankidroid:masterfrom
david-allison:6772-import-refactor

Conversation

@david-allison

Copy link
Copy Markdown
Member

Pull Request template

Purpose / Description

This breaks off a few changes from #9437 for the CSV import

How Has This Been Tested?

The main change was tested, this should purely be adding code, tested bugfixes and nonfunctional refactorings

Learning (optional, can help others)

I'm unlikely to going to get a large change in in a single PR, and it's better to break it down, despite feeling it's readable as-is

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

@mikehardy

Copy link
Copy Markdown
Member

lint check is possibly a remnant as you teased the commits apart?

Lint found 2 errors. First failure:
Fix the issues identified by lint, or add the following to your build script to proceed with errors:
/home/runner/work/Anki-Android/Anki-Android/AnkiDroid/src/main/res/drawable/edit_text_color_enabled_selector.xml:20: Error: The resource R.drawable.edit_text_color_enabled_selector appears to be unused [UnusedResources]
...

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/DeckSpinnerSelection.java

@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.

Read through it all - seems okay. I'm a bit unclear on major vs minor changes (dispatched where? who would use those handlers + how do you add them to use the facility? I didn't see it in the test so was just a bit unclear). One method name thought, but otherwise LGTM + want to merge

Comment thread AnkiDroid/src/main/java/com/ichi2/utils/SequenceUtil.kt Outdated
@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Aug 24, 2021
@david-allison

Copy link
Copy Markdown
Member Author

I'll add more documentation on the dispatch methods. I'd rather the code spoke for itself than explain it here. Thanks!

@david-allison david-allison self-assigned this Aug 24, 2021
Darkens/lightens the text if a field is disabled

usage:

android:textColor="@drawable/edit_text_color_enabled_selector"

tested on all 4 themes
Adds a lot of noise, and exceptions are expected
The default value for "did" is null when creating a template:

Models.defaultTemplate
Also fixes a bug which stems from this, where
showActivityFailedScreen() would fail
It's used in a few places now, best to extract
Same name as in Anki Desktop
This documents a special case in the mapping for consumers
Allows mapping from a collection of CSV columns to notes
(fields and tags)

Also includes Parcelize to allow for simple serialisation
of objects to bundles

For use in the CSV Import (issue 6772)
Allows users to handle this exception separately
Allows users to handle this exception separately

Since we use streams, this exception can be thrown later
@david-allison

Copy link
Copy Markdown
Member Author

Awaiting re-review (assuming I didn't blow up CI), especially to see if the distinction between major and minor changes has been made more clear.

If not, I'll explain the usages and let's brainstorm some better docs.

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

@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.

All LGTM - boom

@mikehardy mikehardy merged commit ec8d716 into ankidroid:master Aug 24, 2021
@mikehardy

Copy link
Copy Markdown
Member

was waiting on this one for a re-sync on I18n now I'll do an alpha with fresh strings and all the merges of the day

@david-allison david-allison deleted the 6772-import-refactor branch August 24, 2021 21:55
@mikehardy mikehardy added this to the 2.16 release milestone Oct 15, 2021
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.

2 participants