Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font size adjustments #7185

Merged
merged 10 commits into from
Apr 4, 2024

Conversation

michaelgregorius
Copy link
Contributor

@michaelgregorius michaelgregorius commented Mar 31, 2024

This is a proposed fix for the scalability issues reported in #7178. It also helps with the migration to Qt6 as reported in #6614.

It renames pointSize to adjustedToPixelSize and changes the implementation such that the function sets the font size in pixels instead of points.

This is done under the assumption that LMMS wants to support the model where users with HiDPI screens will set a global fractional scaling factor that's larger than 1. In that case text that's rendered in pixel sizes will be scaled as well and a text that will look good at 96 DPI will also look good at a higher DPI if scaled accordingly.

Example: A user has a display with 120 DPI. If they set the global scaling factor to 120 DPI / 96 DPI = 1.25 then everything is scaled accordingly and the GUI elements including text should have the same visual appearance, size and legibility.

Other things done

Go through all files that have been adjusted by the PR and check the following aspects:

  • If it is a home-grown widget check if it draws itself at fixed sizes. If that's the case potentially adjust the pixel sizes of the used fonts until they are large enough, legible and it looks good overall.
  • Consider to switch some calls to adjustedToPixelSize with setting the font in point sizes where it potentially does not matter.
  • There are some calls to adjustedToPixelSize which use float values which are not integer. These will be implicitly cast to integer. Adjust all places to appropriate integer values to get a consistent and non-surprising behavior.
  • Check if some adjustments override the pixel font sizes of underlying elements like for example combo boxes, etc. Potentially remove these overrides to ensure that everything looks consistent.

Make the function `pointSize` set the font size in pixels. This works if the intended model is that users use global fractional scaling. In that case pixel sized fonts are also scaled so that they should stay legible for different screen sizes and pixel densities.
Rename `pointSize` to `adjustedToPixelSize` because that's what it does not. It returns a font adjusted to a given pixel size.
Remove all includes that can be removed without breaking the build.
Rename `fontPointer` to `font` because it's not a pointer but a copy.

Rename `fontSize` to simply `size`.
@michaelgregorius
Copy link
Contributor Author

@Rossmaxx, this is a WIP of my proposed solution. I hinges on the assumption that global fractional scaling is the way forward.

At a certain point the include of QApplication should be removed as well and all places that import gui_templates.h and rely on the include should be adjusted as needed.

Adjust the plugins with regards to the use of `adjustedToPixelSize`.

Remove the explicit setting of the font size of combo boxes in the following places to make the combo boxes consistent:
* `AudioFileProcessorView.cpp`
* `DualFilterControlDialog.cpp`
* `Monstro.cpp` (does not even seem to use text)
* `Mallets.cpp`

Remove calls to `adjustedToPixelSize` in the following places because they can deal with different font sizes:
* `LadspaBrowser.cpp`

Set an explicit point sized font size for the "Show GUI" button in `ZynAddSubFx.cpp`

Increase the font size of the buttons in the Vestige plugin and reduce code repetition by introducing a single variable for the font size.

I was not able to find out where the font in `VstEffectControlDialog.cpp` is shown. So it is left as is for now.
Adjust the font sizes in the area of GUI editors and instruments.

Increase the font size to 10 pixels in the following places:
* Effect view: "Controls" button and the display of the effect name at the bottom
* Automation editor: Min and max value display to the left of the editor
* InstrumentFunctionViews: Labels "Chord:", "Direction:" and "Mode:"
* InstrumentMidiIOView: Message display "Specify the velocity normalization base for MIDI-based instruments at 100% note velocity."
* InstrumentSoundShapingView: Message display "Envelopes, LFOs and filters are not supported by the current instrument."
* InstrumentTuningView: Message display "Enables the use of global transposition"

Increase the font size to 12 pixels in the mixer channel view, i.e. the display of the channel name.

Render messages in system font size in the following areas because there should be enough space for almost all sizes:
* Automation editor: Message display "Please open an automation clip by double-clicking on it!"
* Piano roll: Message display "Please open a clip by double-clicking on it!"

Use the application font for the line edit that can be used to change the instrument name.

Remove override to set the font size for LED check boxes in:
* EnvelopeAndLfoView: Labels "FREQ x 100" and "MODULATE ENV AMOUNT"

Remove override to set the font size for combo boxes in:
* InstrumentSoundShapingView: Filter combo box
Adjust the font sizes in the area of the custom GUI widgets.

Increase the pixel font size to 10 pixels in the following classes:
* `ComboBox`
* `GroupBox`
* `Knob`
* `LcdFloatSpinBox`
* `LcdWidget`
* `LedCheckBox`
* `Oscilloscope`: Display of "Click to enable"
* `TabWidget`

Shorten the text in `EnvelopeAndLfoView` from "MODULATE ENV AMOUNT" to "MOD ENV AMOUNT" to make it fit with the new font size of `LedCheckBox`.

Remove the setting of the font size in pixels from `MeterDialog` because it's displayed in a layout and can accommodate all font sizes. Note: the dialog can be triggered from a LADSPA plugin with tempo sync, e.g. "Allpass delay line". Right click on the time parameter and select "Tempo Sync > Custom..." from the context menu.

Remove the setting of the font size in `TabBar` as none of the added `TabButton` instances displays text in the first place.

Remove the setting of the font size in `TabWidget::addTab` because the font size is already set in the contructor. It would be an unexpected size effect of setting a tab anyway. Remove a duplicate call to setting the font size in `TabWidget::paintEvent`.
@michaelgregorius michaelgregorius changed the title WIP: Font size adjustments Font size adjustments Mar 31, 2024
@michaelgregorius michaelgregorius linked an issue Mar 31, 2024 that may be closed by this pull request
@michaelgregorius
Copy link
Contributor Author

@Rossmaxx, I have tested the current state of this pull request under Wayland and it fixes the issue that I have reported in #7178.

@Rossmaxx
Copy link
Contributor

Good to know. LGTM

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Apr 1, 2024

Also, you forgot to remove the include from all places where the function is not used anymore.

Remove unnecessary includes of `gui_templates.h`.
@michaelgregorius
Copy link
Contributor Author

Also, you forgot to remove the include from all places where the function is not used anymore.

Good catch, @Rossmaxx! Done with commit 0c787a9.

@michaelgregorius
Copy link
Contributor Author

@Veratil, @messmerd, @sakertooth, requesting a review from you as you have also been active in #7159 which in the end caused #7178.

src/gui/editors/PianoRoll.cpp Show resolved Hide resolved
src/gui/editors/AutomationEditor.cpp Show resolved Hide resolved
src/gui/EffectView.cpp Show resolved Hide resolved
Directly use `setPixelSize` when drawing the "Note Velocity" and "Note Panning" strings as they will likely never be drawn using point sizes.

Remove a misleading comment in `AutomationEditor`.
@enp2s0
Copy link
Contributor

enp2s0 commented Apr 4, 2024

current master is close to unusable for me because of this issue. LGTM.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Apr 4, 2024

Forgot to remove the include in pianoroll.cpp

@michaelgregorius
Copy link
Contributor Author

Forgot to remove the include in pianoroll.cpp

Fixed with commit 724dfe5.

current master is close to unusable for me because of this issue. LGTM.

Yes, I think we should really get this merged.

@michaelgregorius
Copy link
Contributor Author

Do you approve this PR, @Veratil?

Copy link
Contributor

@Veratil Veratil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking past a few style fixups so we can just get this through.

@michaelgregorius michaelgregorius merged commit 20fec28 into LMMS:master Apr 4, 2024
9 checks passed
@michaelgregorius michaelgregorius deleted the FontSizeAdjustments branch April 4, 2024 19:45
@michaelgregorius
Copy link
Contributor Author

@enp2s0: merged. Thanks to everyone involved!

@michaelgregorius michaelgregorius mentioned this pull request Apr 10, 2024
25 tasks
enp2s0 pushed a commit to enp2s0/lmms that referenced this pull request Apr 12, 2024
Adjust and rename the function `pointSize` so that it sets the font size in pixels. Rename `pointSize` to `adjustedToPixelSize` because that's what it does now. It returns a font adjusted to a given pixel size. Rename `fontPointer` to `font` because it's not a pointer but a copy. Rename `fontSize` to simply `size`.

This works if the intended model is that users use global fractional scaling. In that case pixel sized fonts are also scaled so that they should stay legible for different screen sizes and pixel densities.

## Adjust plugins with regards to adjustedToPixelSize

Adjust the plugins with regards to the use of `adjustedToPixelSize`.

Remove the explicit setting of the font size of combo boxes in the following places to make the combo boxes consistent:
* `AudioFileProcessorView.cpp`
* `DualFilterControlDialog.cpp`
* `Monstro.cpp` (does not even seem to use text)
* `Mallets.cpp`

Remove calls to `adjustedToPixelSize` in the following places because they can deal with different font sizes:
* `LadspaBrowser.cpp`

Set an explicit point sized font size for the "Show GUI" button in `ZynAddSubFx.cpp`

Increase the font size of the buttons in the Vestige plugin and reduce code repetition by introducing a single variable for the font size.

I was not able to find out where the font in `VstEffectControlDialog.cpp` is shown. So it is left as is for now.

## Adjust the font sizes in the area of GUI editors and instruments.

Increase the font size to 10 pixels in the following places:
* Effect view: "Controls" button and the display of the effect name at the bottom
* Automation editor: Min and max value display to the left of the editor
* InstrumentFunctionViews: Labels "Chord:", "Direction:" and "Mode:"
* InstrumentMidiIOView: Message display "Specify the velocity normalization base for MIDI-based instruments at 100% note velocity."
* InstrumentSoundShapingView: Message display "Envelopes, LFOs and filters are not supported by the current instrument."
* InstrumentTuningView: Message display "Enables the use of global transposition"

Increase the font size to 12 pixels in the mixer channel view, i.e. the display of the channel name.

Render messages in system font size in the following areas because there should be enough space for almost all sizes:
* Automation editor: Message display "Please open an automation clip by double-clicking on it!"
* Piano roll: Message display "Please open a clip by double-clicking on it!"

Use the application font for the line edit that can be used to change the instrument name.

Remove overrides which explicitly set the font size for LED check boxes in:
* EnvelopeAndLfoView: Labels "FREQ x 100" and "MODULATE ENV AMOUNT"

Remove overrides which explicitly set the font size for combo boxes in:
* InstrumentSoundShapingView: Filter combo box

## Adjust font sizes in widgets

Adjust the font sizes in the area of the custom GUI widgets.

Increase and unify the pixel font size to 10 pixels in the following classes:
* `ComboBox`
* `GroupBox`
* `Knob`
* `LcdFloatSpinBox`
* `LcdWidget`
* `LedCheckBox`
* `Oscilloscope`: Display of "Click to enable"
* `TabWidget`

Shorten the text in `EnvelopeAndLfoView` from "MODULATE ENV AMOUNT" to "MOD ENV AMOUNT" to make it fit with the new font size of `LedCheckBox`.

Remove the setting of the font size in pixels from `MeterDialog` because it's displayed in a layout and can accommodate all font sizes. Note: the dialog can be triggered from a LADSPA plugin with tempo sync, e.g. "Allpass delay line". Right click on the time parameter and select "Tempo Sync > Custom..." from the context menu.

Remove the setting of the font size in `TabBar` as none of the added `TabButton` instances displays text in the first place.

Remove the setting of the font size in `TabWidget::addTab` because the font size is already set in the constructor. It would be an unexpected size effect of setting a tab anyway. Remove a duplicate call to setting the font size in `TabWidget::paintEvent`.

Remove unnecessary includes of `gui_templates.h` wherever this is possible now.

## Direct use of setPixelSize

Directly use `setPixelSize` when drawing the "Note Velocity" and "Note Panning" strings as they will likely never be drawn using point sizes.
@messmerd
Copy link
Member

messmerd commented Sep 7, 2024

I believe it was this PR that started making a lot of text throughout LMMS smaller for me on Linux. I'm not usually picky about these sort of cosmetic changes, but some text is much more difficult to read now.

For example, VST preset names and other text (before/after):
beforeafter

@michaelgregorius
Copy link
Contributor Author

@messmerd, before this PR LMMS looked as follows under Wayland:

WaylandFontProblems

The reason was that the function pointSize in gui_templates.h was doing some "magic" which broke down under Wayland. It more or less was trying to do something that should be the concern of the GUI toolkit or desktop environment and not of an application.

The current implementation assumes that LMMS is used in an environment that supports global fractional scaling and that users have set the scaling to a sensible value. Users that want to gain screen real estate by setting the scaling to 100% for HiDPI screens will have to do that at the cost of minuscule font sizes.

The global (fractional) scaling model

The mental model for global scaling is rather simple and works as follows.

Let's first assume that the global scaling is set to 100% and we instruct Qt to draw the following elements:

  • Rectangle of size 200x100: This results in a rectangle of 200x100 pixels being drawn.
  • Text with a pixel size of 10 pixels: The font will use up 10 pixels.
  • Text with a point size of 8 points: Qt will compute the resulting pixels using the DPI of the screen and draw a text with the computed pixel size. Let's assume it's 12 pixels in our example.

All pixel sizes are physical pixels regardless of DPI. On a HiDPI screen the 200x100 rectangle might look minuscule.

Now let's assume a scaling factor of 150%. This means that Qt will multiply all resulting pixel sizes with a factor of 1.5:

  • Rectangle of size 200x100: This results in a rectangle of 300x150 pixels being drawn.
  • Text with a pixel size of 10 pixels: The font will use up 15 pixels.
  • Text with a point size of 8 points: For simplicity we assume that Qt will compute the resulting pixels using the DPI of the screen for 100% scaling, i.e. 12 pixels in our example. The 12 pixels are then multiplied with 1.5 which yields 18 pixels.

This means that a rectangle of 200x100 that looks okay on a 96 DPI screen will appear more or less the same on a 144 DPI screen. If a font of 10 pixels is readable on a 96 DPI screen at 100% scaling it will have the same readability on a 144 DPI screen with 150% scaling.

Looking at the example it also becomes obvious why it's called "global scaling". With this mental model we can assume that Qt always works in 100% and then scales everything up by the global scaling factor. Something that looks good with regards to ratios of widget and text sizes at 100% will also look good at higher scaling factors. There is no element that will grow at a different ratio than the other elements.

Fixing the small fonts

One thing that we could do is to try to find out what effective scaling factors have been computed by the methods pointSize and pointSizeF at commit 2c6d88f (see gui_templates.h at the time of that commit).

My assumption is that from a logical point of view some DPI values cancel each other out in these functions. Looking at the previous versions of these functions there's a DPI value that's explicitly computed by the functions but there's also an implicit DPI value which is used by the called setPointSizeF method. These should somewhat cancel out with regards to the resulting pixel sizes because otherwise it would not have looked okay on different systems (under X11).

Then we could search the code for calls to adjustedToPixelSize and adjust the font size parameters by multiplying them with the scaling factor that we have determined. In my opinion we should do it like that and not put some magic scaling factor in adjustedToPixelSize itself or add a new method adjustedToPixelSizeScaled to the code to keep things simple and straight-forward. This will prevent other methods with "magic" code like the aforementioned pointSize in gui_templates.h.

What do you think?

@michaelgregorius
Copy link
Contributor Author

Another approach would be to temporarily adjust adjustedToPixelSize in gui_templates.h as follows and to try out different values for factor.

inline QFont adjustedToPixelSize(QFont font, int size)
{
	double factor = 1.25;
	font.setPixelSize(static_cast<int>(size * factor));
	return font;
}

Something the the area of 1.2 or 1.25 seems to be a good starting point.

@messmerd, can you apply this change and check at what value of factor the VST window resembles the one on the left of your screen shots?

Truncation effects

The method above will obviously truncate the resulting values which is more apparent due to the rather small input values for size which seem to be within the range 7 to 12 in the code. The following LibreOffice Calc sheet can be used to experiment with the effects of different values of factor. It produces the effective mappings from old size to new size when using the approach described above: 7185-AdjustedFontSizes.ods

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Sep 8, 2024

If it's possible, it's better to detect the device pixel ratio from Qt and multiply, even though ideally we shouldn't and leave it to qt, but looks like that's not going as expected.

Another alternative we can take is make the "factor" user define able via a settings combobox. That way, we can accomodate hardware of different ppi without relying on much outside help from Qt. The combobox text can be "Font scaling (change if text is pixelated)" or something similar.

@michaelgregorius
Copy link
Contributor Author

If it's possible, it's better to detect the device pixel ratio from Qt and multiply, even though ideally we shouldn't and leave it to qt, but looks like that's not going as expected.

Looking at the changes made in this PR it looks like this is not possible if Qt is run under Wayland. See the remark about devicePixelRatio in this comment: #7178 (comment). This is assuming that no Wayland fixes have made their way into Qt in the meantime.

Another alternative we can take is make the "factor" user define able via a settings combobox. That way, we can accomodate hardware of different ppi without relying on much outside help from Qt. The combobox text can be "Font scaling (change if text is pixelated)" or something similar.

This likely will not work in a satisfactory way either due to the many ways that the font size is currently set in the application:

  • Style sheets (likely also a mixture between pixel and point sizes)
  • Calls to adjustedToPixelSize
  • Direct calls to setPixelSize
  • Direct calls to setPointSize and setPointSizeF

There are only few occurrences of the latter two but the main discrepancy will likely show up due to setting some font sizes in the style sheets and some in code. Users would likely complain that the scaling factor will only apply to the elements for which the font size is set in code and that the elements which are set by style sheets are unaffected.

Users also would not really have many sensible options because setting the factor to a very high value will lead to effects like the ones shown in this comment: #6185 (comment).

As noted in the aforementioned comment there are many GUI elements with fixed pixel sizes (which will get scaled by the scaling factor). Fonts used in these elements should be defined in pixel sizes as well so that the ratio is always consistent when the element is scaled by the global scaling factor.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Sep 9, 2024

Style sheets (likely also a mixture between pixel and point sizes)
Calls to adjustedToPixelSize
Direct calls to setPixelSize
Direct calls to setPointSize and setPointSizeF

This is more of a consistency issue which should be solved too. I would go as far as to say, make the font sizes declare directly in code and avoid the stylesheet here entirely. That way, my proposal of setting font scaling in settings can work too. Thoughts?

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Sep 9, 2024

Looked at it a bit further and found this.

Screenshot_2024_0909_104452

This seems to make sense. So remapping the function to point sizes again should theoretically fix it.

@michaelgregorius
Copy link
Contributor Author

Looked at it a bit further and found this.

Screenshot_2024_0909_104452

This seems to make sense. So remapping the function to point sizes again should theoretically fix it.

@Rossmaxx, this will likely not fix it because it depends on the DPI of the output device how many pixels make up an inch. So it does not work in the way that the point size simply has to be multiplied with 4/3 to get the resulting pixel size of the font.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Sep 9, 2024

Gave it a further thought and it seems that qt automatically calculates everything for point size and sets it accordingly, atleast that's what it should. For both a 1080p at 1x and a 4k at 2x, it should look the same, even without processing on our end. The processing which was there in the first place before we started seems to be the cause. I did open a PR, and added you as a reviewer. Mind confirming my observations?

@michaelgregorius
Copy link
Contributor Author

@Rossmaxx, I have commented in your PR.

In my opinion the way forward is to adjust the pixel sizes used by adjustedToPixelSize so that they are large enough for the different widgets. 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. Point sizes are dependent on the DPI and might exceed the widget sizes on certain systems.

@messmerd
Copy link
Member

@michaelgregorius

@messmerd, can you apply this change and check at what value of factor the VST window resembles the one on the left of your screen shots?

Before this PR:
before

Master with factor=1.25:
master_w_1 25_factor

Master with factor=1.5:
master_w_1 5_factor

The factor=1.5 screenshot looks close to the original, except for the button font size which was increased, and the instrument window is wider for some reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fonts too large under Wayland
5 participants