Add support for changing hook order #4186

kantlivelong · 2021-07-16T13:23:07Z

What does this PR do and why is it necessary?

Allows the user to override the hook order value for plugins. Useful in cases where one plugin hook might need to be executed before another but they have conflicting order values.

How was it tested? How can it be tested by the reviewer?

Config:
Each plugin can have an additional key called '_hook_order_overrides' which contains a subkey for each hook ID.

Example:

  softwareupdate:
    _hook_order_overrides:
      octoprint.access.permissions: 11
      octoprint.events.register_custom_events: 99

UI:
Settings -> Plugin Manager -> Hook Order (See screenshot)

Any background context you want to provide?

What are the relevant tickets if any?

Screenshots

Further notes

I'm sure the JS side of things can be optimized better. Pointers appreciated.

I wasn't sure the best way to display what the computed default was in the UI. I just labeled it as "Definition". Originally I called it "Original" and displayed the order number instead of the value or "Default". Thoughts?

I used _hook_order_overrides as the config key name but if there is something better/shorter you'd prefer let me know. Same goes for the blueprint path or anything else for the matter.

There are two(2) TODO lines which maybe you can answer:

foosel

Got a bunch of JS refactoring suggestions and other points.

foosel · 2021-07-19T12:24:40Z

src/octoprint/plugin/core.py

@@ -1676,6 +1677,13 @@ def _activate_plugin(self, name, plugin):
                )
                continue

+            if not order:
+                order = 1000


Should be a constant, e.g. DEFAULT_ORDER

foosel · 2021-07-19T12:24:58Z

src/octoprint/plugin/core.py

@@ -1719,6 +1727,9 @@ def _deactivate_plugin(self, name, plugin):
                )
                continue

+            if not order:
+                order = 1000


foosel · 2021-07-19T12:25:04Z

src/octoprint/plugin/core.py

@@ -2167,11 +2178,12 @@ def sort_func(impl):
                                impl[0], sorting_context
                            )
                        )
-                        sorting_value = None
+                        sorting_value = 1000


foosel · 2021-07-19T12:25:12Z

src/octoprint/plugin/core.py

-                        sorting_value = None
+                        sorting_value = 1000
+                else:
+                    sorting_value = 1000


foosel · 2021-07-19T12:27:10Z

src/octoprint/plugins/pluginmanager/__init__.py

+            for hook in data["hooks"]:
+                # TODO: Force really needed?
+                self._settings.global_set_int(
+                    ["plugins", hook["id"], "_hook_order_overrides", hook["hook"]],


Sanity check that id, hook and value are actually set on hook.

foosel · 2021-07-19T12:29:10Z

src/octoprint/plugins/pluginmanager/__init__.py

+        for i in range(len(self._plugin_manager._plugin_hooks[hook])):
+            if self._plugin_manager._plugin_hooks[hook][i][1] == plugin:
+                self._plugin_manager._plugin_hooks[hook][i] = (
+                    value,
+                    plugin,
+                    self._plugin_manager._plugin_hooks[hook],
+                )
+                return


This really feels like it should be added to the core plugin manager class instead, as it heavily interacts with internal data structures

foosel · 2021-07-19T12:29:37Z

src/octoprint/plugins/pluginmanager/__init__.py

+        for hook_name, hooks in self._plugin_manager._plugin_hooks.items():
+            order = dict(name=hook_name, hooks=[])
+            for order_current, plugin_id, _hook in hooks:
+                plugin_name = self._plugin_manager.plugins[plugin_id].name
+
+                _, order_original = self._plugin_manager._get_callback_and_order(
+                    self._plugin_manager.plugins[plugin_id].hooks[hook_name]
+                )
+
+                order["hooks"].append(
+                    dict(
+                        id=plugin_id,
+                        name=plugin_name,
+                        order_current=order_current,
+                        order_original=order_original,
+                    )
+                )
+            result.append(order)


This should prolly also be moved into the core plugin manager itself.

foosel · 2021-07-19T12:31:55Z

src/octoprint/plugins/pluginmanager/static/js/pluginmanager.js

+                .fail(function () {
+                    self.requestError(true);
+                    deferred.reject();
+                })
+                .done(function (data) {
+                    self.requestError(false);
+                    self.fromHookOrderResponse(data.hook_order);
+                    deferred.resolveWith(data);
+                });


Really just for reasons of consistency: Swap order, first the success, then the failure case.

foosel · 2021-07-19T12:36:02Z

src/octoprint/plugins/pluginmanager/static/js/pluginmanager.js

+            for (i = 0; i < data.length; i++) {
+                for (ii = 0; ii < data[i]["hooks"].length; ii++) {
+                    data[i]["hooks"][ii]["orderStaged"] = false;
+                    data[i]["hooks"][ii]["orderTemp"] = false;
+                }
+            }


_.each(data, function(entry) { if (!entry || !entry.hooks) return; _.each(entry.hooks, function(hook) { hook["orderStaged"] = false; hook["orderTemp"] = false; } }

foosel · 2021-07-19T12:48:43Z

src/octoprint/plugins/pluginmanager/static/js/pluginmanager.js

+            for (var k = 0; k < self.hookOrder().length; k++) {
+                var hookGroup = self.hookOrder()[k]["name"]();
+
+                for (var kk = 0; kk < self.hookOrder()[k]["hooks"]().length; kk++) {
+                    var hook = self.hookOrder()[k]["hooks"]()[kk];
+                    if (hook["orderStaged"]()) {
+                        changes.push({
+                            hook: hookGroup,
+                            id: hook["id"](),
+                            value: hook["orderStaged"]()
+                        });
+                    }
+                }
+            }


_.each(self.hookOrder(), function (entry) { var hookGroup = entry.name(); _.each(entry.hooks(), function (hook) { if (hook.orderStaged()) { changes.push({ hook: hookGroup, id: hook.id(), value: hook.orderStaged() }); }); }); });

cp2004 · 2021-07-19T15:19:35Z

I wonder if this could do with a short sentence at the top explaining what it is for. For a plugin developer, it is clear as this is what we use to write the plugins. However, I suspect for many people this will be the first time they have heard of these 'hooks', and they don't have any idea what it is for. Maybe needs something that recommends leaving it as is unless advised/running into problems - this is less necessary, maybe if there's an explanation it would be enough. Don't want to fill up the UI with useless information :)

foosel · 2021-07-19T15:22:46Z

That's actually a good point. Should it maybe even be hidden behind a config flag?

foosel · 2021-07-19T15:32:30Z

Or - another thought - do we maybe need something like "Enable developer options" or "Enable advanced options" for the settings in general?

kantlivelong · 2021-07-19T20:23:12Z

Or - another thought - do we maybe need something like "Enable developer options" or "Enable advanced options" for the settings in general?

Not a bad idea. IMO this would be considered an advanced option rather than dev. I could also break this out into it's own plugin instead of in Plugin Manager so that it can be disabled by default.

foosel · 2021-07-27T14:40:00Z

Advanced kinda settings are definitely something that needs some work first... the question is, should we target 1.7.0 for this here or not?

kantlivelong · 2021-08-02T15:28:29Z

I think 1.7.0 makes sense. dev branch?

Also, do you have any input on the Further notes section of the PR?

foosel · 2021-08-03T09:45:50Z

1.7.0 is feature frozen as of yesterday (I hope to push out a first RC tomorrow, would have been today if I didn't have a doc appointment for my shoulder right in the middle that makes it difficult to plan around), so this will have to wait until 1.8.0, too many open points still and decisions to make regarding general approach for dev side stuff.

Will also give me time to take another look at the TODOs you marked up.

foosel · 2021-10-11T08:15:59Z

@kantlivelong poke :)

kantlivelong · 2021-10-11T14:06:04Z

@foosel OWW MY EYES!

I've been holding out for your comments on the Further Notes section. 😉

foosel · 2021-11-10T11:20:01Z

Sorry, that first got overlooked before my poke, and then needed some time after.

I'm sure the JS side of things can be optimized better. Pointers appreciated.

See the initial review.

I wasn't sure the best way to display what the computed default was in the UI. I just labeled it as "Definition". Originally I called it "Original" and displayed the order number instead of the value or "Default". Thoughts?

I'd seriously just got with "Computed" and displaying the original order number. That should transfer what this is best.

I used _hook_order_overrides as the config key name but if there is something better/shorter you'd prefer let me know. Same goes for the blueprint path or anything else for the matter.

Looking at the settings structure I think this should rather be plugins._hook_order_overrides.<plugin_id>.<hook> instead of plugins.<plugin_id>._hook_order_overrides.<hook>. That would match the use of this namespace for plugins._disabled and plugins._forcedCompatible. Naming wise, most of the settings use camelCase instead of snake_case (there are some exceptions I just saw, but they are rare). So it should probably rather be hookOrderOverrides. We could say that these are overrides is implicit, and thus we arrived at hookOrder.

There are two(2) TODO lines which maybe you can answer:
1. https://github.com/kantlivelong/OctoPrint/blob/08a26d76bcd4f5a957658211e5cdb6cbb34e8079/src/octoprint/plugins/pluginmanager/static/js/pluginmanager.js#L584
// TODO: Could be worth filtering this out as sorting certain hooks makes no difference.

You mean filtering by hook type? I'd honestly just say, treat them all the same, the less magic the better.

2. https://github.com/kantlivelong/OctoPrint/blob/08a26d76bcd4f5a957658211e5cdb6cbb34e8079/src/octoprint/plugins/pluginmanager/__init__.py#L717

# TODO: Force really needed?

Good question. Trying to remember when force was necessary, and IIRC it was when you are writing values directly that don't have a default (so not setting a whole dict on a key that has an empty dict as default, but traversing into the dict). In your case here, the path you are setting doesn't exist in the defaults, so a force is necessary.

foosel · 2022-02-17T09:15:37Z

@kantlivelong poke ;) I'm looking into starting to wrap up 1.8.0, so unless you do a race to the finish line here, this might get delayed into 1.9.0. No harm done, just want to give you a heads-up.

Default hook value changed from None to 1000

kantlivelong · 2022-02-17T14:52:27Z

@foosel Thanks for the nudge. For now I think I'll just submit this without the UI portion. I've kept a copy of the UI branch at https://github.com/kantlivelong/OctoPrint/tree/hook_order_overrides_with_UI should I revisit it later or someone else want to take it on.

I've also adjusted according to your feedback 😄

* fetch settings during platform initialization instead of in plugin core * rename from hook order to sorting order as it's also used for the sorting context of implementations * make sure settings are initialized empty in defaults

foosel

Added some missing docs and slightly refactored things to better match existing stuff.

kantlivelong requested a review from foosel July 16, 2021 13:23

github-actions bot added the approved Issue has been approved by the bot or manually for further processing label Jul 16, 2021

foosel requested changes Jul 19, 2021

View reviewed changes

foosel added this to the 1.8.0 milestone Aug 3, 2021

foosel added needs some work There are some things to do before this PR can be merged targets maintenance The PR targets the maintenance branch labels Sep 9, 2021

kantlivelong force-pushed the hook_order_overrides branch from d584257 to c77c3e0 Compare February 17, 2022 14:26

Add support for changing hook order

66d0f30

Default hook value changed from None to 1000

kantlivelong force-pushed the hook_order_overrides branch from c77c3e0 to 66d0f30 Compare February 17, 2022 14:48

foosel added 3 commits February 22, 2022 10:37

♻️ Refactor sorting override logic

a92eb6b

* fetch settings during platform initialization instead of in plugin core * rename from hook order to sorting order as it's also used for the sorting context of implementations * make sure settings are initialized empty in defaults

📝 Add missing docs for sorting order overrides

d7293d3

✨ Fully support reordering of implementations too

a2982b9

foosel approved these changes Feb 22, 2022

View reviewed changes

foosel merged commit 80ffa88 into OctoPrint:maintenance Feb 22, 2022

foosel removed the needs some work There are some things to do before this PR can be merged label Feb 22, 2022

github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for changing hook order #4186

Add support for changing hook order #4186

kantlivelong commented Jul 16, 2021 •

edited

Loading

foosel left a comment

foosel Jul 19, 2021

foosel Jul 19, 2021

foosel Jul 19, 2021

foosel Jul 19, 2021

foosel Jul 19, 2021

foosel Jul 19, 2021

foosel Jul 19, 2021

foosel Jul 19, 2021

foosel Jul 19, 2021

foosel Jul 19, 2021

cp2004 commented Jul 19, 2021

foosel commented Jul 19, 2021

foosel commented Jul 19, 2021

kantlivelong commented Jul 19, 2021

foosel commented Jul 27, 2021

kantlivelong commented Aug 2, 2021

foosel commented Aug 3, 2021

foosel commented Oct 11, 2021

kantlivelong commented Oct 11, 2021

foosel commented Nov 10, 2021

foosel commented Feb 17, 2022

kantlivelong commented Feb 17, 2022

foosel left a comment

Add support for changing hook order #4186

Add support for changing hook order #4186

Conversation

kantlivelong commented Jul 16, 2021 • edited Loading

What does this PR do and why is it necessary?

How was it tested? How can it be tested by the reviewer?

Any background context you want to provide?

What are the relevant tickets if any?

Screenshots

Further notes

foosel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cp2004 commented Jul 19, 2021

foosel commented Jul 19, 2021

foosel commented Jul 19, 2021

kantlivelong commented Jul 19, 2021

foosel commented Jul 27, 2021

kantlivelong commented Aug 2, 2021

foosel commented Aug 3, 2021

foosel commented Oct 11, 2021

kantlivelong commented Oct 11, 2021

foosel commented Nov 10, 2021

foosel commented Feb 17, 2022

kantlivelong commented Feb 17, 2022

foosel left a comment

Choose a reason for hiding this comment

kantlivelong commented Jul 16, 2021 •

edited

Loading