Page MenuHomePhabricator

DeepCat Refactoring
Closed, ResolvedPublic


Make sure DeepCat is always only searching once
If DeepCat was initialized multiple times (e.g. loaded globally + local checkmark) it would currently add the starting functions multiple times to the HTML-elements on the page. This would mean that the category graph is search multiple times for the same request.

Look at mw.loader.state or set it unconditionally to ready
don’t evaluate mw.libs.deepCat in such a manner, since it might be used for other things in the future

JS refactoring for DeepCat

  • Currently, DeepCat’s code uses local functions before they have been declared. Therefore it might be useful to separate cleanly between a declarative part on the top and and an executive part towards the end.
  • the unnamed executive function should be moved towards the very end of the gadget and be named, which would lead to a better understanding and reduce the risk of initializing multiple times
  • „Ein Besuch auf unter verschärfteren Bedingungen würde sich lohnen.“
  • If mw.Api is really only used for messages, use mediawiki.api.messages instead (shorter and more elegant)

Always check if libraries are loaded
DeepCat is supposed to be usable by other wikis, too (see the 11 global usages). However, in other wikis the libraries which are provided through the declaration of dependencies might not be reliably present. Therefore, please always check that the library really exists[1]

[1] more as to why:
es darf sich nicht einfach blind darauf verlassen werden, dass in einer Gadgets-Deklaration auf dem lokalen Wiki schon alles irgendwie geregelt wäre, zumindest dann, nachdem der Benutzer die Seite angezeigt bekäme und eine Suche interaktiv eingetippt habe; der Start ist ja wohl auch über eine Cirrus-URL möglich??????? Was dann auch auf standardmäßige Mobilansichten auszudehnen wäre.

Use LocalStorage instead of Cookies for DeepCat
Cookies are fully send to the server whenever any page is opened. However DeepCat cookies are not needed on most pages.
Therefore LocalStorage or, if possible, sessionStorage might be a better way to go

This is a translation of part of a comment by @PerfektesChaos: The other part of the comment can be found T142718

Event Timeline

WMDE-leszek triaged this task as Medium priority.
WMDE-leszek moved this task from Backlog to Doing on the TCB-Team-Sprint-2016-08-11 board.

I will submit some details in loose order here.

  • Please note that I only read the JS code; I never started the gadget yet.

Shielding against multiple loading

Once a gadget code has been loaded, by Greasemonkey or global user script, further loading should have no consequences. Especially the code must not equip the HTML document with multiple event triggering for the same elements.

See pageLinkHelper as an example. In the very last lines there are the only executable statements.

When called on autorun (loading), it is checked whether the gadget definition is already ready.

  • If not yet, first task is to set the state to be ready, now blocking any subsequent loading.
    • Then proceed.
  • If already ready do nothing more.
    • If the local gadget definition is executed a moment later, the gadget is known to be done already and will be skipped. That is the advantage of loader.state compared to some internal memorizing like mw.libs.DeepCat.loaded.

Note that the version identification of the code in effect is recorded for the local functions which are really used.

  • That is useful if development and debugging versions might be loaded in advance, overriding the regular components requested later.
  • Any subsequent loading would not influence anything. Actually, nothing will be executed outside local scope.

Physical order of declarations

See pageLinkHelper as of previous section.

  • There is some local initalization of the framework done in the first lines.
  • Then, a pile of local function declarations follows.
  • In the very last lines the one and only executable statement block triggers autorun: local function fire();
  • No function is ever called before it has been declared physically.

JavaScript permits “hoisting” but that might cause a lot of trouble on scopes.

  • /* jshint latedef:true */ will issue a warning on that.

Treat location of CSS as an implementation secret

  • The URL of the CSS resource might change one day.
  • Every global invocation would need to adopt this change, avoiding forwarder loads by @import (305 moved).
  • Loading of the CSS resource, if really needed, should be done within the JS code which is maintained.

Apparently the CSS styles are required once in 3,000 or 10,000 pages visited by the user.

You might try to turn some of the minor issues into inline style attributes of generated elements, especially if it is not expected that users are changing decoration.

  • The major stylish effect is on first instructions, guidelines etc. (“hints”) with larger textual components.
  • Thus seem to be required only if users did not decide to hide them and apparently only after a search field received focus the first time. These conditions may be checked in advance and CSS resource page will be lazy loaded then and only then.

Which throbber image is used when and how a bg image needs to be configured should be set directly by JS code, and will be frozen for ages.

  • Users are not supposed to create their own throbber, but they might assign one by utilizing the additional class selectors.

Use LocalStorage for user config rather than cookie

See also: T143777

Cookies will be transferred back and forth to the server with every page view.

  • Nowadays, cookies are used only if a server will evaluate that information.
  • If not, and the server does not know this gadget, WebStorage is preferred.

Establish a JSON object with all user config to be saved:

  • LocalStorage
      • DeepCat
        • JSON {
          • "hideHints":true
        • }

Please do not create a single item for each single feature.

  • I get mad when I am surveying my WebStorage.

There was already a jstorage present, but has been deprecated recently.

  • Basically a DeepCat could be added there.
  • However, I would recommend to keep independent from decisions outside.

LocalStorage is available on all supported browsers. Users might decide to block them for all domains; so they should learn to make exceptions for wikis. Otherwise they get drawbacks in various fields.

There is a package available which asserts availability and access permission on client side.

This works for both registered and anonymous users and for all accounts sharing that browser profile. There is another approach which will memorize for wiki project accounts of registered users across browsers, but oversized for this purpose.

  • Short term preferences might be stored in SessionStorage, see later.

Ensure loading of all modules

See again pageLinkHelper as an example. In the very last lines there are .using requests for the modules inside the current implementation.

  • If already loaded by local gadgets-definition, then their presence is just ticked off and callback will proceed immediately.
  • If called by greasemonkey or meta:global some might not been accessed now. Then request loading and wait.
  • If gadgets-definition of local project has not been updated yet, but the most recent .js implementation requires more modules now, those will be loaded first.
  • Basically all modules requested by gadgets-definition are loaded by one single call which optimizes network traffic. However, nobody must rely on their presence.