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

Re:#9966 Fix Crash in MZFlickable #9984

Merged
merged 2 commits into from
Oct 27, 2024
Merged

Re:#9966 Fix Crash in MZFlickable #9984

merged 2 commits into from
Oct 27, 2024

Conversation

strseb
Copy link
Collaborator

@strseb strseb commented Oct 24, 2024

Soooo i did set up a 6.8 dev build and was able to repro the crash seen in #9966 - this is a fun one.
In 6.8.0 we get the "onHeightChange" before the window has finished to initialize, so that when we call mapToItem(window.contentItem) we defer a nullptr and crash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qt 6.8.0 no longer links to this it seems so we need to manually declare that :)

Copy link
Collaborator

@oskirby oskirby Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For windows at least, I think we tend to handle this using pragmas: #pragma comment(lib, "Iphlpapi")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This errored and i think noone is using it, so let's just remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qAsConst is depricated in 6.8.0

Copy link
Collaborator

@oskirby oskirby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Just one minor nitpick about how to link libraries on Windows, but it's not worth holding the patch up if you feel strongly about it.

@XavierCLL
Copy link

Hi @strseb

I am not sure if the purpose of this PR is fix all the problems with Qt6.8, I applied this patch (also this one 9961) to the stable version, and the UI is usable, but one thing is not working, when I click in the list of servers "Select location", the UI crash with the following traceback:

trace.txt

@strseb
Copy link
Collaborator Author

strseb commented Oct 27, 2024

Hey @XavierCLL - i was not expecting one small patch to magically fix all issues that might arise with 6.8 :)
Given that we're still shipping the client on most plattforms with qt6.6 i really appreciate i really appreciate the testing, posting traces and bug's filed :)
Let's pull that stacktrace into a new bug and we'll tackle it from there.

@strseb strseb merged commit 874ffe4 into main Oct 27, 2024
117 checks passed
@strseb strseb deleted the basti/qt6.8.0_fixes branch October 27, 2024 11:42
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