This should fix #808.
The approach here is to try and have the Python callback return value be used as the C one is. I'm not totally sure how compatible that is, but IIUC if a Python function doesn't explicitly return it returns None, which should (I guess) fail the `PyObject_IsTrue()` check.
I did *not* test this *at all*: I didn't even try to build it (thanks to a crappy Internet connection tonight, I didn't wait long enough to fetch the dependencies). This PR should be reviewed as what it is: an attempt at a fix by a guy that doesn't know neither the API nor the plugin :)
However, this should also fix a leak as the return value should likely be `DECREF`'d in any case. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany-plugins/pull/809
-- Commit Summary --
* geanypy: Use Python callback return value for keybindings return value
-- File Changes --
M geanypy/src/geanypy-keybindings.c (8)
-- Patch Links --
https://github.com/geany/geany-plugins/pull/809.patch https://github.com/geany/geany-plugins/pull/809.diff
b4n commented on this pull request.
Py_DECREF(args);
+ ret = (PyObject_IsTrue(py_ret) == 1);
The `==1` is to catch the `-1` on error, which should probably lead to returning `FALSE` rather than `TRUE`.
I haven't tested yet (on Windows ATM), but this is exactly what I was thinking to do.
The only question is whether the semantics make any sense, as in whether returning nothing in the Python function should default to returning `FALSE` to Geany. The [API Reference](https://www.geany.org/manual/reference/keybindings_8h.html#afb2861d240a29818...) makes it sound like this is not the normal case.
It may be useful in either case to improve [the doctstrings](https://github.com/geany/geany-plugins/pull/809/files#diff-8c19fc53e5ac66527...) to clarify this, as a separate change if need be.
The only question is whether the semantics make any sense, as in whether returning nothing in the Python function should default to returning `FALSE` to Geany. The [API Reference](https://www.geany.org/manual/reference/keybindings_8h.html#afb2861d240a29818...) makes it sound like this is not the normal case.
It's a good question, but it will always be better than the current situation that returns a more or less random value -- I'm not sure if it's undefined or implementation defined behavior, but GCC seems to return 0. In practice, this seems to lead to no change if the Python function doesn't return, but allows it to return a value that will be passed along.
If you want returning `True` as the default case, I'm not sure what the best solution is but maybe simply assuming `None` means *"didn't return a value"* might be good enough, as it's the default return value, and not a valid value for an explicit return.
It may be useful in either case to improve [the doctstrings](https://github.com/geany/geany-plugins/pull/809/files#diff-8c19fc53e5ac66527...) to clarify this, as a separate change if need be.
Yep, definitely.
In practice, this seems to lead to no change if the Python function doesn't return, but allows it to return a value that will be passed along.
Yeah, I don't mind if this gets merged as is (once tested), and improve the other stuff later. I'd be interested to hear @kugel-'s thoughts since I think he wrote this code, or at least looked at it more recently than me I think.
@b4n @codebrainz I'd volunteer to test it, but not sure about the scenario. Can you give some short list?
@frlan The change here allows a GeanyPy plugin to tell Geany it *did* handle a keybinding, preventing any other handler using the same binding to run.
The problem is kinda tricky in real situation because this feature is more or less broken in Geany, as there's no real way to tell which handler will be called first, so it only works if all handlers on the same binding are well-behaved.
Anyway, to test, for example make a GeanyPy plugin with 2 binding handlers that do something visible (log a message or something). Then, bind those both to the same key combo, and see they both run. Now, try and follow Geany API to return `True` in the handlers, which should prevent the other ones from running. Before this patch, both still run. After, only the first one should (given it returns `True`).
github-comments@lists.geany.org