Fix crash when clicking outline #1315
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
connect_qtconsole's profile option was apparently deprecated in ipykernel 4.2.2 The commit is here: ipython/ipykernel@ef01782
On PyQt5 5.15.11 (the current latest), subElementRect (from qproxystyle) expects three arguments, not two. This isn't consistent with the other subElementRect functions, so I strongly suspect that the problem is somewhere upstream. I see this behavior in the docs and in the package itself, so I don't see a very good way around it. The bindings for subElementRect aren't consistent: ``` bindings/QtWidgets/qcommonstyle.sip 39: virtual QRect subElementRect(QStyle::SubElement r, const QStyleOption *opt, const QWidget *widget = 0) const; bindings/QtWidgets/qproxystyle.sip 41: virtual QRect subElementRect(QStyle::SubElement element, const QStyleOption *option, const QWidget *widget) const; bindings/QtWidgets/qstyle.sip 270: virtual QRect subElementRect(QStyle::SubElement subElement, const QStyleOption *option, const QWidget *widget = 0) const = 0; ``` (`widget` in `qproxystyle` should have a default value of `0`/`nullptr`, but it doesn't) This also shows up in the docs. These have a default: https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtwidgets/qcommonstyle.html##subElementRect https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtwidgets/qstyle.html##subElementRect but the one we're using here doesn't: https://www.riverbankcomputing.com/static/Docs/PyQt5/api/qtwidgets/qproxystyle.html##subElementRect Even if other versions have a default value, it should work fine as a positional argument, and `None` would be the default value anyway.
|
I managed to get another crash still based on this, so it's apparently not ready to merge quite yet. |
A few more places in my previous commit didn't break in my "test" project (presumably because they weren't being drawn), but after switching back to a real project, they broke again. This does more of the same thing (using `None` instead of relying on the default argument, which is missing)
This time I actually went through the docs and found all the elements in `qproxystyle` that didn't have a default value for the widget, which are ``` sizeFromContents subControlRect subElementRect ``` and then went through the entire Manuskript project with grep and made sure I didn't leave any more places that needed this change. I confirmed that I would also crash in the full screen editor -- I'm not sure how to get to the collapsibleGroupBox code within Manuskript itself, but from looking at the code, it's almost definitely the same issue.
It appears to be using a different type base QT object, so it doesn't cause a crash here. It wouldn't have hurt anything, but still better to not make a change when it's unnecessary
|
This is ready to be merged whenever you want, it's totally done on my end. There aren't any more crashes around this behavior that I'm aware of. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the crash in #1299, which ended up being very simple.
I wrote a decent bit of explanation in both of the commits, but for the short summary: At least in the more recent versions of PyQt5,
subElementRectforqproxystyle(which is used inpaint) expects three arguments, instead of having a default value for widget ofNone.This shouldn't be a problem even if older versions do have a default value, since it would already be
None.