search

mborgerson / pyside6_qtads

Python bindings to Qt Advanced Docking System for PySide6
Other
22 stars 9 forks source link

Update bindings to QtAds 4.0.1 #6

Closed n-elie closed 1 year ago

n-elie commented 1 year ago

This will hopefully fix crash described in https://github.com/githubuser0xFFFF/Qt-Advanced-Docking-System/issues/494

mliberty1 commented 1 year ago

Thanks @n-elie - I was not aware of that ADS issue. I have seen crashes that seem to be this issue in our Joulescope UI. I was operating under the assumption that it was our C / python binding code that was causing this. Thanks for submitting this PR and saving me time to track this one down!

Thanks @mborgerson for maintaining these python bindings! We moved from Qt's docking to ADS for our upcoming 1.0.x Joulescope UI release, and the UI is much better for it!

mborgerson commented 1 year ago

Thanks @n-elie - I was not aware of that ADS issue. I have seen crashes that seem to be this issue in our Joulescope UI. I was operating under the assumption that it was our C / python binding code that was causing this. Thanks for submitting this PR and saving me time to track this one down!

Thanks @mborgerson for maintaining these python bindings! We moved from Qt's docking to ADS for our upcoming 1.0.x Joulescope UI release, and the UI is much better for it!

@mliberty1 Do you have the required version of PySide6 installed when you observed the crash with the latest version of PySide6-QtAds 4.0.1.1 wheels? The current release of PySide6-QtAds wheels require specifically PySide6-Essentials==6.4.2, and soon will be updated and will require 6.4.3. The app I put these bindings together for (https://github.com/angr/angr-management) is tested to run on all platforms, so I suspect this is the root cause of your issues. Glad to hear another Python app is benefiting from the bindings!

mborgerson commented 1 year ago

This will hopefully fix crash described in githubuser0xFFFF/Qt-Advanced-Docking-System#494

@n-elie Thanks for the update. Did you observe the issue for yourself? If so, can you check my comment above regarding the crash you were seeing? I'd like confirmation before updating

n-elie commented 1 year ago

Yes, I observed the issue. I don't use the wheel from PyPI but the conda package. With the current version of bindings, the simple example looks like this and crash on exit or if I try to detach the docked window: image

Looks like this after update and no crash: image

I suspect the addDockWidget function from CDockManager to be responsible for this behavior as QtAds 4.0 added an extra int parameter.

mborgerson commented 1 year ago

I suspect the addDockWidget function from CDockManager to be responsible for this behavior as QtAds 4.0 added an extra int parameter.

@n-elie: I see, good catch! Annoyingly the build did not fail when the binding schema diverted from the C++ API (#9), and because I don't exercise this path in my use case it went unnoticed. Hopefully we can fix #9 and #8 soon to prevent this from happening again.

mliberty1 commented 1 year ago

Do you have the required version of PySide6 installed when you observed the crash with the latest version of PySide6-QtAds 4.0.1.1 wheels? The current release of PySide6-QtAds wheels require specifically PySide6-Essentials==6.4.2, and soon will be updated and will require 6.4.3.

@mborgerson - Yes, I do have the correct versions, at least on my primary Win 11 x64 machine. Here is what pip says:

PySide6==6.4.2
PySide6-Addons==6.4.2
PySide6-Essentials==6.4.2
PySide6-QtAds==4.0.1.1

I also checked the mac and linux dev platforms, and they look correct, too.

I just did pip3 install -U PySide6-QtAds PySide6-Addons PySide6-Essentials PySide6, and now I have:

PySide6==6.4.3
PySide6-Addons==6.4.3
PySide6-Essentials==6.4.3
PySide6-QtAds==4.0.1.2

If I see more issues when moving, undocking & docking widgets, I'll report back and see if I can help track down the issue.

The angr Management UI looks nice! I need to add some pictures to the Joulescope UI repo. You check out the forum for pictures for now. We went all in on pubsub for internal UI communication for this version. I still need to get around to adding support for external plugins, which I see you already have in angr Management!

mborgerson commented 1 year ago

@mliberty1 Thanks for checking the versions! I believe the mentioned issue should be fixed now that this PR has been merged, with thanks to @n-elie! Feel free to create an issue here if you run into another major crashing issue.

The angr Management UI looks nice! I need to add some pictures to the Joulescope UI repo. You check out the forum for pictures for now.

Thanks. The Joulescope UI also looks good, nice work! I'm glad to see that you've made it an open source project.