Skip to content

Getting ready to Qt 6 (2/n). Remove QApplication::globalStrut() #579

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 15, 2022
Merged

Getting ready to Qt 6 (2/n). Remove QApplication::globalStrut() #579

merged 1 commit into from
Apr 15, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Apr 8, 2022 •
edited
Loading

This function has been deprecated in Qt 5.15.0, and has been removed in Qt 6.

@hebasto
qt: Remove QApplication::globalStrut() call
3eaf5db 
This function has been deprecated in Qt 5.15.0, and has been removed in
Qt 6 (see 033d01bd6e2aef740ad1408a04d3ca0ae3b9ba9b upstream commit).
@hebasto hebasto mentioned this pull request Apr 8, 2022
Closed
5 tasks
shaavan
shaavan reviewed Apr 10, 2022
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Concept ACK

I successfully verified that the property globalStrut() is obeselete in Qt 5. Refernce. So conceptually (and by approach), I agree with this PR. However, I would like to confirm one thing before ACKing this PR.

You mentioned that this change could have user-faced behavior changes. However, by my understanding:

  • QSize::expandedTo returns a size max of caller and argument. See for reference.
  • By default, GlobalStrut has 0 widths and height value until it is not changed, which is not being done in the codebase.
  • Hence A.expandedTo(0) should be equivalent to A.
  • Hence, removing expandedTo() should be a simple refactoring change in this case.

So would you please explain how this PR could have user-faced behavior change?

jarolrod
jarolrod reviewed Apr 12, 2022
View reviewed changes
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK 3eaf5db

Visually tested on Linux, macOS, and cross compile to windows. I did not notice any visual changes between this PR and master.

I tested dynamic builds on Linux and macOS using Qt 5.15. It would be nice if someone could test this PR using our minimum supported version of 5.11.3. Currently unable to do this testing myself.

@hebasto hebasto added the Qt 6 Transition to Qt 6 label Apr 12, 2022
luke-jr
luke-jr approved these changes Apr 13, 2022
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK 3eaf5db

From my investigation of globalStrut, Qt5 itself never actually supported it correctly in the first place.

@hebasto
Copy link
Member Author

hebasto commented Apr 13, 2022

@shaavan

Concept ACK

I successfully verified that the property globalStrut() is obeselete in Qt 5. Refernce. So conceptually (and by approach), I agree with this PR. However, I would like to confirm one thing before ACKing this PR.

You mentioned that this change could have user-faced behavior changes. However, by my understanding:

  • QSize::expandedTo returns a size max of caller and argument. See for reference.

  • By default, GlobalStrut has 0 widths and height value until it is not changed, which is not being done in the codebase.

  • Hence A.expandedTo(0) should be equivalent to A.

  • Hence, removing expandedTo() should be a simple refactoring change in this case.

So would you please explain how this PR could have user-faced behavior change?

Thank you for your review. Your analysis is correct. I've removed mention of user-faced behavior change from the PR description.

@hebasto hebasto merged commit f3e0ace into bitcoin-core:master Apr 15, 2022
@hebasto hebasto deleted the 220409-strut branch April 15, 2022 10:01
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 18, 2022
@hebasto
Merge bitcoin-core/gui#579: Getting ready to Qt 6 (2/n). Remove `QApp…
2586b3e 
…lication::globalStrut()`

3eaf5db qt: Remove `QApplication::globalStrut()` call (Hennadii Stepanov)

Pull request description:

  This function has been deprecated in Qt 5.15.0, and has been [removed](qt/qtbase@033d01b) in Qt 6.

ACKs for top commit:
  jarolrod:
    ACK 3eaf5db
  luke-jr:
    utACK 3eaf5db

Tree-SHA512: 71ee539b6ffa3755f7e6beaa72a8937886471e298830878def6dd9f48c601611d94d52c638bc1602f938df2ba84ff8b130ea8da8e6c08ae7146173fa613a5003
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@jarolrod jarolrod jarolrod left review comments

@shaavan shaavan shaavan left review comments

@luke-jr luke-jr luke-jr approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Qt 6 Transition to Qt 6
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hebasto @luke-jr @jarolrod @shaavan