Improve dock layout for lower resolution

[MrStevns]

A follow up PR from #1824

I've adjusted the margin as well as the min width for all the dock widgets so they are aligned.
In addition the font has also been adjusted from 13 pt to 11 pt.

The result looks like this:

Front: new layout
Back: old layout

The widgets that had the greatest impact adjusting this was the tool options widget as well as the onion skin widget.

Side by side:

Old	New

Adjusting the font also helped quite a bit.

11 point font size	13 point font size
142 px	162 px

I've also added extra QLayout around certain elements, which is not a mistake. Doing so fixes some problems where Qt added extra margins around checkboxes and labels.

[chchwy]

Would you be able to change the margins of the flowlayout in the toolbox as well?
It should be at toolbox.cpp line141 something like:

FlowLayout* flowlayout = new FlowLayout(0, 3, 3);

[MrStevns]

I played around with that too, though it causes the widget to never go into one vertical line, at least on mac.

3 px margin	0 px margin

It bothers me a bit but I can't figure out how to force the layout into submission. For some reason it won't get smaller than 60 px or so... even though I believe i've tweaked all possible values. I must be missing something.

edit: Aha! it's the title widget that enforces the min width.

[chchwy]

It bothers me a bit but I can't figure out how to force the layout into submission. For some reason it won't get smaller than 60 px or so... even though I believe i've tweaked all possible values. I must be missing something.

edit: Aha! it's the title widget that enforces the min width.

yea, title bar is to blame.

[MrStevns]

Small improvement. The toolbox can now also be shrunk down when laid out horizontally.

[MrStevns]

Hmm not sure what's up but the font looks larger on some of the widgets on my windows machine.
it looks like setting the font point size specifically almost has the opposite effect on windows... sigh 😩

https://forum.qt.io/topic/125993/font-point-size-weird-behaviour-on-windows I found this and some other threads mentioning similar problems... what a can of worms.

Sure we could just go back to the old font but seriously... Qt, you had one job.

[MrStevns]

Fixed. The "solution" is to use qstylesheet instead. In this case i'm setting the font-size of all widgets that are children of BaseDockWidget.

[MrStevns]

I have also tested these changes on a linux distro now.

[J5lx]

The overall direction of this PR looks fine to me, but there’s something weird going on with the font size. On Windows, it looks practically unchanged except for the title bars, whereas on Linux the text is painfully tiny for me, much smaller than the default font size (note the status bar for comparison):

My theory is that this is because of the way you’re hard-coding the font size. I experimented around a little and found that setting the font size to 86% (first screenshot) of the global default gave me a result that (subjectively) seemed much closer to the effect seen in your original screenshots, while 85% (second screenshot), which is closer to your original 11/13 ratio, still looked a little too small for my liking but still better than the original hard-coded 11px.


For reference, I’m using setStyleSheet(QString("QWidget { font-size: %1pt }").arg(.86 * QFont().pointSizeF())); (pixelSize() does not work). Note that I haven’t tried this on Windows yet.

Also, on Qt 6, I now get a new warning in the console:

Init Dock widget: "TimeLine"
Init Dock widget: "ColorWheel"
Init Dock widget: "Color Inspector"
Init Dock widget: "ColorPalette"
Init Dock widget: "Onion Skin"
Init Dock widget: "ToolOption"
QWidget::setMinimumSize: (scrollArea/QScrollArea) Negative sizes (0,-1) are not possible
Init Dock widget: "ToolBox"

And lastly, your most recent commit seems to have broken the colour inspector preview somehow, it now defaults to the standard widget background on startup. Not sure what exactly is going on there…

[MrStevns]

Thanks for testing it out. 👍

On Windows, it looks practically unchanged except for the title bars, whereas on Linux the text is painfully tiny for me, much smaller than the default font size (note the status bar for comparison):

I've reverted the font change again.
This seems to be one of the those things that require a greater understanding of Qt in order to change properly, so we'll keep the same font for now. I don't like the idea of scaling the font with some arbitrary number. It must be possible to use proper px or pt values and then it's a matter of figuring out how those work in Qt compared to web, if it's css they're basing their logic of.

Also, on Qt 6, I now get a new warning in the console:

I'll fix that.

[J5lx]

I don't like the idea of scaling the font with some arbitrary number. It must be possible to use proper px or pt values and then it's a matter of figuring out how those work in Qt compared to web, if it's css they're basing their logic of.

“Scaling the font with some arbitrary number” is pretty standard on the web and generally recommended for accessibility, I’m not sure what’s the problem with it. E.g. the main font size on our forum is 0.938em, and I haven’t changed it. If you have something else in mind, sure. But please consider taking the system default as a base in one way or another since we mainly use native styling.

[MrStevns]

“Scaling the font with some arbitrary number” is pretty standard on the web and generally recommended for accessibility, I’m not sure what’s the problem with it. E.g. the main font size on our forum is 0.938em, and I haven’t changed it. If you have something else in mind, sure. But please consider taking the system default as a base in one way or another since we mainly use native styling.

Alright fair enough, I'm used to mobile development where it's not like this but if that's how it's done, then we'll do it like that.
I've reverted the font changes now though, instead I think it would be wiser to create a font size preference, so the user can control this themselves. That's out of scope for this PR though.

And lastly, your most recent commit seems to have broken the colour inspector preview somehow, it now defaults to the standard widget background on startup. Not sure what exactly is going on there…

This was caused by a style override which has been reverted.

Would you mind reviewing again @J5lx

[J5lx]

I still get the warning that I mentioned above. Other than that it looks good to me though.

[MrStevns]

Hmm strange, it's not something i'm seeing in my build anymore.
I'm using Qt 6.7.0, what version are you on?

Also, is this with the default layout or have you moved the docks around?

[J5lx]

I’m on 6.7.2 and seeing that warning with the default layout although, upon closer inspection, only some of the time. I did some debugging and I found there’s at least two issues at play here. The first one is only indirectly related to the warning, but could very well be the reason why you’re not seeing it. That’s because at its very end, initUI calls getMinHeightForWidth which reads mDockArea. However, mDockArea is only written in the dockLocationChanged signal handler which was only connected earlier in initUI. Therefore the mDockArea value is uninitialized at that time and reading it in getMinHeightForWidth results in undefined behaviour – sometimes the if branch executes, other times the “else branch”.

Only in the latter case does the second issue occur which is what actually causes the warning. That’s because at this point in time, BaseDockWidget::getMinHeightForWidth always returns -1, which is then directly passed directly into setMinimumHeight in initUI, resulting in the warning “QWidget::setMinimumSize: (scrollArea/QScrollArea) Negative sizes (0,-1) are not possible”.

[MrStevns]

Ah... good point regarding mDockArea being uninitialized, I forget that it was an enum 😅

I've made a few changes, though i'm unsure about whether we should simply change -1 to 1 in BaseDockWidget rather than me simply not calling the base class at all, if it always gives a warning. What do you think?

[J5lx]

Since you brought up BaseDockWidget, I had a closer look at it and realised that the entire min height handling in there is actually entirely Toolbox-specific and it can be simplified a bit by consolidating that in Toolbox itself. Please take a look: MrStevns#23

[J5lx]

With that taken care of, everything looks good to me now. Feel free to merge if you’re happy with it as well.

[MrStevns]

Thanks Jakob! 🙏