Skip to content

Conversation

@xMAC94x
Copy link

@xMAC94x xMAC94x commented Nov 21, 2025

see issue #1186, though this does not fix the width problems

@xMAC94x xMAC94x changed the title with #1186 gtk4 seemed to no longer set a min_height on the bluetooth… [bluetooth] grow popup size until a configureable limit based on the number of devices Nov 21, 2025
@postsolar
Copy link
Contributor

Could you specify what unit it is in the docs? Like, pixels, centimeters, devices, bananas 😄

@xMAC94x
Copy link
Author

xMAC94x commented Nov 21, 2025

Could you specify what unit it is in the docs? Like, pixels, centimeters, devices, bananas 😄

will do

@xMAC94x xMAC94x force-pushed the xMAC94x/bluetooth_min_height branch from 015f904 to bb2f42a Compare November 21, 2025 17:04
@JakeStanger
Copy link
Owner

Thanks for this. I'm going to hold off merging this until I have time to properly dig into the issue, because I feel like introducing more config options to resolve it is not the right way to go, especially pixel-based.

@kuzy000
Copy link
Contributor

kuzy000 commented Nov 23, 2025

I think this should also work (right after creating devices):

devices.set_max_content_height(grow_height_until);
devices.set_propagate_natural_height(true);

Shame that max-content-height isn't accessible from CSS.

@JakeStanger
Copy link
Owner

JakeStanger commented Nov 23, 2025

Pros/cons of each approach?

If we do go for this option btw, I'm thinking max_height would be a better name for the config option.

@xMAC94x
Copy link
Author

xMAC94x commented Nov 23, 2025

limiting the max is kinda undesired, when we have set scrollable = false.

Or we only do this in case when scroll-able is enabled 🤷

@kuzy000
Copy link
Contributor

kuzy000 commented Nov 24, 2025

Or we only do this in case when scroll-able is enabled 🤷

I think scrollable = false is the same as setting max_height = 99999 . So maybe we can replace scrollable with something like max_height = -1 or max_height = null? Seems like volume module have something similar with its truncate.length: the default value is null.

Maybe max_height: Option<i32> instead of scrollable is a good idea.

https://github.com/JakeStanger/ironbar/wiki/volume#configuration

@JakeStanger
Copy link
Owner

Maybe max_height: Option<i32> instead of scrollable is a good idea.

Agreed with this

@xMAC94x xMAC94x force-pushed the xMAC94x/bluetooth_min_height branch from bb2f42a to 082652c Compare November 24, 2025 15:53
@xMAC94x
Copy link
Author

xMAC94x commented Nov 24, 2025

Maybe max_height: Option<i32> instead of scrollable is a good idea.

Agreed with this

implemented this.

When revieweing you might probably see the > 0 check in the match clause. This is necessary for the toml configs which DO NOT allow unsetting a default value. So when we have a default value for max_height other than null (like we have) -> toml can NEVER unset this max_height back to None ...
And there are too many issues, like toml-lang/toml#30 about this :/

fn default() -> Self {
Self {
scrollable: true,
max_height: Some(330),
Copy link
Owner

Choose a reason for hiding this comment

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

Is 330 based on anything in particular?

Copy link
Author

Choose a reason for hiding this comment

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

It's a semi-useful number that I thing results in a good visual height.

And it has the effect, when the List is full the last Item will be chopped in half, which is indicating users clearly that the popup is scroll-able. (tested with bluetoothctl and scan on )

Copy link
Owner

Choose a reason for hiding this comment

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

And it has the effect, when the List is full the last Item will be chopped in half, which is indicating users clearly that the popup is scroll-able.

This fully depends on styles - any font size, padding, margin or min-height changes can offset this. What was this tested against?

…e bluetoothbox, this adds a method to allow the box to grow until a limit of height is reached.
…agate_natural_height fn in combination with limiting max_content_height

If you are wondering why negative values and 0 are interpreted as No Scrollbar - TOML doesnt provide a possibilty to overwrite a default, so if we would rely on toml and have a default != null it would NOT be possible to ever deactivate the scrollbars :/
This is a flaw/design decision by toml. And ofc here it makes sense to have scrollbar by default.
@xMAC94x xMAC94x force-pushed the xMAC94x/bluetooth_min_height branch from 082652c to e1cbed0 Compare November 24, 2025 22:46
@postsolar
Copy link
Contributor

What is the tradeoff between limiting to number of devices and limiting to specific amount of pixels? I think the final choice will be relevant to some other modules too, e.g. #1050 (and as I just discovered it actually stops opening the popup once intended height approaches monitor height).

@xMAC94x
Copy link
Author

xMAC94x commented Nov 25, 2025

What is the tradeoff between limiting to number of devices and limiting to specific amount of pixels? I think the final choice will be relevant to some other modules too, e.g. #1050 (and as I just discovered it actually stops opening the popup once intended height approaches monitor height).

I don't know how to translate between items and pixels. You can change the height of the entries via CSS and I maybe its also possible to have individual entries with different height than others. Do you have an algorithm in mind here ?

@JakeStanger
Copy link
Owner

Calculating the height reliably would be nigh impossible. It's possible to get the actual height after everything's rendered, but I can't imagine that's very useful.

@xMAC94x
Copy link
Author

xMAC94x commented Nov 26, 2025

agree, @postsolar would you also be fine with a pixel version ?
For personal configuration you can prob do the calculation item.len() -> required pixels once and then set it via the config.

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.

4 participants