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

Allow add-on authors to more easily extend Anki's web views #258

Closed
wants to merge 1 commit into from

Conversation

glutanimate
Copy link
Contributor

Extends all pertinent web views with runFilter hooks that enable add-on authors to easily prepend and append HTML to the body of the view, or add their own CSS or JS to the head element.

Also implements two runFilter hooks specific to the stats areas of the Overview and DeckBrowser, granting more control over these commonly modified parts of Anki's UI.

These changes should help alleviate some of the conflicts between add-ons that target the same HTML views, and also do away with some of the hacks that are currently in use to achieve similar
goals.


I've been meaning to submit something like this for a while now. What prompted me to finally work on these changes is an old issue that recently resurfaced in my efforts to refactor and port Review Heatmap to 2.1: Review Heatmap extends Anki's Overview and DeckBrowser screens with a custom element showcasing user activity over time. For the DeckBrowser screen this element is positioned right below the stats area. For the Overview screen it's right below the table element.

The main challenge I initially faced when modifying these parts of Anki's user interface was that they were also commonly targeted by other add-ons. So users installing an add-on like More Overview Stats would suddenly see their heatmap disappear, as the _table() function I was wrapping to add my heatmap would be overwritten by the other add-on. This forced me to move to a more hacky and unstable implementation in the form of completely overwriting Overview._renderPage() and Overview._body. To this day this is how Review Heatmap works on the Deck screen. Needless to say, this in turn has been the cause of many incompatibilities with other add-ons, e.g. Night Mode.

My first hope with this PR is that the changes will allow add-on authors to modify Anki's web elements less intrusively. My second hope is that it will make the web part of add-on development more accessible in general.

To test these changes in practice I've also compiled a quick proof-of-concept add-on that is available here.

Extends all pertinent web views with runFilter hooks that enable
add-on authors to easily prepend and append HTML to the body of the
view, or add their own CSS or JS to the head element.

Also implements two runFilter hooks specific to the stats areas
of the Overview and DeckBrowser, granting more control over these
commonly modified parts of Anki's UI.

These changes should help alleviate some of the conflicts between
add-ons that target the same HTML views, and also do away with
some of the hacks that are currently in use to achieve similar
goals.
@glutanimate
Copy link
Contributor Author

glutanimate commented Oct 9, 2018

Quick thought: In their current iteration the runFilter hooks rely on string concatenation, which from what I understand isn't as efficient in Python as using join() statements (especially for longer strings). With that in mind, and also looking at how the editor implements righttopbtns, do you think I should update the hooks to switch to a list of strings, instead (which add-on authors would be able to append to, just like the editor's buttons)?

@dae
Copy link
Member

dae commented Oct 10, 2018

How about instead of passing in a bunch of filter results each time, you add a filter=None argument. If a string like "preview" is provided, then stdHtml() calls something like body = runFilter(filter+"StdHtml", body), where the filter can prepend or append to body before returning it? That keeps the calling code cleaner and keeps the argument count down.

You could also add a filter for the head area, but are you aware of any add-ons doing things in head that couldn't also be accomplished by prepending to the body area?

%(stats)s in overview.py could be named something like %(extra)s - at the moment it feels like a solution specific to the heatmap add-on.

I doubt a few string concatenations will make a big difference to performance compared to disk I/O, but if you are thinking of changing it, now would be the time to do so :-)

@snowKNOWZ
Copy link

can someone please make an add on to ignore the space bar for the 2.1 version after you have answered the card... I like to use the space bar to scroll PLEASE

@glutanimate
Copy link
Contributor Author

Sorry for the lack of response, Damien. I'll try to come back to this soon. I'm currently on the last stretch of getting the Review Heatmap 2.1 port out and haven't had a lot of time outside of that.

@snowKNOWZ

Sorry, but this is not the right place to ask. Please start a thread here or here, instead.

@dae
Copy link
Member

dae commented Oct 25, 2018

No problem!

@krassowski
Copy link
Contributor

I gave it some thought today. I really like the idea of using filters to inject add-ons generated content, it would make theming and development so much easier. Also, I think that moving the filter calls to AnkiWebView.stdHtml (as suggested by @dae) would be better.

May I also suggest that we could have an additional argument in the init of AnkiWebView, which would determine the filter name? This would go along these lines:

class AnkiWebView(QWebEngineView):

    def __init__(self, parent=None, title="default"):
        QWebEngineView.__init__(self, parent=parent)
        self.title = title

Please compare it with the current hard-coded behaviour: https://github.com/dae/anki/blob/dd842873ccdf543ded6ccd45f5e3303d08393ea6/aqt/webview.py#L85-L89

This of course assumes changing all the relvant constructors from e.g.

self._previewWeb = AnkiWebView()

to:

self._previewWeb = AnkiWebView(title='previewWeb')

This might have an additional benefit for debugging in some strange situations (as it would be easier do identify the origin of a webview with a title).

I would run following filters, each time when self.title != 'default':

  1. suffix and prefix for appending custom content
  2. body for manipulation of the pre-generated content
  3. head: @dae, for me there is nothing that cannot be accomplished outside of the head. However, a frequent fear is that if we will link to on-disk/external CSS outside of the head (e.g in the bottom of the page) we can get a FOUC effect, especially detrimental to night mode users. I do not know if this will happen with the current web renderer, but it might be better to let developers put content inside of the head too. Another example would be an add-on enabling users to fetch some on-line content to show on cards; if the content download was to be started from the bottom, this will might lead to unintentional content jumping.

i wonder if it would be beneficial to have also a CSS filter, specifically for CSS additions...

@glutanimate IMO there is no need to worry about the string concatenation in there, it's completely negligible when compared to the cost of drawing the interface. Additionally, the cost of concatenations would be proprtional to the number of addons observing the filters - I doubt that there will be thousands of such add-ons any time soon ;)

@dae
Copy link
Member

dae commented Nov 25, 2018

A single argument that defines the title and filter at the same time sounds good to me.

If you're adding CSS to the top of the body I presume that will avoid a FOUC. I'd suggest we start with just a body filter and see how it goes, as it's easy to add more later, but hard to remove an extra filter once people start using it.

Also you mention "suffix and prefix" - perhaps I'm just misunderstanding you, but I'm suggesting a single filter that is passed the entire body - hooks can append to, prepend to, or modify the text as necessary.

@krassowski
Copy link
Contributor

Yes, you are right if we have the body filter we no longer need the suffix and prefix filters. My original, unwritten idea was that those variables are already there and that would make fewer add-on collisions:

Imagine an add-on which modifies text colors on cards; it would change only <font color="red"> to <font color="green">; now if I want to use <font color="red">add-on</font> in my add-on and append it by the body filter, it will be changed as well despite that the authors of both add-ons did not intend that originally. This is just a silly example but if an add-on starts messing with the body (or worse, with JS code in there) it could lead to more strange collisions. Of course, if I want my HTML to be modified by other add-ons I will append to the body and not to the suffix/prefix, but that's not always the case.

I agree with your point about keeping the number of filters low though.

@dae
Copy link
Member

dae commented Nov 26, 2018

I'm not sure separate prefix and suffix buffers really improves things that much, as it only helps for add-ons that are content to insert text at the very top or bottom of the body - it doesn't really improve things when multiple add-ons want to alter the stock body. For the latter cases I think we may still be better off with extra hooks to ease insertion of content at key points.

Ultimately there's only so much that can be done to improve things at the moment - all the string-based page building Anki does is a bit of a mess, and it needs an overhaul post 2.1.

@dae
Copy link
Member

dae commented Apr 29, 2019

Closing this for now - please feel free to create a new request if you decide to start working on this again.

@dae dae closed this Apr 29, 2019
glutanimate added a commit to glutanimate/anki that referenced this pull request Feb 6, 2020
In preparation of web content hooks

cf. ankitects#258 (comment)
@glutanimate
Copy link
Contributor Author

@krassowski: I've filed an updated PR in #439, picking up on some of the things we discussed in here. Just thought I'd let you know in case you want to take a look!

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.

4 participants