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

Avoid duplicating crowdanki_uuid when cloning deck config #135

Merged
merged 2 commits into from
Dec 11, 2021

Conversation

aplaice
Copy link
Collaborator

@aplaice aplaice commented Jul 3, 2021

Please do not merge this before at least the first commit of #127 (this bug (the #118 bug) mitigates the more serious bug that #127 solves)!

Partially deal with #118.

Testing, it seems that I somehow haven't broken the tests despite fiddling with anki/hook_vendor without adding to the overrides.

Note that deck_conf_did_add_config is not available as a "legacy" hook, so the new hook mechanism has to be used. It might (or might not) be worth converting the existing hooks to the new mechanism.

I've chosen to simply delete the crowdanki_uuid property from the cloned deck config, rather than immediately regenerate a new uuid, to avoid code duplication — when the new deck config needs a uuid, one will be generated for it, by the standard mechanism.

Testing

This PR can be tested to show that it works correctly by following the reproduction sequence in #118 or by inspecting the crowdanki_uuids of the deck configs with the debug console:

  1. Open the console with CtrlShift;.
  2. Run the following and check that none of the UUIDs are the same (and are present/absent when expected):
for c in self.col.decks.all_config():
    if "crowdanki_uuid" in c:
        print(f"{c['crowdanki_uuid']} for {c['name']}")
    else:
        print(f"no uuid for {c['name']}")

Duplicate deck config crowdanki_uuids "in the wild"

There will almost certainly be many cases where users have deck configs with duplicate crowdanki_uuid and this PR obviously won't fix that — it'll just prevent more such cases occurring. Since this is a minor bug, I don't think that it's worth complicating the CrowdAnki codebase by including an automated fixer.

Instead, I'll write a debug console script for diagnosing and fixing this and the other UUID issues.

@aplaice
Copy link
Collaborator Author

aplaice commented Aug 22, 2021

In Anki 2.1.46, the fix no longer works with the new deck options change pop-up (if one uses "Clone preset", the deck config's UUID is sadly also cloned). (The fix still works if one uses the old deck options Qt dialog, accessible with Shift-click, but that's little consolation, since it's sadly going away, in the long run.)

I'll investigate and update! (Edit: will be considerably trickier than expected, as described below.)


The issue is that now the deck options screen isn't implemented in Qt, in python, but in JavaScript/Svelte, as a sort of mini-webpage. The options screen communicates with the "main" process via a request ("path" /_anki/updateDeckConfigs). Fortunately, the "server" is implemented in Python (in qt/aqt/mediasrv.py) and calls the UpdateDeckConfigs function from pylib/anki/decks.py, so we could implement some sort of monkey patching at either of these points. Unfortunately, the actual logic of UpdateDeckConfigs is on the Rust side (to which we don't have access), so we'd need to do quite a lot of monkey patching (intercept the contents of the request, parse it, change it to remove the duplicated uuid (we'll know which is the duplicate and which the original, because the duplicate should have a higher creation time), and then regenerate the request).


Given that the fix does work for earlier versions of Anki (up to and including 2.1.44) or if a user uses the old options screen, (for a newer version of Anki), it might perhaps be worth merging this PR, just without closing the associated issue.

@aplaice aplaice force-pushed the disambiguate_cloned_deckconf branch from 8d6432d to 44eafe2 Compare August 22, 2021 21:27
@ohare93
Copy link
Contributor

ohare93 commented Dec 2, 2021

Just had this issue affect me, yet again 😭 I am on Anki 2.1.44 and cloned via the Clone Preset button, as described above.

Where are the CrowdAnkiUUIDs themselves stored? Could they be periodically (on start up or on command) all checked and duplicates changed?

What is the easy way to make a fix when in this broken state?

@ohare93
Copy link
Contributor

ohare93 commented Dec 2, 2021

Fixed it a terrible manual way. Generated a new guid (replaced that in the CrowdAnki file), reimported back into Anki, then converted all the original notes to the new type and deleted the original note type. Really shitty that this is still happening though 😞 a regular user would have no chance to fix this, or even know what is going on

@aplaice
Copy link
Collaborator Author

aplaice commented Dec 2, 2021

Note type

Are you using CrowdAnki from master or from AnkiWeb? (The issue has been (mostly, subject to the provisos listed in #136*) fixed in master, but not released yet.) (If you're on master and still being hit by the bug, then it's more problematic.)

* The main proviso is that if you clone note types in an Anki/AnkiMobile/AnkiDroid instance other than the one where CrowdAnki is installed (and obviously you can't have CrowdAnki on mobile) you'll still be hit by the issue, but hopefully that should be rare. (Also, existing collections aren't cleaned up.)

Where are the CrowdAnkiUUIDs themselves stored? Could they be periodically (on start up or on command) all checked and duplicates changed?

Yes, they could be periodically checked (to work around the proviso and to fix existing Anki collections), but it's hard to determine which note model id is the original and which the new one (in principle, ids should be monotonically increasing, but I think there are subtle edge cases when timestamps happen to clash, and I don't want to change the UUID for the old model).). (Alternatively, we could have a second (?) storage place for note_model_id to crowdanki_uuid lookups, which wouldn't be affected by Anki's model cloning.)


Really shitty that this is still happening though

Yes, sorry!


(BTW this PR is about deck configs which almost nobody cares about, not note models.)

@aplaice
Copy link
Collaborator Author

aplaice commented Dec 2, 2021

What is the easy way to make a fix when in this broken state?

[note: for note models not deck configs]

Debug console:
from uuid import uuid1

uuids = []

for model in filter(lambda model: 'crowdanki_uuid' in model,
                    sorted(self.col.models.all(), key=lambda m: m["id"])):
    # we're sorting in the hope that note models with higher ids
    # (which mostly corresponds to creation date) were created later,
    # so we change the uuid of the copies, not the original
    cu = model["crowdanki_uuid"]
    if cu in uuids:
        print("Replacing UUID for note model " + model["name"] + "!")
        model["crowdanki_uuid"] = str(uuid1())
        self.col.models.save(model)
    else:
        uuids.append(cu)

@ohare93
Copy link
Contributor

ohare93 commented Dec 2, 2021

Really shitty that this is still happening though

Yes, sorry!

(BTW this PR is about deck configs which almost nobody cares about, not note models.)

No need to apologise 😉 I was complaining in general.

Note type

Are you using CrowdAnki from master or from AnkiWeb? (The issue has been (mostly, subject to the provisos listed in #136*) fixed in master, but not released yet, as Stvad is needed to upload to AnkiWeb and I don't want to bother him too frequently, so I wanted to get all the essential fixes into master before releasing.) (If you're on master and still being hit by the bug, then it's more problematic.)

I am on Master, but it seems I am out of date 😅 only 26 commits though 😆 my own dumb fault, then

Where are the CrowdAnkiUUIDs themselves stored? Could they be periodically (on start up or on command) all checked and duplicates changed?

Yes, they could be periodically checked (to work around the proviso and to fix existing Anki collections), but it's hard to determine which note model id is the original and which the new one (in principle, ids should be monotonically increasing, but I think there are subtle edge cases when timestamps happen to clash, and I don't want to change the UUID for the old model).). (Alternatively, we could have a second (?) storage place for note_model_id to crowdanki_uuid lookups, which wouldn't be affected by Anki's model cloning.)

Sounds like a reasonable fix 👏 I hope to have some time off soon, where I would be happy to help.

aplaice added a commit to aplaice/CrowdAnki that referenced this pull request Dec 5, 2021
The issue of note model UUIDs being cloned along with the note models
themselves has been fixed (Stvad#136).  However, many users' collections
will already contain such duplicated note model UUIDs.  Also, if a
user clones a note type on another platform, then the UUID will still
be duplicated.

The disambiguation is run before export and snapshot, since that's
when it's most needed to avoid broken `deck.json`s.

In the long, run we could perhaps switch to running the code only
after syncing (since our "attack surface" would be cloning of note
models on other platforms).

Running `disambiguate_note_model_uuids` takes 10 ms on a collection
with 20 note types (without duplicates), which is IMO an acceptable
overhead.

The file `disambiguate_uuids.py` could also contain the (far lower
priority) disambiguation of deck config UUIDs (see Stvad#135).
Fix Stvad#118.

Testing, it seems that I somehow haven't broken the tests despite
fiddling with anki/hook_vendor without adding to the overrides.

Note that deck_conf_did_add_config is not available as a "legacy"
hook, so the new hook mechanism has to be used.  It might (or might
not) be worth converting the existing hooks to the new mechanism.
@aplaice
Copy link
Collaborator Author

aplaice commented Dec 11, 2021

As noted, this fix doesn't help in Anki 2.1.46, with the new deck options change pop-up. Unfortunately, the approach from this PR can't be adjusted for the new pop-up (due to its architecture — JS frontend communicating with the Rust backend via a very thin Python wrapper).

(Something like #155 would work, but it'd have to be the only solution (not just as a safety net as in #155), which doesn't feel very elegant.)

However, the old deck options pop-up still appears when you use the old (v1) scheduler or shift-click on the "options" button (and obviously is fully functional for Anki 2.1.45 and lower), so I think it's still worth including the fix.

@aplaice aplaice force-pushed the disambiguate_cloned_deckconf branch from 44eafe2 to 4306b59 Compare December 11, 2021 13:37
@aplaice aplaice merged commit 19cfef2 into Stvad:master Dec 11, 2021
@aplaice aplaice deleted the disambiguate_cloned_deckconf branch December 11, 2021 13:40
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