gregdavill / KiBuzzard

MIT License
425 stars 33 forks source link

dialog: Fix scaling issues #113

Closed gregdavill closed 9 months ago

gregdavill commented 9 months ago

This returns some behaviour to normal, but keeps the accurate scaling of width and height.

Screencast from 2024-02-26 21-24-57.webm

gregdavill commented 9 months ago

The changes in #99 were also trying to set a fixed height, this meant that adding padding to top/bottom also altered the text height. While I can see that begin useful sometimes, there were some quirks arising from that.

IMO it's more intuitive to have the height refer specifically to the text height, so labels with different padding, or standard and knockout labels have the same text height.

Similar with the width adjustment, while it's useful to see the adjustment your text is having on the width, I don't think that overriding the user entered value actually helps. Maybe we add a separate total_height/width message somewhere?

arturo182 commented 9 months ago

Thanks for looking into it. It works a lot better now, but there's still a bit of weirdness.

The padding boxes don't allow you to set "0", they will always fall back to 0.001. Probably a float rounding issue. Possibly also we don't need 3 decimal digits, 1 or 2 would be enough.

The Height and Width adjust arrows always adjust the value by 0.25 which is fine when you start at x.0, but it gets weird if you start at a different value. Maybe they should adjust by 0.1. And, again, probably 3 decimal digits is too much.

Lastly, maybe we should have the adjust arrows for Height, Width, and Padding all inc/dec by the same value, for consistency.

See:

https://github.com/gregdavill/KiBuzzard/assets/249082/4b914068-2583-4bf0-830f-d4a0c2560950

arturo182 commented 9 months ago

One more regression I noticed: the Height is broken for multi-line labels, it's applied to the total label instead of each line separately, see:

https://github.com/gregdavill/KiBuzzard/assets/249082/be342af5-dbb7-4d74-9d2b-ae963aa967f8

gregdavill commented 9 months ago

Thanks for taking a look!

Internally the padding has always stopped at 0.001. This helps with weirdness that happens with the knockout operation. But no reason the padding control needs to update to show this value.

I'm not sure why I used 0.000 format for the height/width. Once on a PCB these values are more like +/- 0.1 so I'm happy to drop down to a single decimal digit.

I've put a simple fix in for the height, it now just calculates this based on the first line height.

arturo182 commented 9 months ago

Seems to work great now, one small issue that making the padding widgets smaller revealed is that there is some weird spacing logic for Padding, the Left widget is not getting spaced properly. I took the liberty of fixing this 😄

Before:

buzz_before

After:

buzz_after

gregdavill commented 9 months ago

Thanks for the extra testing and fix! :smile: