lxqt / liblxqt

Core utility library for all LXQt components
https://lxqt-project.org
GNU Lesser General Public License v2.1
60 stars 51 forks source link

Fixed setting of item geometries in grid layout #351

Closed tsujan closed 2 months ago

tsujan commented 2 months ago

With a horizontal layout, the item width was increased by one pixel when there was empty space, but that one pixel wasn't taken into account anywhere else — and similarly with a vertical layout. I removed it, and the occasional position shifts disappeared from task buttons of lxqt-panel.

In short, the original calculations inside GridLayout::setGeometry didn't seem correct to me.

Fixes https://github.com/lxqt/lxqt-panel/issues/2065

tsujan commented 2 months ago

I didn't know whom I should ask for reviewing/testing, partly because the code was very old, but also because I didn't know who would be accessible or could reproduce the panel issue. @palinek, @stefonarch, ...?

stefonarch commented 2 months ago

I think I can reproduce, didn't ever really noticed.

Before: https://github.com/user-attachments/assets/4ac5c35d-b67c-4710-a534-d1c406c557d7

After: https://github.com/user-attachments/assets/40f11750-3e9c-4377-9e00-16d43fa8a61f

tsujan commented 2 months ago

I think I can reproduce, didn't ever really noticed.

Yes, your first video shows the shift @isf63 and I were talking about, and It doesn't happen in your second video.

isf63 commented 2 months ago

Tested and works. Didn't test with vertical panels however.

Looking at the diff (which I hardly understand), it looks like moderate logic changes. But if it is indeed correct then it results in less code, which is nice.

tsujan commented 2 months ago

Tested and works.

Thanks for testing.

Didn't test with vertical panels however.

Its logic is exactly the same.

Looking at the diff (which I hardly understand), it looks like moderate logic changes.

I guess the original coder believed that a grid consisted of horizontal and vertical lines with a thickness of one pixel. That assumption complicated the logic and caused the problem.

tsujan commented 2 months ago

However I look at it, the old code could have been written only based on a wrong assumption. Merging...