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

Fix pointcloud properties dialog minimum size #55976

Merged
merged 8 commits into from
Feb 8, 2024

Conversation

uclaros
Copy link
Contributor

@uclaros uclaros commented Jan 24, 2024

Description

The 3D View tab in a point cloud's properties dialog was forcing the dialog to relatively big minimum height.
Peek 2024-01-24 17-45

Probably fixes #43276 , not sure about the disappearing part but I hope it was some QT weirdness when hitting the minimum size.

@github-actions github-actions bot added this to the 3.36.0 milestone Jan 24, 2024
Copy link

github-actions bot commented Jan 24, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 0b6ca1c)

*
* \since QGIS 3.36
*/
class GUI_EXPORT QgsStackedWidget : public QStackedWidget
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice approach!

Can I suggest for future proofness that we:

  1. Add an enum:
enum class SizeMode {
    ConsiderAllPages, //!< The sizes of all pages are considered when calculating the stacked widget size
    CurrentPage, //!< Only the size of the current page is considered when calculating the stacked widget size
};
  1. Add getters/setters for SizeMode, default SizeMode to ConsiderAllPages (ie match QStackedWidget)
  2. Adapt the subclass sizeHint methods to consider the SizeMode

This way we could start moving everything over from QStackedWidget to QgsStackedWidget, and we'd have a natural place to add other tweaks and improvements in future. Much like what we do for QgsScrollArea. (There's also a test to force use of QgsScrollArea over QScrollArea, we could potentially add the same for QgsStackedWidget later)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I guess we could, I'm not sure about the potential gain (other than homogeneity). In most places we use a stacked widget it either holds similar widgets or it is used as a temporary container and the inactive page gets deleted.
With QgsScrollArea it was quite different, as it implemented the absolutely necessary functionality to avoid messing with widgets while scrolling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's definitely value in this minimal height stacked widget elsewhere. There's lots of places where we have ugly empty space in dialogs because a stacked widget has forced a too large minimum height for the current page, and all the widgets under that get pushed down for no visible reason.

I'd still push for the enum setting added, because this is kind of our one change to do that and have the default behavior match QStackedWidget. If we need to add other tweaks to QgsStackedWidget in future then things get a bit messier as we'd need to avoid breaking the initial default behaviour.

#ifndef QGSSTACKEDWIDGET_H
#define QGSSTACKEDWIDGET_H

#define SIP_NO_FILE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's expose, there's potential value here for plugins

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we maybe backport this as is and then expose and expand on a separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with that

@nyalldawson nyalldawson merged commit 7f6b4b4 into qgis:master Feb 8, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Point Cloud Property window resizing issue
3 participants