allow use of multiple editors and fix initial button state

[kieranlblack]

This PR fixes two things currently broken with the editor integration:

update editor focus lost hook to report when note was changed

Previously when a note was modified, the plugin would manually reload the note in the editor. This commit modifies things to take advantage of the built in hook infrastructure to auto-reload the note.

As a bonus, this fixes a bug caused assuming only one editor would be open at a time and closes #27.

ensure button status is correctly set when editor opens

The loadNote / editor_did_load_note hooks run as soon as a note is loaded, but not necesarrily after the ui component has been mounted to the DOM. By adding a delay of 50 ms before setting button state, this should help alleviate the race condition.

[kieranlblack]

While this is an improvement over the original state of things, the state of the toggle is currently shared across all open editors rather than being toggleable on a per-editor basis.

Also, if the button is toggled in one editor, it will not be toggled in the other editors.

I'm not sure of a good mechanism to clean up editor references when they are closed out so I wanted to avoid doing that for now.

[mdyan]

I'm not too sure about the race condition you mentioned, but I would guess one way to fix it might be to add a editor_did_init hook and refresh the button in there. It seems like this happens after Editor.setupWeb() is called.

[Gustaf-C]

Thank you for this! I'm in the middle of my midterms, so I won't be able to have a proper look at this for a while, although I'll say that I'm not too fond of the idea of solving a race condition by adding an arbitrary delay. If it could be solved in a more consistent way that would be preferable.

[kieranlblack]

@mdyan The editor_did_init hook actually fires really early on in the lifecycle, before even the editor_did_load_note or loadNote hooks run so the component won't be mounted at those points in time also the note type won't be loaded for us to see if the plugin should be active or not.

[kieranlblack]

@Gustaf-C good luck with the midterms 😤. I too am not a fan of trying to smooth over the race condition with the delay, I don't really think of this so much as a solution as instead a reliable alleviation.

I took another look this time with less tired eyes and I think we can use the work done with the editor_state_did_change hook to detect when the component has been mounted. I'll implement something with this and we should be able to get rid of the race condition.

Edit: not ideal is that this hook is new in 23.10 so anyone with older clients will have issues

[mdyan]

I just saw your comment about EditorState only being available in 23.10. What if we use gui_hooks.editor_did_focus_field instead?

EDIT: On second thought, I don't think that will work, as the editor does not necessarily get focus.

[kieranlblack]

Good idea but yeah, I think for example the editor in the browse menu doesn't automatically focus the first field.

If it was really important to maintain backwards compatibility, we could add a check for the Anki version and only register the hook / check for the editor state if the version was high enough along with adding the delay-based approach from earlier to smooth over the race condition to handle things.

I'm not sure what the userbase looks like of the new fork though, considering it was created in general to be compatible with 23.10, I have few qualms about leaving the older versions behind.

[Gustaf-C]

Yes I forked this so people could run it with 23.10, and the fact that the fixes so far seem to have been backwards compatible is just a bonus. I'll leave up the old version with backwards compatible changes but restrict it to those running <= 2.1.66, and once this fix has been integrated it will be released as a new version restricted to >= 23.10.

Making lots of code just in case someone is using an old version is not something I feel like maintaining.

[Gustaf-C]

Looks good!

Not sure why those imports fail the tests though...

[kieranlblack]

Not sure why those imports fail the tests though...

aqt and anki are not listed as requirements in requirements.txt so they are not getting installed

[kieranlblack]

Once we merge #35 and get these rebased, the tests should pass.

[Gustaf-C]

I'm not completely sure why that worked, but at least it did.

[kieranlblack]

Hmm, I am also not sure, that is going to cause issues with linters though since they cannot explore that far to find a module. I'll mess around with it in test PR.