vslavik / poedit

Translations editor for Mac, Windows and Unix
https://poedit.net
MIT License
1.71k stars 274 forks source link

Fix sizing of the ID column on wxGTK #747

Closed super7ramp closed 2 years ago

super7ramp commented 2 years ago

This is a naive attempt to fix the sizing issues of the ID column observed with wxGTK and reported in #643 and #714.

Basically:

I've tested this with the following environment:

vslavik commented 2 years ago

I don't understand the reference to https://github.com/wxWidgets/wxWidgets/commit/560a81b913f23800e286d297d8cd38e72a207641 in code — do you mean that that change caused this?

I don't follow the reasoning or the code either, but it may be that I just need one more fresh look.

Do I understand correctly that this is related to the last column being auto-growing natively in controls? In other words, would moving the ID column to be the first also fix it?

Finally, there are now reports of the same thing on macOS 12.3 may be helping with that too.

vslavik commented 2 years ago

I don't follow the reasoning or the code either, but it may be that I just need one more fresh look.

Never mind, the explanatory notes in #643 and #714 help - thanks!

super7ramp commented 2 years ago

I don't understand the reference to https://github.com/wxWidgets/wxWidgets/commit/560a81b913f23800e286d297d8cd38e72a207641 in code

This special handling of the width value when column is hidden existed in a different form: It has been moved from line 730. Instead of PoeditListCtrl::CreateColumns() setting the width to 0 when column is hidden, I let DataViewFixedColumn::GetWidth() returning 0 when it is hidden - which seems functionally equivalent.

This was not a necessary change and lacked explanations. It felt to me the new DataViewFixedColumn::GetWidth() was a more appropriate place to handle this specific handling of the ID column width than PoeditListCtrl::CreateColumns().

Should I:

Do I understand correctly that this is related to the last column being auto-growing natively in controls? In other words, would moving the ID column to be the first also fix it?

I do think it is linked to the last column being auto-growing and that moving the ID column to be the first column would fix #714 at least.

vslavik commented 2 years ago

This special handling of the width value when column is hidden existed in a different form: It has been moved from line 730.

Yes, but you're referring a specific bug in wx here, one which was already fixed. If this part of the code is for compatibility with older wx, then it should be inside #if !wxCHECK_VERSION(3,1,6) so that it can be removed once wx-3.2 is a prerequisite (which it will be the moment a stable 3.2 series is released; I'm only supporting wx-3.0 compilation to accommodate conservative distros).

super7ramp commented 2 years ago

Yes, but you're referring a specific bug in wx here, one which was already fixed. If this part of the code is for compatibility with older wx, then it should be inside #if !wxCHECK_VERSION(3,1,6) so that it can be removed once wx-3.2 is a prerequisite

Added a conditional as per your suggestion except it checks version 3.1.3 which seems the first wxWidgets release to include the referenced commit https://github.com/wxWidgets/wxWidgets/commit/560a81b913f23800e286d297d8cd38e72a207641.

vslavik commented 2 years ago

Just a note that this fix doesn't address a similar problem on macOS 12 - that's to be expected, but I hoped it could. I'm going to merge this together with macOS fix - it's possible (likely?) that one will affect the other...

vslavik commented 2 years ago

The macOS issue was unrelated (111df0f3); both are now merged.

Thanks a lot for figuring the tricky issue under wxGTK and implementing the fix!