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 was trying to fix #27 yesterday so I happen to be familiar with this code.

The onFocusLost changes seem good. I'm fairly certain getting rid of self.editor is correct as previously EditManager was leaking an Editor reference.

I am not sure about the button state changes.

[mdyan]

Are you certain changed is meant to be returned through?

Seems like it's fine, although it appears false is always passed here? (There is no API as far as I can tell...)

https://github.com/ankitects/anki/blob/fc24a4be5293530b65d63038a0d0061a76ccedfd/qt/aqt/editor.py#L418

[kieranlblack]

Yeah changed should be returned here. The value is intended to be propagated through the hooks. My guess is that this is so hooks later on can detect if a hook before them modified the note already.

It is in the built code so I don't have a great way of linking to it but you can see this here:

class _EditorDidUnfocusFieldFilter:
    _hooks: list[Callable[["bool", "anki.notes.Note", "int"], bool]] = []

    def append(
        self, callback: Callable[["bool", "anki.notes.Note", "int"], bool]
    ) -> None:
        """(changed: bool, note: anki.notes.Note, current_field_idx: int)"""
        self._hooks.append(callback)

    def remove(
        self, callback: Callable[["bool", "anki.notes.Note", "int"], bool]
    ) -> None:
        if callback in self._hooks:
            self._hooks.remove(callback)

    def count(self) -> int:
        return len(self._hooks)

    def __call__(
        self, changed: bool, note: anki.notes.Note, current_field_idx: int
    ) -> bool:
        for filter in self._hooks:
            try:
                changed = filter(changed, note, current_field_idx)
            except:
                # if the hook fails, remove it
                self._hooks.remove(filter)
                raise
        # legacy support
        changed = anki.hooks.runFilter(
            "editFocusLost", changed, note, current_field_idx
        )
        return changed

[kieranlblack]

@mdyan I cleaned up the code a little, I think this version is a little more structured, what are your thoughts?

[mdyan]

The changes to onFocusLost look good to me.
For posterity I'd like to point out that these change the focus behavior, but I think this is fine.

[mdyan]

If changed is supposed to be returned, I believe this can be all simplified to

changed = update_fields(note, field, allFields)

and then we_changed can be deleted

[kieranlblack]

The code is a little awkward here, I am open to suggestions and while we would be able to |= with changed, I would like to avoid reassigning function parameters.

If we wanted to get rid of we_changed we could instead return early, something like what I have below. I actually like this pattern a little more than what I currently have.

        if note_type := note.note_type():
            allFields = mw.col.models.field_names(note_type)
            field = allFields[index]
            if not update_fields(note, field, allFields):
                return changed

        return True

[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]

LGTM

[mdyan]

The changes to onFocusLost look good to me.
For posterity I'd like to point out that these change the focus behavior, but I think this is fine.

[mdyan]

I really don't understand how Svelte works but from my naive glance at NoteEditor.svelte this solution seems correct as EditorState is updated and broadcast in the onMount callback.

Since you reported loadNote was being called before the UI is mounted to the DOM (I'm a bit fuzzy on what this actually means though), this code should fix the race condition.

[kieranlblack]

Thanks for taking a look. As for the DOM business, in that piece of code you pointed out, since it is in the onMount, it runs when the component has been "mounted" aka rendered for the first time. This is problematic for us since even though the browser might be set up, if the component hasn't been rendered yet then the toggle function wouldn't be available for us to use (since it is injected in the onMount). Hopefully this clears things up.

[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]

I think new_state is unused?

[kieranlblack]

Yeah it is not used, do you have a linter complaining? If not I would tend towards not _ing it in the parameter list.

[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.