Fix hardcoded fonts scaling issues

[Rossmaxx]

Fixes a regression from #7185

The regression seems to have been caused by missing calculation between point font sizes to pixel sizes. I manually changed the values.

[michaelgregorius]

@Rossmaxx, can you please point out what you think I am misunderstanding or confusing? It sounds like I broke something because I did not understand it when in fact I think I have a good understanding of the issues.

The solution of this PR will likely break on HiDPI screens and will not look consistent across different systems.

Results of a Perplexity session

I had a chat with Perplexity AI about the topic which you can find here: https://www.perplexity.ai/search/if-i-specify-a-point-size-for-2LjnIZSORsqque0wFoO8xw

The most important part is the second question which I will (partially) quote here for completeness:

Q: Do I understand correctly that a given point size will effectively lead to rendering at different pixel sizes depending on the DPI of the display?

A: Yes, you understand correctly. The relationship between point size and pixel size is indeed dependent on the display's DPI (Dots Per Inch). Here's a more detailed explanation:

1. Point to pixel conversion: The basic formula is: pixel size = (point size * DPI) / 72 This is because 1 point is defined as 1/72 of an inch.
2. DPI variation: Different displays have different DPI values. For example:
   • A typical desktop monitor might have 96 DPI
   • A high-resolution laptop screen could have 192 DPI or higher
   • A smartphone display might have 300 DPI or more
3. Practical example: Let's say you set a font to 12 points:
   • On a 96 DPI display: 12 * (96/72) = 16 pixels
   • On a 192 DPI display: 12 * (192/72) = 32 pixels
   • On a 300 DPI display: 12 * (300/72) = 50 pixels
4. Qt's handling: Qt tries to abstract this complexity by using device-independent pixels (DIPs) internally. However, the final rendering still depends on the physical characteristics of the display.

The practical example in point 3 shows that specifying a point size of 12 points leads to different pixel sizes at which the font will be rendered depending on the DPI.

I am aware that AI systems are not the single source of truth and might write wrong stuff. However, this matches with my already existing understanding. Furthermore, I'd also wonder why there should be a function to set text in point sizes if all that it would do would just be to multiply the point size with 4/3 to yield a pixel size.

Example: LMMS' combo box

Let's use LMMS' own combo box as an example of how this might break. The combo box renders itself at a fixed height of 22 pixels: https://github.com/LMMS/lmms/blob/d703f391533c3c5c682b5baf0984be44b8786bfa/include/ComboBox.h#L54

This means that at 96 DPI a 12 point text will fit as it uses 16 pixels. However, the text will already be clipped at 192 DPI and 300 DPI because the 32 and 50 pixels of the text will not fit the combo box widget anymore.

Proposal for a fix

In my opinion the way forward is to adjust the pixel sizes used by adjustedToPixelSize so that they are large enough for the different situations. As I have noted somewhere else, GUI element that use (hard-coded) pixel sizes should specify their font sizes in pixel sizes as well because it is the only way to reliably get consistent size ratios between the widget and the text sizes.

[michaelgregorius]

If my assumptions are correct then the text rendering of the combo box should start to break at DPIs of 158 and higher. Here's how to calculate the critical DPI for the used font size of 10 which will be interpreted as 10 points with this PR:

DPI = 22 * 72/10 = 158.4

The formula calculates at which DPI a point size of 10 will map to 22 pixels. That's where these numbers are coming from.

At 158 DPI the text will use the full vertical space of the combo box. Any higher DPI should make it exceed it.

The following gives an overview of how the ratio of text height to combo box height should change for different DPI values:	DPI	Pixels	Ratio
96	13.333	0.6
117	16.25	0.75
158	22	1
196	27.2	1.24

[Rossmaxx]

So basically, the old implementation was correct for LMMS' usage because every other widget seems to have hardcoded sizes. The way i see it, the fonts are now corrected but the behaviour will become inconsistent because the rest of the widgets use pixel sizes. Even in other places, there seems to be a mix of pixel and point sizes.

If i understand correctly, this PR goes from one regression to another. The best course for now would be to adjust the pixel sizes for better visibility.

Though for hidpi support, we should eventually transition everything to use point sizes instead, which will be easier once we support svg graphics, but that's another issue.

[michaelgregorius]

So basically, the old implementation was correct for LMMS' usage because every other widget seems to have hardcoded sizes. The way i see it, the fonts are now corrected but the behaviour will become inconsistent because the rest of the widgets use pixel sizes. Even in other places, there seems to be a mix of pixel and point sizes.

Assuming that you mean this commit with "old implementation". I think that it has worked because it's using the DPI as an additional factor (effectively 96 / QApplication::desktop()->logicalDpiY()). Setting the font size in points, i.e. the implementation of setPointSizeF, also uses the DPI internally to determine the font size, so I think these DPI values canceled each other out which means that the function has effectively set a pixel size. And it did so by doing a needlessly complex computation. And it also broke down because Wayland reports other DPI values than X.

One could also say that the fonts might look corrected on your system but that they will likely look broken on a HiDPI screen. Do you happen to know the DPI of you system? I assume that it's below 120 so that for example the fonts of the combo box will not exceed the rest of the combo box and have a nice ratio between font size and widget size.

Would be interesting if someone with a very high DPI screen could test your PR and report if the combo boxes look okay.

If i understand correctly, this PR goes from one regression to another. The best course for now would be to adjust the pixel sizes for better visibility.

Yes, that would be the best fix in my opinion. Because then for certain widgets everything would be defined in pixel sizes and thus it would always scale well with global fractional scaling settings and the visual ratios of the element would always be consistent.

Though for hidpi support, we should eventually transition everything to use point sizes instead, which will be easier once we support svg graphics, but that's another issue.

Using point sizes for all widgets would mean that the widget implementations would need to be adjusted to take the font size that's set for a widget into account for example when they report their minimum sizes and paint themselves. I assume that's what Qt widgets do.

[messmerd]

Here's how it looks for me

[michaelgregorius]

Here's how it looks for me [...]

What's your DPI, @messmerd?

[Rossmaxx]

@michaelgregorius I have reverted the behaviour and manually changed the values per your suggestion.

@messmerd and @musikbear is this still ok for you now?

One follow up I would like to do is to specify the font sizes as constants and pass those. Agree or not?

[musikBear]

specify the font sizes as constants

I would rather like that font-size was a CSS value or a user-choice in settings. Any other program has that option. In the perfect solution, it would be the UI-containers that scaled themself according to the chosen font-size. Ok that may look really ugly, but remember that it would be what the user did select, and the user may have a really good reason, so the appearance should not be a factor at all. But scaling forms is afaik not an option in qT (?)

@musikBear is this still ok for you now?

I cant see any screenie with the altered values Best screenie would be at default magnification with most possible sub-forms opened over song-editor, like

[michaelgregorius]

One follow up I would like to do is to specify the font sizes as constants and pass those. Agree or not?

Agree. It's something that I also did in my working copy when I started to work on it. I defined them per component, e.g. in Knob.cpp, in LcdWidget.cpp and everywhere where more than one adjustment was needed. Example:

constexpr int c_labelSize = 12;

Then I used c_labelSize in the calls to adjustedToPixelSize.

[michaelgregorius]

With the current branch the fonts are more readable but some screen are in dire need of more "breathing space". Here are some screenshots of how it looks on my system.

Instrument

The instrument view is a little bit wider now to the right and it seems that this is caused by the "Stacking and Arpeggio" tab because it is the only one that uses the full space.

The screens in general look a bit crowded now. However, I think one thing that should be done anyway is to remove the fixed size for the instrument view and to let it size according to the layouts in the different tabs. It's a bit unfortunate that the size of the five other tabs is currently dictated by the minuscule size of the pixmapped instrument window.

Mixer

Pattern Editor

Tempo and Time

[michaelgregorius]

specify the font sizes as constants

I would rather like that font-size was a CSS value or a user-choice in settings. Any other program has that option. In the perfect solution, it would be the UI-containers that scaled themself according to the chosen font-size. Ok that may look really ugly, but remember that it would be what the user did select, and the user may have a really good reason, so the appearance should not be a factor at all. But scaling forms is afaik not an option in qT (?)

This will not be possible as long as LMMS uses it's own custom widgets with hard-coded pixel sizes and hard-coded layouts, i.e. no Qt layouts.

[musikBear]

This will not be possible as long as LMMS uses it's own custom widgets with hard-coded pixel sizes and hard-coded layouts

Is hardcoding a must? If it is custom, why hardcode it. I realize that everything could co really wrong, and the UI next to selfdestruct but an option: Reset-UI to default would be only thing needed to get back to default. Before i dabble into a mockup, it would be good to know if hardcoded sizes of widgets is an absolute must

[Rossmaxx]

Michael says you need to hardcode font sizes because of the fact that rest of LMMS widgets (ie combobox, mixer line, instrument ui, text boxes, knob etc) are hardcoded in pixels and using properly scaled fonts will make the entire UI inconsistent. It's quite a paradox but we need to live with it.

We can convert fonts back to point sizes but in order to do that, first the other widgets must become properly scaleable. I believe Michael wrote about it somewhere.

[Rossmaxx]

I have extracted the common font values to constants. One other thing that i did is to rename the gui_templates.h to FontHelper.h to reflect the naming on the functionality better.

[Rossmaxx]

The instrument view is a little bit wider now to the right and it seems that this is caused by the "Stacking and Arpeggio" tab because it is the only one that uses the full space.

I somewhat fixed it in the latest commit by reducing the knob size. The issue still partially exists. It just looks better.

However, I think one thing that should be done anyway is to remove the fixed size for the instrument view and to let it size according to the layouts in the different tabs. It's a bit unfortunate that the size of the five other tabs is currently dictated by the minuscule size of the pixmapped instrument window.

As an intermediate step, we can center all the instrument sub widgets (and if possible, let the tabs take full space of it). That's beyond scope of this PR however.

updated screenshot:

[Rossmaxx]

https://github.com/LMMS/lmms/actions/runs/10879040716

artefacts for testing

[michaelgregorius]

@Rossmaxx, I have noticed that the number of calls to adjustedToPixelSize has even increased with this pull request, e.g. it was added to plugins/Eq/EqCurve.cpp, plugins/TapTempo/TapTempoView.cpp, etc.

I assume that you have checked if the widgets where it has been added are indeed not implemented in a scalable way? I have checked for EqCurve.cpp and the handle where it was added is indeed not scalable. Did you check for all widgets?

[Rossmaxx]

I have noticed that the number of calls to adjustedToPixelSize has even increased with this pull request

I'm trying to consolidate font related code to our wrapper function, in order to remove direct calls to setPixelSize.

I assume that you have checked if the widgets where it has been added are indeed not implemented in a scalable way?

I took the liberty of assuming that scalable widgets won't use point/pixel sizes directly in the first place and they will use the layout provided font. If this is false, I'll check.

[Rossmaxx]

Like what I've said before, we've got quite a paradox now. The fonts look really nice but the rest of the hardcoded sized widgets don't match the good looking fonts.

[michaelgregorius]

Like what I've said before, we've got quite a paradox now. The fonts look really nice but the rest of the hardcoded sized widgets don't match the good looking fonts.

@Rossmaxx, what exactly do you mean? Can you post a screenshot that shows what you mean?

[michaelgregorius]

@Rossmaxx, I have checked out your branch and noticed that the note labels of the piano are clipped now:

It might be related to this comment: https://github.com/LMMS/lmms/pull/7493#discussion_r1761438091

[musikBear]

the hardcoded sized widgets don't match the good looking fonts

Ya VOLPAN and the texts are a bit up in the dials.. But cost against benefit -Imo its better that the text is readable

[michaelgregorius]

the hardcoded sized widgets don't match the good looking fonts

Ya VOLPAN and the texts are a bit up in the dials.. But cost against benefit -Imo its better that the text is readable

That's because the Knob class does not really take the label into account when it computes its new size when some new label text is set. See here: https://github.com/LMMS/lmms/blob/588aab338968864ae250709f3312fcd9bb481430/src/gui/widgets/Knob.cpp#L141

It computes the final height of the whole widget by taking the height of the knob pixmap, i.e. the dial, and then adding 10 pixels to that. The label is drawn with the base line at height - 2 which means that there are 8 pixels left for the label until it starts to wander into the knob area. Unfortunately, changing anything in that area would likely break lots of manual layouts where knobs are used.

Due to the current implementation it's also not possible to set individual font sizes for different knob instances because the font size is hard coded in the Knob class itself, i.e. it does not evaluate the currently set font and then renders itself accordingly like for example most Qt widgets do. This means that it's not even possible to instruct the knob to use a smaller font size for the "VOL" and "PAN" labels in the track widgets so that it looks better.

[michaelgregorius]

Here are some results how it would look if Knob::setLabel was fixed as follows:

void Knob::setLabel( const QString & txt )
{
    m_label = txt;
    m_isHtmlLabel = false;
    if( m_knobPixmap )
    {
        auto f = adjustedToPixelSize(font(), SMALL_FONT_SIZE);
        auto fm = QFontMetrics(f);

        const int width = qMax<int>(m_knobPixmap->width(), horizontalAdvance(fm, m_label));
        const int height = m_knobPixmap->height() + fm.height();

        setFixedSize(width, height);
    }

    update();
}

The instrument track looks as follows:

The sample track shows the cutoff that happens with the default height of the track whereas the 3OSC track shows a slightly enlarged track.

As predicted it also breaks some layouts:

[Rossmaxx]

I've shrinked the overall fonts to a bit smaller values to accomodate sizes of other widgets. This might give up a bit of readability but it brings a level of consistency between widgets. I've also shrunk knob labels a bit.

[Rossmaxx]

@michaelgregorius regarding knobs, I believe there's a QDial that we can use to replace our custom knobs. It's beyond scope of this PR but can you see how it goes please?

[Rossmaxx]

https://github.com/LMMS/lmms/actions/runs/10897193378

artefacts

[michaelgregorius]

@Rossmaxx, there's one last remark. I think after that it would be good to go for me.

[michaelgregorius]

@michaelgregorius regarding knobs, I believe there's a QDial that we can use to replace our custom knobs. It's beyond scope of this PR but can you see how it goes please?

The dial uses an int interface so it's not a suitable replacement for the LMMS Knob class with the float interface. I think the Knob should either be adjusted so that it can be used with different font sizes or a new knob class should be added. The latter should then offer more flexibility and a more correct implementation. It could then be used in contexts that use layouts and that are not as hard coded as many other places in the software.

I think I already have an idea for an improvement of the existing class and might give it a try once this PR is merged.

[michaelgregorius]

@Rossmaxx, is this ready to merge or do you prefer to wait for another review?

[messmerd]

Here's how it looks for me now on Linux Mint 22. My screen's DPI is 96x96.

[Rossmaxx]

It's actually ready for merge but it would help if more people reviewed the fix.

[messmerd]

The "Log. scale" text in Vectorscope is a little clipped on the end

[Rossmaxx]

@messmerd I didn't notice that. Can you find any other such clippings, so that I can sort those out before merge.

But fixing that with shrinking font size won't help as the font there is already too small.

[michaelgregorius]

The "Log. scale" text in Vectorscope is a little clipped on the end [...]

Fixed with commit 0823bf174f9.

[Rossmaxx]

I'll merge this within 2 days if no one objects.

[Rossmaxx]

Latest builds for testing. https://github.com/LMMS/lmms/actions/runs/11036192688 @musikBear @bratpeki wanna test?

[musikBear]

Latest builds for testing. https://github.com/LMMS/lmms/actions/runs/11036192688 @musikBear @bratpeki wanna test?

I get too much clutter installing/ uninstalling (rinse repeat) For next to daily tests of new versions we need a way to not need to install / uninstall over and over -iow : we need a zip-version as in #7006 Tresf meant that 7zip could open lmms.exe, i could not do that.

[bratpeki]

@bratpeki wanna test?

Yeah, sure, as soon as I get my laptop set up.

[Rossmaxx]

I get too much clutter installing/ uninstalling (rinse repeat) For next to daily tests of new versions we need a way to not need to install / uninstall over and over -iow : we need a zip-version as in #7006 Tresf meant that 7zip could open lmms.exe, i could not do that.

I can understand your concern. You actually misunderstood the 7zip method. You actually unzip the installer and you can go into the unzipped folder and run lmms.exe. If you have any more doubts, feel free to ask.