Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update bindings to QtAds 4.0.1 #6

Merged
merged 1 commit into from
Mar 30, 2023
Merged

Conversation

n-elie
Copy link

@n-elie n-elie commented Mar 27, 2023

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

@mliberty1
Copy link

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
Copy link
Owner

mborgerson commented Mar 30, 2023

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
Copy link
Owner

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
Copy link
Author

n-elie commented Mar 30, 2023

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
Copy link
Owner

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.

@mborgerson mborgerson merged commit c490f7f into mborgerson:main Mar 30, 2023
@mliberty1
Copy link

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
Copy link
Owner

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants