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

Made ComboBox scrollable #5581

Merged
merged 2 commits into from
Jul 12, 2024
Merged

Made ComboBox scrollable #5581

merged 2 commits into from
Jul 12, 2024

Conversation

FloVanGH
Copy link
Member

@FloVanGH FloVanGH commented Jul 9, 2024

Fixes #4727

@FloVanGH FloVanGH requested a review from tronical July 9, 2024 07:36
Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

I have concerned about the hardcoded size for the item height. Also only 5 visible items is really small.
Qt's comobbox has that configurable with the default being 10.

callback selected <=> i-base.selected;
callback selected <=> base.selected;

property <length> item-height: 40px;
Copy link
Member

Choose a reason for hiding this comment

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

Can this really be hardcoded like this? Where does the value comes from?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ogoffart it comes from the fluent style. Yeah problem is I didn't find a way to get the height but I need it for the popup height calculation. Because sooner or later the qt style will be deleted I thought it's ok for now. If not just let me know, and maybe you know how to get the height.

@FloVanGH
Copy link
Member Author

FloVanGH commented Jul 9, 2024

I have concerned about the hardcoded size for the item height. Also only 5 visible items is really small. Qt's comobbox has that configurable with the default being 10.

I think until we have a way to place the popup depending on available width / height of e.g. window or parent we should not allow the combobox height to be to big, because it will end up in more situations someone mentions that the combobox popup is cut off. Or what do you think?

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Yes indeed, we need to place the popup window differently if it is at the bottom of the screen.
This is complicated logic that needs support in the runtime and improvement to the PopupWindow which is unfortunately out of scope for the upcoming release.
I still think 5 is a bit small though, i'd prefer if it was a bit more.

And I'm not happy about the hardcoded size in the Qt style at all. You could get the size through NativeStyleMetrics perhaps. Anyway, i get your point that you don't like to touch this style too much.

Given that this is clearly an improvements for the other styles (as opposed to have the menu half clipped) i think we should merge this with a bit more elements visible and not set the height to the hardcoded size in the Qt style.


for value[index] in root.model: NativeStandardListViewItem {
item: { text: value };
is-selected: root.current-index == index;
has-hover: ta.has-hover;
combobox: true;
height: root.item-height;
Copy link
Member

Choose a reason for hiding this comment

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

Even if we hardcode the size for the purpose of computing the maximum height, we shouldn't hardcode the size here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ogoffart yes I know. It I also prefer not to set hard coded with / height. But in this case I'm not able to determine the viewport-height because of again the restriction of accessing stuff inside of the PopupWindow. So without hardcoded height it is not possible at the moment to implement the grow behavior for the ComboBox's Popup. Alternative would be to give the popup a fixed height.

@ogoffart
Copy link
Member

With #5584 merged, it is now possible to position based on the size.

@FloVanGH
Copy link
Member Author

With #5584 merged, it is now possible to position based on the size.

@ogoffart it also does not fix the need of hardcoded size. The problem is not only the placement of the popup it is also to stretch the popup depending on count of displayed items. Alternative would be a fixed height for the Popup what it's also ok I think.

@ogoffart
Copy link
Member

I think a hardcoded maximum size would be a good workaround for now. (without fixing the size of the NativeStandardListViewItem)

@FloVanGH FloVanGH requested a review from ogoffart July 12, 2024 08:55
@FloVanGH FloVanGH merged commit 806d12b into master Jul 12, 2024
35 checks passed
@FloVanGH FloVanGH deleted the florian/cb-scroll branch July 12, 2024 13:14
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.

ComboBox should use a scrollable view when the model is too large to display
3 participants