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

add support for overwriting existing card #859

Merged
merged 14 commits into from
May 21, 2024

Conversation

StefanVukovic99
Copy link
Member

@StefanVukovic99 StefanVukovic99 commented Apr 23, 2024

Implements part of #578

Copy link

github-actions bot commented Apr 23, 2024

✔️ No visual differences introduced by this PR.

View Playwright Report (note: open the "playwright-report" artifact)

@StefanVukovic99
Copy link
Member Author

This basically works now, but it's a full overwrite of all the fields. I think that covers some use cases by itself, like fixing audio and updating from JMdict to jitendex, so should we clean this up and do the per-field settings in a separate PR?
Context:

image
with the "Mode" determining whether the field should be:

  1. skipped (for fields that may be manually changed, e.g. picture)
  2. appended to (e.g. allows gathering example sentences)
  3. overwritten with the newest version.

@Kuuuube Kuuuube added kind/enhancement The issue or PR is a new feature or request area/ui-ux The issue or PR is related to UI/UX/Design area/anki The issue or PR is related to Anki integration and removed area/ui-ux The issue or PR is related to UI/UX/Design labels May 5, 2024
@StefanVukovic99 StefanVukovic99 marked this pull request as ready for review May 19, 2024 14:53
@StefanVukovic99 StefanVukovic99 requested a review from a team as a code owner May 19, 2024 14:53
@StefanVukovic99
Copy link
Member Author

I think this is good to go

@Kuuuube
Copy link
Member

Kuuuube commented May 19, 2024

Are you able to test/check if this works on ankiconnect android? Not a blocker but just so we know what to expect.

@StefanVukovic99
Copy link
Member Author

Not easily, I've never tested anything on mobile

@Kuuuube
Copy link
Member

Kuuuube commented May 19, 2024

Looks like when there are multiple duplicate cards it just picks the first one to overwrite. Might be good to make it clear to users in some way which card is being overwritten.

@Kuuuube
Copy link
Member

Kuuuube commented May 19, 2024

Looked over everything and tested all cases of the Duplicate card scope and When a duplicate is detected.

The only problem I found is when the model of the duplicate note and the selected anki note model don't match and the duplicate note is in the same deck, attempting to overwrite does nothing. The button should probably be grayed out for this.

Copy link
Member

@Kuuuube Kuuuube left a comment

Choose a reason for hiding this comment

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

Approving despite the previously mentioned issue.

It isn't reasonable to pull the existing duplicate notes from anki to check their model and could lead to big slowdowns in the anki buttons appearing if it pulls notes that have a lot of media.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really not a fan of the recycle icon because it's so overloaded with semantics. It could mean recycle, reset. I feel like the semantic "overwrite" wouldn't show up in people's vocabulary until like 5 definitions down.

I'm going to mark this as non-blocking since we can change it in the future.

@jamesmaa jamesmaa added this pull request to the merge queue May 21, 2024
Merged via the queue into themoeway:master with commit ba9fa33 May 21, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/anki The issue or PR is related to Anki integration kind/enhancement The issue or PR is a new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants