Check for Duplicates In Mining Mode

[bpwhelan]

closes #107.

Added a config (default false) for checking duplicates when entering mining mode.

This will check all the results in mining mode for an already existing expression in the configured anki deck, and display a warning next to the result if it finds one.

Creating this as a draft as I want to:

• Take another look at the method for checking duplicates. I assume it can be simplified quite a bit... I did, however, already try creating a method that would send everything to anki in one call, since "canAddNote" can receive multiple notes, but performance was markedly worse.
• Do some extensive testing on the overall performance impact of this with large dictionary(s) as requested. With just the default dictionaries it seems to be completely unnoticeable.

[rampaa]

I did, however, already try creating a method that would send everything to anki in one call, since "canAddNote" can receive multiple notes, but performance was markedly worse

Interesting. Is AnkiConnect responding to, say, 100 separate canAddNote requests faster than a single request for those 100 notes? Or is it slower due to how we create and process those requests in JL?

[rampaa]

Accessing the Result property of a Task is a blocking operation, thus it should be avoided. Instead we should do something like

bool duplicateCard = await MiningUtils.CheckDuplicate(result).ConfigureAwait(true);
if (duplicateCard)

[bpwhelan]

This is the biggest thing that I'm having trouble with since this method is synchronous.

Toying with something to do the check for duplicates after the window is loaded and toggling the visibility so there is 0 impact on loading the window.

[rampaa]

We probably should use GetMiningParameters here. The implementation provided here just partially creates mining parameters but for all we know the user might have SourceText as their note type's first field (which is all Anki cares for when checking if a card is a duplicate AFAIK) and its value is missing here.

Or better yet, we can check what's the first Field in the config and just create a note with that field to check whether it's a duplicate note.

[rampaa]

Why do we have this check here? As is, this method will always return false if the user enabled the Allow duplicate cards option, is this really the intended behavior?

[bpwhelan]

Already removed in local.

[bpwhelan]

IN 0d51e21

• Changed this to add the icon (collapsed), and then do the duplicate check work afterwords.
• Moved around boolean logic to where we don't even enter the duplicate check method if we don't need to.
• Changed to only send one call to ankiconnect, when i got it to go successfully async, ankiconnect could not handle the requests and most of them were getting thrown out due to 503 or something.

2 things..

• I think the checkduplicates method is a bit overcomplicated... But I couldn't really find another way to do it cleanly.
• I'm not sure if there is any danger with with this line. I can't get it to break, but I'm not entirely confident what would be the best way to make this safer.

[rampaa]

CheckResultForDuplicates method should return a Task because:

1. async void should always be avoided, the only exceptiona to this rule is event handlers
2. Before calling the UpdateLayout method in DisplayResults method, we should ensure we made all the changes to the GUI, because otherwise UpdateLayout will not be able to render the GUI in its final state and methods like UpdatePosition should be called after the popup is rendered in its final state. So we can't use a fire-and-forget approach here. So we do need to await CheckResultForDuplicates's result and waiting CheckResultForDuplicates before showing the popup effectively means that the lookup speed is still affected by CheckResultForDuplicates and I am not sure if we can avoid it.

That being the case, I suggest discarding CheckResultForDuplicates altogether and doing the following in DisplayResults method:

    public Task DisplayResults(bool generateAllResults)
    {
        ...

        bool[]? isDuplicateCardArray = null;
        int resultCount;
        if (generateAllResults)
        {
            resultCount = LastLookupResults.Count;

            CoreConfigManager coreConfigManager = CoreConfigManager.Instance;
            if (coreConfigManager.CheckForDuplicateCards
                && !ConfigManager.Instance.MineToFileInsteadOfAnki
                && coreConfigManager.AnkiIntegration)
            {
                isDuplicateCardArray = await MiningUtils.CheckDuplicates(LastLookupResults, _currentText, _currentCharPosition).ConfigureAwait(true);
            }
        }
        else
        {
            resultCount = Math.Min(LastLookupResults.Count, ConfigManager.Instance.MaxNumResultsNotInMiningMode);
        }

        StackPanel[] popupItemSource = new StackPanel[resultCount];

        for (int i = 0; i < resultCount; i++)
        {
            LookupResult lookupResult = LastLookupResults[i];

            if (!_dictsWithResults.Contains(lookupResult.Dict))
            {
                _dictsWithResults.Add(lookupResult.Dict);
            }

            popupItemSource[i] = PrepareResultStackPanel(lookupResult, i, resultCount, pitchDict, pitchDictIsActive, showPOrthographyInfo, showROrthographyInfo, showAOrthographyInfo, pOrthographyInfoFontSize, isDuplicateCardArray?[i] ?? false);
        }

        PopupListView.ItemsSource = popupItemSource;
        GenerateDictTypeButtons();
        UpdateLayout();
    }

Then we can simply do the following in PrepareResultStackPanel

if (duplicateCard)
{
    Button duplicate = new()
    {
        ...
    };

    _ = top.Children.Add(audioButton);
}

I think this is the proper way of doing it. Though I'd like to have some tests regarding the performance impact of doing it like this.

[bpwhelan]

Yeah, This is how I had it written at some point... But it was the most noticeable in terms of performance for the time I had it in.

I'm not really sure I get the technical reason for everything to be final before UpdateLayout but my understanding is not really that important. Guess my thought was that the buttons already exist in the render, their visibility is just being toggled.

[rampaa]

Guess my thought was that the buttons already exist in the render, their visibility is just being toggled.

If you used Visibility.Hidden instead of Visibility.Collapsed that would mostly be correct. Visibility.Collapsed does not reserve space for the control in the layout, thus when you make it visible it can change the popup height/width when dynamic height/width is enabled for the popup, which can, in turn, affect where it should be positioned exactly. If we were to use Visibility.Hidden instead of Visibility.Collapsed, then we wouldn't have this problem but then we would have blank space for non-duplicate entries where the duplicate icon is placed.

[bpwhelan]

Everything done including suggested changes, sitting on it for a bit to test and refactor.

[bpwhelan]

Sorry for the delay on this. in a687c8d.

• Moved Icon to the end of the row so we can actually toggle visibility of the icon instead of uncollapse it. Let me know if this is still not desired. I would prefer not to add any time to lookups even if it's relatively small, and this seems like the only realistic way of doing it.
• PR review changes like return types, array instead of List, etc.
• Changed to Textblock instead of button
• Made method to get first mining param instead of getting everything and then only grabbing the first.

Edit: Realized I forgot to include an image of what it looks like on the end.

[bpwhelan]

Changes in dbcc60a all per PR. Bulk of this commit is in GetMiningParameter to fill out all the ones possible, like you said just a lot of duplicate code ripped straight out of the full GetMiningParameters method.

• Fill out GetMiningParameter Method
• Move duplicateIcons List out of Fields
• More small Object/List Related Changes

[bpwhelan]

In a5aee43, more changes per PR review

Add braces to case
refactor frequencies to inline conditional
get rid of unused var in Preferred Frequency
move unused and default cases to top
moved ankiconfigcheck
downmerged from upstream
changed to array instead of list per upstream change
other small pr related changes

[rampaa]

Thanks for contributing to JL!