ScratchAddons icon indicating copy to clipboard operation
ScratchAddons copied to clipboard

Require waitForElement optimizations for userscripts that run in project pages

Open WorldLanguages opened this issue 2 years ago • 8 comments

Setting this issue as a reminder to research this and think of ideas. Most addons apply these optimizations but I will check if some of them don't.

WorldLanguages avatar Jun 01 '23 20:06 WorldLanguages

We should also consider a re-thought of the waitForElement options API, which might be confusing for new contributors. Possibly {markAsSeen:true} should be the default?

WorldLanguages avatar Jun 01 '23 20:06 WorldLanguages

Some suggestions:

  • Remove the markAsSeen option, default it to true, and encourage the use of addon.tab.waitForElementOnce (doesn't exist yet) instead.
  • Avoid reusing the same set of reduxEvents each time. Give a name to each set and expose them as addon.tab.waitForElement.EDITOR_DOM_INITIALIZED, for example.

Regarding project pages:

  • Each waitForElement call should indicate whether it plans to be executed in a project page (indicating true or false would be mandatory).
  • If above is true, then each call should indicate if it is intended for the project page, the project editor, or both. This replaces isPlayerOnly optimizations.
  • Optimizations such as "currently in the code/costume/sound " (editorTab.activeTabIndex) will continue to look the same, but maybe they could also be made more expressive. We should consider this, as there's 14 files that check if the tab index matches exactly a number.

WorldLanguages avatar Aug 20 '23 19:08 WorldLanguages

Related comment: https://github.com/ScratchAddons/ScratchAddons/pull/6695#issuecomment-1734230346 - we could possibly kill the while(true) pattern in favor of events/callbacks of some sort

WorldLanguages avatar Sep 25 '23 18:09 WorldLanguages

Related comment 2: https://github.com/ScratchAddons/ScratchAddons/issues/6880#issuecomment-1823304648 - careful with waitForElement:false, as an element might exist but possibly not all of its children. Probably a good idea to ignore an element unless all their children exist - unless if waitForElementOnce("body") ?

WorldLanguages avatar Nov 26 '23 15:11 WorldLanguages

Related comment 3: https://github.com/ScratchAddons/ScratchAddons/issues/6956#issuecomment-1858406128 - worth running all selectors again if window has been resized

WorldLanguages avatar Dec 15 '23 19:12 WorldLanguages

I'm thinking, maybe we could consider the editor and project pages completely separate concepts - and make it so that every userscript that wants to run on /projects/* needs to explicitly ask for it in the addon manifest. The "projects" match would cease to exist, and no amount of asterisks would be enough to run in project pages.

WorldLanguages avatar Feb 18 '24 18:02 WorldLanguages

Also related: If a waitForElement call includes reduxEvents, it should still run the selector for the first time, right? Looks like that is not the case, according to this addon: https://github.com/ScratchAddons/ScratchAddons/blob/master/addons/disable-auto-save/userscript.js#L10

WorldLanguages avatar Feb 25 '24 22:02 WorldLanguages

Also related: If a waitForElement call includes reduxEvents, it should still run the selector for the first time, right? Looks like that is not the case, according to this addon: https://github.com/ScratchAddons/ScratchAddons/blob/master/addons/disable-auto-save/userscript.js#L10

This code is wrong - if the addon is dynamically enabled, addListener is called twice.

mxmou avatar Feb 26 '24 17:02 mxmou