search

nucleic / enaml

Declarative User Interfaces for Python
http://enaml.readthedocs.io/en/latest/
Other
1.53k stars 130 forks source link

Remove signaling/callableref/weakmethod #411

Closed MatthieuDartiailh closed 4 years ago

MatthieuDartiailh commented 4 years ago

With the release of 0.11.0 I realized that signaling is broken. After a bit of investigation (and a lot of pain trying to fix it), I noticed that it has been true since 0.10.1 (basically it never worked on Python 3). Given that this was never reported before (and it has been more than two years now), I am in favor of simply removing signaling and the related callableref and weakmethod.

@sccolbert I remember you mentioning somebody having a use case for it. Could you ping them ? I can try to fix the situation but this is nasty part of C that I would be happy to just drop.

tstordyallison commented 4 years ago

We do use this for our internal widgets (@ JPM). I would guess it was added by @sccolbert for this reason...

We've already fixed up some ref counting issues with the version in 10.x (that we merged with our original fork in order to get Python 3 support, thanks for that!). So I wouldn't be too surprised if things aren't quite right. There aren't any tests in the repo for it and we picked it up on the CI for our widgets, not enaml itself.

At this point - for our use cases - I'd be quite happy to pull in signaling/callableref/weakmethod into the C++ code base that sits along side our internal widgets and maintain it from there. We are on 2.7 + 3.7 in production today, with 2.7 going away towards the end of the year. I expect we'll start to move to 3.8 sometime thereafter and we'll fix up whatever we need to then.

Less C++ to maintain needlessly in enaml.

MatthieuDartiailh commented 4 years ago

Thanks for the feedback @tstordyallison ! Could you describe what is the added value of signaling in your case ? I would be happy to know because I have never come across a use case that would require it. Also since you have a use case for it, it may have value for others (once properly tested and documented) so it may make sense to keep it. If you can provide the patch you applied to 0.10.x to get things to work I would be happy to apply it here (if this does not conflict with JP policies) and I can probably handle the cppy transition.

tstordyallison commented 4 years ago

https://github.com/nucleic/enaml/blob/56163cb545a0f6b8a0b47eedf74bdf5896ac4c0b/enaml/src/weakmethod.cpp#L77

Dug out the PR - there was only actually one change (though it was in pythonhelpers.h in our fork) to set takeref=true. This seems to be missing on master too (e.g. cppy::incref).

As for the usage, I'll have more of a dig when I'm properly logged in tomorrow - IIRC it was to ease communications from the inside of a UI toolkit agnostic C++ widget -> enaml (a bit like Qt signals).

MatthieuDartiailh commented 4 years ago

Thanks a lot ! I can confirm that this fixes the issue.

If you can provide me enough material to properly document this feature, I will close this PR and fix signaling in 0.11.1.

tstordyallison commented 4 years ago

So - the more I search around our codebase - the more I'm keen for this to stay in enaml if poss! We use it in a bit more than just the widget I had in mind (though not a crazy amount).

It's essentially a Qt-less signal. Same sort of API, but doesn't need Qt. As we still (though not for much longer) support wxWidgets, we use it there as a nice API for some of the widgets we have to work in both wxWidgets & Qt. It also works nicely in various headless tests where we don't always have or want the wxWidgets or Qt dependencies on the test grid (which reminds me that I have some headless testing code I promise I will contribute back one day!).

MatthieuDartiailh commented 4 years ago

Thanks for the feedback @tstordyallison . Since you provided what seems to be the last fix I needed to get everything working again, and since you have a valid use case I am fine keeping that functionality (but I may need a bit of time to polish everything). The idea of throwing it out was mostly motivated by the apparent lack of use (I wish you had reported the breakage earlier) and my frustration running after bugs in the C++ codebase. If you can contribute an example using signaling that would be great. And I am curious about headless testing !

MatthieuDartiailh commented 4 years ago

0.11.1 is now available on PyPI with the fix. Conda package should follow in a couple of days on conda-forge. Thanks @tstordyallison for providing the last piece required to fix this code and motivate the fact that we keep it. Please do not hesitate to report any issue you may find in the future.