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 an option to allow both viewing and adding duplicates #693

Merged
merged 2 commits into from
Mar 18, 2024
Merged

Add an option to allow both viewing and adding duplicates #693

merged 2 commits into from
Mar 18, 2024

Conversation

eloyrobillard
Copy link

@eloyrobillard eloyrobillard commented Feb 16, 2024

Overview

This PR keeps add buttons on when the term has a duplicate:

updated pop-up dictionary showing new add button for term with a duplicate

This way the user can both add duplicate notes and check current duplicates if they wish to. As mentioned below this is mainly useful when mining for:

  • a term with several different meanings
  • a term with a difficult meaning / unclear how to use it in a sentence, where several example sentences help

Also, as @Maltesaa mentioned: this helps to recreate a card for suspended notes without having to manually delete those suspended notes

Based on #74

Copy link

github-actions bot commented Feb 16, 2024

⚠️ Visual differences introduced by this PR; please validate if they are desirable.

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

@toasted-nutbread
Copy link

Prior to getting into the details of the implementation, a question: in your experience, how frequent is it that you need to do this? Or anyone else as well, how frequently is it a problem that there is already a note but you want to add another "duplicate" note?

@eloyrobillard
Copy link
Author

eloyrobillard commented Feb 17, 2024

Prior to getting into the details of the implementation, a question: in your experience, how frequent is it that you need to do this? Or anyone else as well, how frequently is it a problem that there is already a note but you want to add another "duplicate" note?

Can't speak for anyone else, but this is the one feature I've always wished Yomichan/Yomitan offered (and looking at #74, I'm not the only one).
I'd say I add duplicates for about 5-10% of the terms, usually for 2 reasons:

  • one term with several different meanings
  • one term with a difficult meaning / unclear how to use it in a sentence, where several example sentences help (more than one sentence per card takes too long to read imo, so I split them up)

@toasted-nutbread
Copy link

Okay, the reason behind my asking is because IMO this should probably not be an option, but just be a right-click menu on the buttons or something like that.

@eloyrobillard
Copy link
Author

eloyrobillard commented Feb 18, 2024

Okay, the reason behind my asking is because IMO this should probably not be an option, but just be a right-click menu on the buttons or something like that.

@toasted-nutbread

  1. As part of the existing Note IDs right-click menu for example, or on a different button?

  2. Do you favor right-click menus because they are better at hiding advanced features, or for a different reason?
    If it's indeed to hide them, that may be hiding it too well. For the note IDs menu, for example, I couldn't find any info aside from random comments in GitHub.

The made gain of having this as an option as that it can be left on forever1. Also, options are easy to document in the UI. Maybe it could be put all the way at the bottom of the "Check for card duplicates" section to further hide it, and call it "[Advanced]" or "Dangerously enable add buttons".

Or maybe even add an "You have duplicates for this card. Are you sure you want to add a new card?" alert when clicking the add buttons.

Footnotes

  1. So that I only have to click the add button, as opposed to right-click + left-click every time.

@eloyrobillard eloyrobillard marked this pull request as ready for review February 19, 2024 06:48
@eloyrobillard eloyrobillard requested a review from a team as a code owner February 19, 2024 06:48
@Maltesaa
Copy link

Maltesaa commented Feb 23, 2024

Or anyone else as well, how frequently is it a problem that there is already a note but you want to add another "duplicate" note?

  • one term with several different meanings
  • one term with a difficult meaning / unclear how to use it in a sentence, where several example sentences help (more than one sentence per card takes too long to read imo, so I split them up)

Another use case for this is when you want to recreate a card for suspended notes, I do this all the time.

You could ofc just delete or move the suspended cards to another deck, but not having to remember to do that would be nice. Having duplicates would also make it so you're able to see that the already existing note has the leech tag which would be helpful when making the card.

@djahandarie
Copy link
Collaborator

@eloyrobillard
Regarding UI:

  • I agree with @toasted-nutbread that this should not be a setting, as our settings page is way to complicated, and having settings introduces quite a bit of complexity into the codebase where you need to account for the combinatorial explosion of potential ways the app can be configured when trying to make any wide change after-the-fact
  • That said, I think right click menus are basically not a feasible UI pattern in browsers as they are very rare and almost no one would think to right click on stuff. I don't think we should use them at all.
  • I think we can achieve a decent "always-on" UI here by use of better iconography. For example, the plus icon can change into a "stacked" plus icon (i.e., the icon duplicated ontop of itself once and shifted maybe ~3px up and to the right), and change the hover text to indicate it's a duplicate add.

What do you think?

@eloyrobillard
Copy link
Author

eloyrobillard commented Feb 26, 2024

@djahandarie

The stacked "+" icon sounds fun, I'd be happy to implement it.

It would only show up when "Check for cards duplicates" is on, right?
In which case the description for "Check for card duplicates" wouldn't need to say "the add buttons will be disabled" anymore?

@Kuuuube
Copy link
Member

Kuuuube commented Feb 26, 2024

I think it would be best if the icon shows up always. But when "Check for cards duplicates" is on it shouldnt let you add a duplicate and should be grayed out similar to how it is now.

Although this does make the option's name a bit of a misnomer I think that would best integrate into how things currently are done.

@eloyrobillard
Copy link
Author

eloyrobillard commented Feb 26, 2024

I think it would be best if the icon shows up always. But when "Check for cards duplicates" is on it shouldnt let you add a duplicate and should be grayed out similar to how it is now.

When we don't check for duplicates how would we choose between the normal or stacked "+" icon, since we wouldn't know if there are duplicates?

@Kuuuube
Copy link
Member

Kuuuube commented Feb 26, 2024

It would have to always check for duplicates like that.

The main issue I see here with keeping the button active always when is "Check for cards duplicates" is on is visual clarity. At a very quick glance a user may not see the difference between a stacked + and a single +. If we're going to go that route and keep the option actually checking for dupes or not, maybe it should be something that stands out more like a red + when dupes are found instead.

@Kuuuube
Copy link
Member

Kuuuube commented Feb 26, 2024

Though, red vs green raises its own issues (colorblindness). So maybe red and stacked makes the most sense.

@eloyrobillard
Copy link
Author

eloyrobillard commented Feb 26, 2024

True. Could also be a smaller circle in top-right of the "+" icon showing the number of duplicates.

Either way, always checking for duplicates is probably not an option since it would make Anki/AnkiConnect freeze for some users with ultra-massive Anki collections (as per FooSoft#993).

@Kuuuube
Copy link
Member

Kuuuube commented Feb 26, 2024

Heres my suggestion for the icons, made them stacked based on what darius suggested as well:

add-term-kanji-red
add-term-kana-red

Capture1
Capture

If you want to use these heres the files icons.zip (they go into ext\images)

@eloyrobillard eloyrobillard changed the title Add an option to allowing both viewing and adding duplicates Add an option to allow both viewing and adding duplicates Feb 27, 2024
Copy link

codspeed-hq bot commented Feb 27, 2024

CodSpeed Performance Report

Merging #693 will degrade performances by 19.65%

Comparing eloyrobillard:add-duplicates-while-checking-for-them (1b860a6) with master (e47a0f4)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 2 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master eloyrobillard:add-duplicates-while-checking-for-them Change
transformations (n=57) 54.7 ms 37.2 ms +47.13%
Translator.prototype.findTerms - (n=39) 790.9 ms 984.2 ms -19.65%

@Casheeew Casheeew added the kind/enhancement The issue or PR is a new feature or request label Feb 27, 2024
@eloyrobillard
Copy link
Author

eloyrobillard commented Feb 28, 2024

Here's what I have for now (using @Kuuuube's buttons):

image showing new 'add duplicate' buttons

The new add buttons only show up when the entry has duplicates in Anki (so only when "Check for card duplicates" is on).

@Kuuuube
Copy link
Member

Kuuuube commented Feb 28, 2024

Gave it a test, it works well. Just needs a commit to actually add the svgs.

@eloyrobillard
Copy link
Author

added the SVGs

djahandarie
djahandarie previously approved these changes Mar 3, 2024
@djahandarie
Copy link
Collaborator

@toasted-nutbread If you have any comments on the code please go ahead and review, otherwise will merge in a day or two

@toasted-nutbread
Copy link

New icons should probably be added to resources/icons.svg as new layers, as this has generally served as the source for editing icons. Idk if anyone has modified this since Yomichan, but I used Inkscaped for it.

ext/js/data/anki-note-builder.js Show resolved Hide resolved
ext/js/comm/anki-connect.js Outdated Show resolved Hide resolved
ext/js/comm/anki-connect.js Outdated Show resolved Hide resolved
ext/js/background/backend.js Outdated Show resolved Hide resolved
ext/js/display/display-anki.js Show resolved Hide resolved
ext/js/display/display-anki.js Outdated Show resolved Hide resolved
ext/js/display/display-anki.js Outdated Show resolved Hide resolved
ext/js/comm/anki-connect.js Outdated Show resolved Hide resolved
ext/js/comm/anki-connect.js Outdated Show resolved Hide resolved
ext/js/background/backend.js Outdated Show resolved Hide resolved
@Casheeew Casheeew added the area/anki The issue or PR is related to Anki integration label Mar 4, 2024
@eloyrobillard
Copy link
Author

New icons should probably be added to resources/icons.svg as new layers, as this has generally served as the source for editing icons. Idk if anyone has modified this since Yomichan, but I used Inkscaped for it.

done

Copy link
Collaborator

@djahandarie djahandarie left a comment

Choose a reason for hiding this comment

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

Thanks for sticking through the long peer review process!

@djahandarie djahandarie added this pull request to the merge queue Mar 18, 2024
Merged via the queue into yomidevs:master with commit 7ee76d7 Mar 18, 2024
8 checks passed
@StefanVukovic99
Copy link
Collaborator

Belated, but the description in settings
image
doesn't seem to fit the way it now works?

May also be good to update/expand the docs at https://github.com/themoeway/yomitan/blob/7681131782d958997663b1fb443a3e32e8eef550/docs/anki-integration.md?plain=1#L108 to explain the new feature.

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.

7 participants