Seems to keep Popup_dictionary addon from functioning

[grahamkwaj]

Love the addon, unfortunately I can't seem to use it and popup_dictionary at the same time. Night_mode will always work if enabled but it blocks the functioning of some other addons including the most essential one for me. Is there anything I can do to get around this?

[krassowski]

Hi @grahamkwaj, thanks for reporting this issue. Could you tell me which version of Anki and the Night Mode add-on do you use?

[grahamkwaj]

Night mode I re-installed the latest version yesterday, 2.2.3, Anki version 2.1.5 Qt.5.9.2

…

On Nov 18, 2018, at 04:55, krassowski ***@***.***> wrote: Hi @grahamkwaj, thanks for reporting this issue. Could you tell me which version of Anki and the Night Mode add-on do you use? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[krassowski]

Thanks, that narrows the scope.

I have installed the add-on (and noted that it is not published yet and not officially supported in Anki 2.1) though I really have no idea how it works and what it is supposed to do. Don't get me wrong - I am happy to help, but the usage and configuraion sections in documentation are literally empty.

Would you mind writing a reproducible step-by-step instruction to reproduce your issue (i.e. how it works when night mode is disabled and how it does not work when it is enabled), preferrably with some screenshots?

[krassowski]

@glutanimate, I think that you might want to now that we are chatting about an experimental version of one of your add-ons in here. Please feel welcome to comment if there is anything you would like to add.

[grahamkwaj]

Thanks for looking into this! Popup dictionary brings pulls up all cards related to the word or phrase highlighted. Anki simply behaves like I don't have the add on if I enable night mode. You can either highlight, and press ctrl+shift+D or just double click the word and see all other related cards. [image: image.png] Night mode off [image: image.png] Night mode on

…

On Sun, Nov 18, 2018 at 6:19 PM krassowski ***@***.***> wrote: @glutanimate <https://github.com/glutanimate>, I think that you might want to now that we are chatting about an experimental version of one of your add-ons in here. Please feel welcome to comment if there is anything you would like to add. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#53 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ArCwp0HVgbd5DfklM9pzlO2j8K99XoCiks5uwfj2gaJpZM4YnwwO> .

[glutanimate]

@krassowski Thanks for the nudge!

Graham explained the user-facing part well. The expected behaviour is for the user to be able to double-click any term or phrase to invoke a pop-up of related cards. Alternatively they might use the Ctrl+Shift+D hotkey to do the same.

But from a technical standpoint, the add-on appends some custom HTML to the reviewer to add support for the pop-up, etc. This, in all likelihood, is where it might be interacting with Night Mode. Seems like the issue is limited to 2.1. From what I recall I actually took care to support NM on 2.0 (the pop-up even has a NM-specific theme!).

I'm hoping to push forward with my Anki PR to improve the add-on conflict situation in web views soon. So perhaps resolving this might be better relegated until that is part of Anki proper.

For that matter, if you have any suggestions on how to best avoid conflicts like this on Anki's side I would love to hear them! I'm not sure if my approach in the PR is the best, and I'd love to hear your feedback as someone who has extensive experience with modifying Anki's web views.

[krassowski]

@glutanimate I gave a thumb up to your PR the second I saw it! I tried my best to leave some constructive comments in the discussion, I hope that this help.

Getting back to the discussed add-on - it's awesome! I like this function very much.

@grahamkwaj I played around a bit and found a workaround for you. Please use the menu bar and go to View → Night Mode → Choose what to style and untick "Reviewer Cards". It should work then (though some night mode eye-candy styling rules and functions will be disabled)

Here is my toy example:

[glutanimate]

Hey Michal,

I've been revisiting some of the commonly reported add-on conflicts recently, trying to find a proper way to resolve them. As before, using Pop-up Dictionary (PD) and Night Mode (NM) together currently only works when disabling 'Reviewer Cards' styling. The same issue, albeit without a workaround like that, also seems to be the case for the Touch Screen add-on (TS).

We talked about the PR to Anki before, and that's definitely an effort I want to pick up again very soon, but at least over the short and medium term, I think it would be great if we could figure out if there is another viable workaround.

Adoption of new Anki releases takes time, and given that update notifications are oftentimes multiple point releases behind, a solution implemented on Anki's side would likely still take upwards of 3 months to sift though to the majority of the installation base. Even then you would still have to deal with stragglers on old Anki releases who might not want to update out of fear of messing with a mission-critical app. All of this of course is made worse by the fact that we don't have an easy way to specify a minimum required Anki version, like many other plug-in systems do.

In any case, I've been looking at the aforementioned add-ons' code recently, and it seems to me that the core issue lies with the different ways in which NM and TS, and PD and many other add-ons respectively, patch Reviewer.revHtml. PD and most other add-ons wrap Reviewer.revHtml using Anki's native anki.hooks.wrap, whereas NM and TS seem to use a custom decorator to modify the instance method in mw.reviewer.revHtml. Sadly it seems like that approach overrules any changes performed to Reviewer.revHtml for add-ons that are imported after NM or TS. This is illustrated by the fact that renaming other add-ons, or reversing the import order with the ANKIREVADDONS env var, can temporarily fix these types of collisions. [1]

Obviously I'm not familiar enough with NM's internals to say for sure if this custom approach for patching revHtml is necessary, but I do think it's worth discussing whether there perhaps might be a different way for NM to go about this. From my limited understanding of NM's feature-set, it seems like conditional appending of content into mw.reviewer.web via revHtml should be achievable without discarding changes to the method by other add-ons.

Right now, any add-on that depends on modifying Reviewer.revHtml is destined to malfunction while NM (or respectively its reviewer styling) is active. With NM being as popular as it is, this constitutes a major sticking point in Anki's add-on ecosystem. So if we could find a way to eliminate this issue once and for all, it would be a great boon to add-on development as a whole.

If there's anything I can do to help with this effort, please do let me know. I tried walking through NM's code to see if there is a quick and easy PR I could submit, but it would take me much longer to familiarize myself with the code to a point where I would feel comfortable to do so. So I felt the best point to start would be to simply put this up for discussion for now. If we're lucky, perhaps we can find a quick an simple way to address this that would not require any major overarching architectural changes to NM.

Thanks!

---

[1]: Please note that this means that it's important to be wary of the package name when testing for add-on conflicts. I.e.: You will likely observe different results when testing with night_mode vs. 1496166067.

[krassowski]

Hi @glutanimate, thank you for explaining the situation very clearly. I think that you are right about the use of anki.hooks.wrap. It might be trivial to fix, though I am not 100% sure.

My guess/hope is that this might be simply a legacy thing - while there were many limitations of the wrap approach when I was writing this code, I guess that a lot has changed since the initial release 4 years ago and it could be easily implemented now.

I will be happy to work on it later on (say October) or merge a PR earlier.

[krassowski]

Thinking more about it, the NM has quite a unique option to turn itself on and off. I understand that with wrap, it is not possible to unwrap the NM code from any function and a custom logic would be needed in each patched function; this might be resonably easy to implement, or it might not. There is also a question of constants (like strings) which are patched - if I remember correctly these cannot be wrapped.

[krassowski]

Wait, a minute, the example that you have pointed to... it uses @wraps decorator, which leads us to

Anki-Night-Mode/night_mode/internals.py

Lines 225 to 242 in b846545

	if hasattr(attr, 'wraps'):
	if not target:
	raise Exception(f'Asked to wrap "{key}" but target of {name} not defined')
	original = getattr(target, key)
	if type(original) is MethodType:
	original = original.__func__
	new = wrap(original, callback_maker(attr), attr.position)
	# for classes, just add the new function, it will be bound later,
	# but instances need some more work: we need to bind!
	if not isclass(target):
	new = MethodType(new, target)
	cls.replacements[key] = new

where wrap:

Anki-Night-Mode/night_mode/internals.py

Line 235 in b846545

new = wrap(original, callback_maker(attr), attr.position)

is anki.hooks.wrap. It would seem that the problem might be somewhere else...

So using (and not using) wrap is kind-of irrelevant, the important thing is that the result of wrap or other replacement function (in terminology of NM) is not assigned back to the original class immediately. Therefore, if the PD does:

def onProfileLoaded():
    """Monkey-patch Reviewer delayed in order to counteract bad practices
    in other add-ons that overwrite revHtml and _linkHandler in their
    entirety"""
    Reviewer.revHtml = wrap(Reviewer.revHtml, onRevHtml, "around")
    Reviewer._linkHandler = wrap(Reviewer._linkHandler, linkHandler, "around")

then the Reviewer.revHtml (the one on the right-hand side) is the old method (not wrapped by NM) and it gets lost as NM has no idea about the wrap. So yes, currently the only way to make it work is to ensure that NM runs first or last (depending on if looking at the load time or registration time)...

Potential solution

If I could wrap the anki.hooks.wrap function, I could listen for the wraps and apply those to both original and night-mode copies of the wrapped method; this can be done by monkey-patching anki.hooks. While normally such an action is not sustainable in the long-term, this is a public API which if changed will break other add-ons anyway. This might be easier to do than re-wiring the current NM implementation. Pseudocode:

import anki.hooks

wrap = anki.hooks.wrap

def nm_wrap_spy(old, new, pos='after'):
    wrap_replacements(old, new, pos)
    return wrap(old, new, pos)

def wrap_replacements(old, new, pos):
    method_name = old.__name__
    # a tuple with an id of a method, e.g. (Reviewer, 'revHtml');
    # note: using object (method) reference would not work,
    # as it changes with the subsequent wrap operations
    key = (type(old), method_name)
    if key in StylerMetaclass.replacement_store_by_method:
        replacement_store = StylerMetaclass.replacement_store_by_method[key]
        wrapped_with_nm = replacement_store[method_name]
        replacement_store[method_name] = wrap(wrapped_with_nm, new, pos)

anki.hooks = nm_wrap_spy

plus some code to define StylerMetaclass.replacement_store_by_method in the metaclass.

Does it make sense to you @glutanimate, or do I miss something important here?

Edit: or, maybe, delaying the wrap until the NM switch could work - though I am sceptical of this one, as we would only get more unpredictable race conditions at activation.