Individual knob labels rendered using the widget font #7525
Conversation
Enable the knob to render the label correctly at arbitrary sizes if it's configured to do so. Otherwise it will render like before. The used mode is determined when a label is set for the knob because as long as the label is not set a knob does not have one anyway. The painting code now always renders the label with the font that's set for the widget. The are now two methods to set the label text. The new method `setLabelLegacy` renders the label as before albeit in a slightly adjusted implementation. It now sets the widget font to a fixed pixel size font and then calculates the new widget size as before, i.e. not really taking the size of the font into account. This might lead to overlaps if the font of the knob is large. The method `setLabel` now has an additional (temporary) parameter called `legacyMode`. It is by default set to `true` so that all knobs still render like they did before. This is implemented by delegating to `setLabelLegacy` if it's set to `true`. Otherwise the method calculates the new size of the widget by taking the pixmap and the label with the current font into account. Please note that as of now you must set the knob font before calling any of the methods that sets the label. This is because the new size is only calculated via these code paths. However, this is already much better than only being able to use one hard-coded label size for all knobs.
Switch all callers of `setLabel` to `setLabelLegacy` so that it becomes obvious in which places the old knob implementation is used.
Remove the parameter `legacyMode` from `setLabel`. Add the member `m_legacyMode` as it is needed in `Knob::changeEvent` so that we do not switch the behavior when the knob is enabled/disabled.
Extract `setLegacyMode` and `updateFixedSize`. Also add the getter `legacyMode`.
Introduce legacy knob builders and apply them to the plugins. There are three new methods which encapsulate how to create a corresponding legacy knob: * `Knob::buildLegacyKnob` * `CustomTextKnob::buildLegacyKnob` * `TempoSyncKnob::buildLegacyKnob` These methods set the knob they build to legacy mode and also set a label in legacy mode. The idea is to concentrate the relevant legacy code in these methods. They will later also be useful to quickly find all the places in the application where legacy knobs are used. The three methods are applied to the plugins directory. Most plugins use the build methods to build their knobs which also enables the removal of the explicit calls to `setLabelLegacy` from their code. For some plugins their implementations were adjusted so that they can deal with showing the labels in the applicaiton font, i.e. in the font size of the widget their are contained in. Most of the times this involved removing the fixed size and putting the elements in a layout (while also removing move calls). The following LMMS plugins use the application font now and are thus better readable: * Amplifier * BassBooster * Dispersion * Flanger * Peak Controller * ReverbSC * StereoEnhancer Effect The Vectorscope now shows the "Persist." label in the same size as the label of the check boxes. Setting an empty label was removed for Lb302.
Apply the legacy knob builders in the GUI components. Most components use the legacy knobs until they can be redesigned:
* Effect view ("W/D", "DECAY", "GATE")
* LFO Controller
* Instrument window
Everything related to the instrument window is for now kept to use the legacy knobs and should be adjusted at a later point to be more flexible:
* Envelope and LFO
* Functions
* Sound Shaping
The Instrument and sample track both use the legacy knobs for the "VOL" and "PAN" knobs. This might be adjusted later.
The following components now render the labels of their knobs with the application font size:
* MIDI CC Rack
* The class `LadspaControlView`, which is not in used anymore
Some vertical spacing was added to the MIDI CC Rack for slightly improved separation of the elements. The knobs are center aligned in the layout so that the transition between element under and over "CC 100" is cleaner. Setting the models in an explicit loop was removed and is now done when the knobs are created.
## Technical details
Extend `Knob::buildLegacyKnob` with the option to also set the name of the knob. This is needed for some changes in this PR.
The method `KnobControl::setText` now needs to distinguish between legacy mode and non-legacy mode.
Remove `Knob::setLabelLegacy`. Instead make sure that the `Knob` updates its size in the following situations: * The label is set. * The font changes. * Legacy mode is set or unset (already implemented). The handling of font changes has been added to `Knob::changeEvent`. The update in case of a changed label is added to `Knob::setLabel`. Every client that called `setLabelLegacy` now also sets the legacy font in size `SMALL_FONT_SIZE` as this was previously done in `setLabelLegacy`. The label is set via `setLabel` now. Both actions should result in an up-to-date size. The method `KnobControl::setText` now only sets the label via `setLabel`, assuming that the wrapped knob was already configured correctly to either be a legacy knob or not.
Use the descent of the font to calculate the distance of the base line from the bottom of the knob widget if we are not in legacy mode. In legacy mode we still assume the descent to have a value of 2, i.e. the base line will always have a distance of 2 from the bottom of the knob widget regardless of the used font. Also refactor the code a bit to make it more manageable.
Extract the method `Knob::drawLabel` which draws the label. It is called from `paintEvent`.
Use non-legacy knobs for the "VOL" and "PAN" knobs of the instrument and sample track. This gives a bit more separation between the knob and the label but to make this work the font size had to be decreased by one pixel.
Introduce the builder method `buildKnobWithSmallPixelFont` in `Knob` and `TempoSyncKnob`. It creates a non-legacy knob with a small pixel sized font, i.e. it still uses the small font but with a corrected size computation and corrected space between the knob and the label. It is mostly used in places with manual layouts where there's enough space to have the bit of extra space between the knob and the label. The following plugins use these knobs: * Bitcrush * Crossover EQ * Dual Filter * Dynamics Processor * Multitap Echo * Spectrum analyzer * Mallets * Waveshaper * ZynAddSubFx The "IN" and "OUT" label of the Bitcrush plugin use the default fixed font size now because the plugin uses a pixel based layout. Using the point based application font looked off. They are also used in the following component: * Effect view, i.e. the "W/D", "DECAY", "GATE" knobs of an effect * LFO Controller
Use non-legacy knobs with small pixel fonts for the parameter lists of VST instruments and effects. This is accomplished by renaming `CustomTextKnob::buildLegacyKnob` to `buildKnobWithSmallPixelFont` and removing the call to `setLegacyMode`.
Fix the handling of styled knobs which are created in non-legacy mode. Styled knobs do not use pixmaps and have no labels. Their size is set from the outside and they are painted within these limits. Hence we should not compute a new size from a pixmap and/or label in `Knob::updateFixedSize`. This fixes the following plugins: * FreeBoy * Kicker * Monstro * Nescaline * Opulenz * Organic * Sf2 Player * sfxr * SID * SlicerT * Triple * Watsyn * Xpressive The functionality broke with commit defa8c0180e. An alternative would have been to check for a styled knob in the contructor or `initUI` method and to set the legacy flag for these. The best solution would likely be to create an own class for styled knobs and to pull that functionality out of `Knob` because they somewhat clash in their handling.
|
@Rossmaxx, @sakertooth, don't be intimidated by the long blurb. 😅 You will find that most of the changes in the code are making calls to There are only very few places left that use the legacy knob. These could be addressed in further pull requests. It should also be considered to extract the "styled" knob into its own class because it is used and behaves quite differently from the pixmap knob. |
|
IIRC, @LostRobotMusic too has a knob refactor planned for his upcoming Disintegrator/Limiter plugins. So in that case, extraction of styled knobs into it's own class would be a better first step over this PR. I don't wanna understate your efforts. You did put a lot of effort but this knob refactor started from a wrong angle from my perspective. We should have started with a new dynamic knob and work our way towards replacing the existing knob. |
The styled knobs are only used in the context of plugins while the other knobs are used throughout the application. Therefore many places will benefit from these changes.
Let's first improve the existing stuff before thinking about other dynamic knobs which are "in the sky". They can still be implemented once this is merged. I am all for incremental improvements. |
I don't see how my plans to eventually clean up the code in a certain file is even remotely relevant to this PR. Please stop speaking for me. |
In this case, I'm trying to avoid duplication of efforts and also to make the knobs handle new upcoming changes (in this case your limiter/disintegrator) better. I'm not "speaking for you" this time. You did plan a clean up right? Which is what this PR is about.
I missed this point when I typed the previous comment. I don't really understand what the knob changes are even with the verbose description, but I'll try. |
Ok, let's clarify this for good. @LostRobotMusic, did you already start to make large changes to the
Do you understand the "TL;DR" at the top of the description, @Rossmaxx? To give you a specific example. Let's say that during your changes in #7493 someone would have said: "Everything looks fine except the knob labels in plugin 'XYZ' are too large now." In that case your only option would have been to globally decrease the font size in the I'd greatly appreciate if this could be reviewed as I have spent a large amount of the last weekend on this. It makes the GUI more flexible and I doubt that the I already wondered if the description might be intimidating while typing it. However, please just check the code. Much of it consists of repeated adjustments. The main things that need to be understood are the changes in the |
Rossmaxx
left a comment
Rossmaxx
left a comment
There was a problem hiding this comment.
This is what I felt for now.
Btw it seems like you switched to use layouts in many places. Is this a preparation to scalability?
Parameter whitespaces in the builder methods of `Knob`. Use `adjustedToPixelSize` in `InstrumentTrackView` and `SampleTrackView`.
|
Thanks for the code review so far, @Rossmaxx!
Yes, some plugins have been switched to layouts to make them more flexible, i.e. to allow rendering their knob labels in the application font. The plugin windows do not really need to be resizable but their implementation is more flexible now. If we for example decided to change the font sizes later this would be the only thing that's necessary and there would be no need to manually move around widgets. |
No, not in any way. All I said on Discord was that Knob.cpp is a mess and that I'll have to clean it up at some point to add a new styled knob type for two specific plugins. Those plans aren't at all relevant to this PR and will not conflict in any way or shape or form. |
Thanks for the heads up, @LostRobotMusic! By the way, I have also experimented with extracting a |
|
Can we merge this @Rossmaxx? |
|
I can't say anything without testing, and I can't test for 2 more days. I feel like me approving stuff looking at the diff is hurting the codebase more than helping. I would like to invite @messmerd as a second reviewer. |
Perhaps this wasn't obvious in my reply, but the constructor can remain in the old format, mark as deprecated and a new constructor can handle the new signature.
The PR doesn't make adjustments to the old behavior to the vast majority of these areas, so I will still have to disagree from the standpoint that touching them now has a benefit. |
I'd prefer to go on as follows. We merge this PR as is and if someone thinks that some things must be changed or further improved then they can do so in a new PR. IMO the code is good as is and it's definitively a step forward and an improvement from the current state in master. It brings new features and improvements to the interfaces. So why not simply take this incremental improvement? Otherwise I'd like to ask you for further clarifications so that there are no misunderstandings:
I still believe that the number of files that are touched cannot be reduced much. All the files that must be changed to further specify the new knob behavior would still appear changed and this should be the majority of the files. So it seems we are only talking about a hand full of files for the knobs that still want the legacy behavior. Please also note that with the current state the legacy knobs could be found very quickly, e.g. when trying to reduce them in follow-up issues, because you'd simply have to search for all occurrences of Last but not least, there's currently only one constructor which allows you to activate legacy behavior. All other constructors will activate the new mode. So in a way this constructor is the "the constructor" from above. |
I understand that often context gets lost in convo; I'll work towards a proposal in code and cross-link for feedback. |
If you want you can also create a pull request against this branch in my repo. I think that would be more convenient as we'd have direct results for evaluation. I could then merge your PR into this branch and then we merge it into master. What do you think? |
Also, y no merge? |
In the old implementation the Put differently, without this PR if a user asked "Can't you make the knob label in this plugin smaller/larger?" then the answer would have to be: "No, because this would change the appearance of all knobs in the whole application. It might not fit in some places, e.g. due to hard-coded layouts." This PR provides the possibility to adjust individual knobs without affecting all others, i.e. the answer to the users request now might become: "Yes, it's a great idea, we will make this change." Long story short, with this PR:
|
I am not sure if this question is directed towards me. As described above I'd be very happy about a merge. All that's missing is one approving commit of a reviewer with write access. I also offered my help in case the PR causes merge conflicts in some other PRs. For me the change set of this PR is of normal size for something that makes changes to an element that's used throughout the whole application. Even if it could be broken down somehow it would simply mean a quick series of PRs that would introduce the same change set anyway. I have to admit that I do not understand the hesitancy at all. This PR brings improvements and leaves the code in a better state than before. Changes are necessary for things to move forward and improve. If someone has an idea on how to improve it even more it could be done immediately after the merge. |
|
Hoping @sakertooth or @messmerd can take a look, then |
Oh, there's also this, my bad! |
Crosslinks:
Note, this code may have some mistakes in it with some poorly assumed constructors, but it's intended to show what I was describing. In hindsight, this only saves 4 touchd files, so it's not nearly as helpful as I had expected it to be. It also makes the API slightly less intuitive by using automagic-constructor (versus explicit flag). If any of it is useful, feel free to borrow from it. Even if it's not desired, using the enum as-intended may be desired for reasons that are subjective but should be self-explanatory. |
This statement is incorrect... It's dozens of lines (not hundreds). Sorry for the mistake. |
Rename the enum `Knob::Mode` and its values so that they better describe what they influence. `Knob::Mode` is renamed to `Knob::LabelRendering` to indicate that its value affects the label rendering. The value `NonLegacy` is now called `WidgetFont` to indicate that the knob uses the font settings of the widget when rendering the label. The value `Legacy` is now called `LegacyFixedFontSize` to indicate that it's a legacy behavior which uses a fixed font size that does not adhere to the font size that's set for the widget's font. Adjust all callers accordingly.
Document the constructor of `TempoSyncKnob` that can be used to set the label rendering to lecacy mode.
|
Thanks for the approval @tresf!
No problem! I knew how to read it. 🙂 I took inspiration from your branch to rename the enum I also started to add the deprecation warnings to the constructors like in your branch but then noticed that these constructors are used to create knobs without labels. Therefore I did not go forward with it. I intend to merge this PR soon, so that further improvements can build upon the changes made so far. Thanks to everyone involved! |
|
The linux-x86_64 build has some problems with the wine repositories which do not seem to match what the distribution expects: https://github.com/LMMS/lmms/actions/runs/15320424705/job/43114397704?pr=7525 I will check if I can get a good build later. Otherwise I will merge anyway because linux-x86_64 is what I build locally, so it should be good. |
This comment was marked as outdated.
This comment was marked as outdated.
The new names are much more intuitive. I still feel using a Boolean to set an enum is a mistake, especially if more label rendering techniques appear in the future (otherwise a Boolean should set a Boolean). I'm not strong enough opinionated to block the PR on this though. |
Rename `m_legacyMode` to `m_fixedFontSizeLabelRendering`. Rename the method `legacyMode` to `fixedFontSizeLabelRendering`. Rename `setLegacyMode` to `setFixedFontSizeLabelRendering`. Also remove the boolean parameter from the method as it was only called with `true` anyway.
I can completely understand that. The idea is that the enum allows to better search for the remaining places where the legacy behavior is activated. With the enum it's a simple search for There is a single constructor that allows to enable the legacy mode. However, trying to search for calls to that one constructor might also not yield good results depending on the IDE that's being used. Visual Studio Code usually returns all constructor calls for me when I search for a specific one, hence the enum even if it is a subpar solution. Ideally the remaining legacy knobs will be fixed one after the other so that at the end the enum can be removed completely again. |
Adjust the `Knob` class so that it defaults to taking the font size of
the knob's font into account when rendering its label. This allows to
use labels of different sizes for different knobs. Previously all knob
labels throughout the whole application were rendered with the same fixed
font size. Hence it was not possible to adjust the label size for a
single knob because this would have affected all other knobs as well.
The new implementation also allows the knobs to pick up CSS rules.
To be able to control the knob behavior two new constructors have been
added to the `Knob` class. Both constructors are concerned with creating
knobs with labels and therefore they directly take the label text as a
parameter. This removes numerous explicit calls to `setLabel` in the
code.
There is only one constructor that allows to switch between the new
behavior of taking the widget's font size into account and the old legacy
behavior of always rendering with the same fixed font size of
`SMALL_FONT_SIZE`. The parameter was modelled as an enum to make it
easier to find the remaining knob instances that use the legacy behavior.
This makes it easier to find them in case they should be removed as well.
In that case the string `LegacyFixedFontSize` can be searched.
The other new constructor allows to directly set the knob's (and
therefore the label's) font size to a pixel value.
Corresponding constructors have been added to `TempoSyncKnob`. The
constructors of `KnobControl` and `CustomTextKnob` have been adjusted to
take advantage of the new constructors. An usused constructor was removed
in `CustomTextKnob`.
The method `Knob::setLabel` has been made protected because labels should
now be set through the new constructors.
A new property called `m_fixedFontSizeLabelRendering` was added to the
`Knob` class. It controls how the labels are rendered. Fixed font size
legacy rendering can be activated by calling the protected method
`setFixedFontSizeLabelRendering`. The current setting can be queried via
`fixedFontSizeLabelRendering`.
## Changes in the plugins
Some plugins have been switched to using layouts to organize their
widgets so that they can accommodate for knobs with label sizes set by
the application font.
The fixed font size (legacy) rendering mode is still used in the
following places:
* EnvelopeAndLfoView
* InstrumentSoundShapingView
* InstrumentFunctionViews
* EffectView
* InstrumentTrackView
* SampleTrackView
* Delay plugin
* Carla plugin
# individual commit messages
What follows are the individual commit messages of the commits that have been squashed into one commit. They might help in case of more detailed investigations of how things came to be.
* Knob with correct label rendering
Enable the knob to render the label correctly at arbitrary sizes if it's configured to do so. Otherwise it will render like before. The used mode is determined when a label is set for the knob because as long as the label is not set a knob does not have one anyway.
The painting code now always renders the label with the font that's set for the widget.
The are now two methods to set the label text. The new method `setLabelLegacy` renders the label as before albeit in a slightly adjusted implementation. It now sets the widget font to a fixed pixel size font and then calculates the new widget size as before, i.e. not really taking the size of the font into account. This might lead to overlaps if the font of the knob is large.
The method `setLabel` now has an additional (temporary) parameter called `legacyMode`. It is by default set to `true` so that all knobs still render like they did before. This is implemented by delegating to `setLabelLegacy` if it's set to `true`. Otherwise the method calculates the new size of the widget by taking the pixmap and the label with the current font into account.
Please note that as of now you must set the knob font before calling any of the methods that sets the label. This is because the new size is only calculated via these code paths. However, this is already much better than only being able to use one hard-coded label size for all knobs.
* Switch from `setLabel` to `setLabelLegacy`
Switch all callers of `setLabel` to `setLabelLegacy` so that it becomes
obvious in which places the old knob implementation is used.
* Remove parameter `legacyMode` from `setLabel`
Remove the parameter `legacyMode` from `setLabel`. Add the member
`m_legacyMode` as it is needed in `Knob::changeEvent` so that we do not
switch the behavior when the knob is enabled/disabled.
* Extract methods
Extract `setLegacyMode` and `updateFixedSize`. Also add the getter `legacyMode`.
* Introduce legacy knob builders
Introduce legacy knob builders and apply them to the plugins. There are three new methods which encapsulate how to create a corresponding legacy knob:
* `Knob::buildLegacyKnob`
* `CustomTextKnob::buildLegacyKnob`
* `TempoSyncKnob::buildLegacyKnob`
These methods set the knob they build to legacy mode and also set a label in legacy mode. The idea is to concentrate the relevant legacy code in these methods. They will later also be useful to quickly find all the places in the application where legacy knobs are used.
The three methods are applied to the plugins directory. Most plugins use the build methods to build their knobs which also enables the removal of the explicit calls to `setLabelLegacy` from their code.
For some plugins their implementations were adjusted so that they can deal with showing the labels in the applicaiton font, i.e. in the font size of the widget their are contained in. Most of the times this involved removing the fixed size and putting the elements in a layout (while also removing move calls). The following LMMS plugins use the application font now and are thus better readable:
* Amplifier
* BassBooster
* Dispersion
* Flanger
* Peak Controller
* ReverbSC
* StereoEnhancer Effect
The Vectorscope now shows the "Persist." label in the same size as the label of the check boxes.
Setting an empty label was removed for Lb302.
* Legacy knob builders in GUI
Apply the legacy knob builders in the GUI components. Most components use the legacy knobs until they can be redesigned:
* Effect view ("W/D", "DECAY", "GATE")
* LFO Controller
* Instrument window
Everything related to the instrument window is for now kept to use the legacy knobs and should be adjusted at a later point to be more flexible:
* Envelope and LFO
* Functions
* Sound Shaping
The Instrument and sample track both use the legacy knobs for the "VOL" and "PAN" knobs. This might be adjusted later.
The following components now render the labels of their knobs with the application font size:
* MIDI CC Rack
* The class `LadspaControlView`, which is not in used anymore
Some vertical spacing was added to the MIDI CC Rack for slightly improved separation of the elements. The knobs are center aligned in the layout so that the transition between element under and over "CC 100" is cleaner. Setting the models in an explicit loop was removed and is now done when the knobs are created.
## Technical details
Extend `Knob::buildLegacyKnob` with the option to also set the name of the knob. This is needed for some changes in this PR.
The method `KnobControl::setText` now needs to distinguish between legacy mode and non-legacy mode.
* Remove `Knob::setLabelLegacy`
Remove `Knob::setLabelLegacy`. Instead make sure that the `Knob` updates its size in the following situations:
* The label is set.
* The font changes.
* Legacy mode is set or unset (already implemented).
The handling of font changes has been added to `Knob::changeEvent`. The update in case of a changed label is added to `Knob::setLabel`.
Every client that called `setLabelLegacy` now also sets the legacy font in size `SMALL_FONT_SIZE` as this was previously done in `setLabelLegacy`. The label is set via `setLabel` now. Both actions should result in an up-to-date size.
The method `KnobControl::setText` now only sets the label via `setLabel`, assuming that the wrapped knob was already configured correctly to either be a legacy knob or not.
* Use descent to calculate base line
Use the descent of the font to calculate the distance of the base line from the bottom of the knob widget if we are not in legacy mode. In legacy mode we still assume the descent to have a value of 2, i.e. the base line will always have a distance of 2 from the bottom of the knob widget regardless of the used font.
Also refactor the code a bit to make it more manageable.
* Extract `Knob::drawLabel`
Extract the method `Knob::drawLabel` which draws the label. It is called from `paintEvent`.
* Use non-legacy knobs for instrument and sample track
Use non-legacy knobs for the "VOL" and "PAN" knobs of the instrument and sample track. This gives a bit more separation between the knob and the label but to make this work the font size had to be decreased by one pixel.
* Introduce `buildKnobWithSmallPixelFont`
Introduce the builder method `buildKnobWithSmallPixelFont` in `Knob` and `TempoSyncKnob`. It creates a non-legacy knob with a small pixel sized font, i.e. it still uses the small font but with a corrected size computation and corrected space between the knob and the label. It is mostly used in places with manual layouts where there's enough space to have the bit of extra space between the knob and the label.
The following plugins use these knobs:
* Bitcrush
* Crossover EQ
* Dual Filter
* Dynamics Processor
* Multitap Echo
* Spectrum analyzer
* Mallets
* Waveshaper
* ZynAddSubFx
The "IN" and "OUT" label of the Bitcrush plugin use the default fixed font size now because the plugin uses a pixel based layout. Using the point based application font looked off.
They are also used in the following component:
* Effect view, i.e. the "W/D", "DECAY", "GATE" knobs of an effect
* LFO Controller
* Non-legacy knobs for VSTs
Use non-legacy knobs with small pixel fonts for the parameter lists of VST instruments and effects.
This is accomplished by renaming `CustomTextKnob::buildLegacyKnob` to `buildKnobWithSmallPixelFont` and removing the call to `setLegacyMode`.
* Fix styled knobs
Fix the handling of styled knobs which are created in non-legacy mode. Styled knobs do not use pixmaps and have no labels. Their size is set from the outside and they are painted within these limits. Hence we should not compute a new size from a pixmap and/or label in `Knob::updateFixedSize`.
This fixes the following plugins:
* FreeBoy
* Kicker
* Monstro
* Nescaline
* Opulenz
* Organic
* Sf2 Player
* sfxr
* SID
* SlicerT
* Triple
* Watsyn
* Xpressive
The functionality broke with commit defa8c0180e.
An alternative would have been to check for a styled knob in the contructor or `initUI` method and to set the legacy flag for these.
The best solution would likely be to create an own class for styled knobs and to pull that functionality out of `Knob` because they somewhat clash in their handling.
* Code review changes
Parameter whitespaces in the builder methods of `Knob`.
Use `adjustedToPixelSize` in `InstrumentTrackView` and `SampleTrackView`.
* Code review changes
Make the code that computes the new fixed size in legacy more readable
even if it is just legacy code that's was not touched. Add some code
documentation.
Other cosmetic changes:
* Whitespace adjustments
* Remove unused parameter in `paintEvent`
* Rename `knob_num` to `knobNum`
* Add documentation for legacy mode
Add some documentation which explains what the effects of legacy mode
are.
* Code review
Remove unnecessary dereference.
Also remove unncessary code repetition by introducing
`currentParamModel`.
* Decrease the label size of some knobs
Decrease the size of the following knob labels to 8 pixels:
* "VOL" and "PAN" in the instrument and sample track views
* "W/D", "DECAY" and "GATE" in the effect view
Technically this is accomplished by introducing
`Knob::buildKnobWithFixedPixelFont` and
`TempoSyncKnob::buildKnobWithFixedPixelFont`.
Both versions of `buildKnobWithSmallPixelFont` now also delegate to
the new methods.
* Adjustments to CrossoverEQControlDialog
Commit the adjustments that were done to `CrossoverEQControlDialog` which I had forgotten to add after fixing the merge.
* Fix formatting of CrossoverEQControlDialog
Fix the formatting of `CrossoverEQControlDialog` which got messed up after copying the code from the current version on GitHub.
* Code review changes
Use `std::max` instead of `qMax`. Remove some unnecessary whitespace.
* Protected legacy mode methods
Make `legacyMode` and `setLegacyMode` protected to ensure that legacy knobs can only be built using the factory method `buildLegacyKnob`. In the long term legacy mode should be removed.
* Code review: remove indexed access
The original request in the code review was to use `size_t` instead of
`uint32_t` in the for-loop. However it is possible to completely remove
the indexed access and to turn it into a simple iterated for-loop.
Also remove code repetition in the calculation of the maximum knob width
of the group. Use std::max instead of manual management.
* Fix u_int16_t to uint16_t
This should hopefully fix the WIndows builds.
* Fix AudioFileProcessor knobs
Fix a problem with the `AudioFileProcessorWaveView::knob` which is caused
by the fact the this knob uses the pixmap based knob type `Bright26`
without a label. Most other knobs that inherit from `Knob` set their knob
type to `Styled` which means that no pixmap is used to render the knob.
In the specific case the knob instance is created and the contructor
runs. In the constructor the AFP knob is set to a fixed size of (37,47).
However, at a later point the method `Knob::changeEvent` is triggered by
Qt due to a font change. This in turn calls `Knob::updateFixedSize` which
then recomputes the fixed size and effectively changes the width of the
knob to the width of the pixmap which is 27. Because the knob previously
was rendered centered with a width of 37 this means that the knob is now
effectively shifted by five pixels to the left.
This commit counters this effect by moving the affected AFP knobs five
pixels to the right. A visual difference between the fixed version and
the current master showed no differences. So this should fix the problem.
Because setting the knob to a fixed size of (37,47) does not have any
lasting effect anyway the code is removed from the constructor of the AFP
knob.
* Use legacy knobs in EffectView
* Legacy knobs for instrument & sample
Use legacy knobs for the instrument and sample track view ("VOL", "PAN").
* Add documentation to Knob builder methods
Add some documentation to the `Knob` builder methods. Mark `buildLegacyKnob` as deprecated and note that it should not be used in new code.
* Ensure legancy rendering for legacy knobs
Ensure that legacy knobs are always rendered at a size of 12 pixels,
i.e. `SMALL_FONT_SIZE`.
The previous implementation used the font metrics of the knob's current
font to compute the new fixed size and to render the label. It assumed
that the knob was created using `Knob::buildLegacyKnob` and that
therefore the font is set to 12 pixels. However, this meant that legacy
knobs can still be affected by style sheet settings. The following CSS
rule for example resulted in legacy knobs with a larger font size:
```
* { font-size: 18px; }
```
The fix is to use a font with a size of `SMALL_FONT_SIZE` when
calculating the fixed size of the `Knob` widget and when rendering it if
we are in legacy mode. This ensures that a legacy knob is unaffected by
CSS rules.
The non-legacy knob still uses the widget's font size and therefore it is
affected by CSS rules. However, this is a feature and not a bug because
when for example using a rule like the one above the knob does exactly
what it's asked to do.
* Remove unused constructor
Remove an unused constructor from CustomTextKnob
* Remove Knob::buildKnobWithSmallPixelFont
Remove the builder method `Knob::buildKnobWithSmallPixelFont` and replace
it with an equivalent new contstructor which takes the same arguments as
the build method.
* Remove Knob::buildKnobWithFixedPixelFont
Remove `Knob::buildKnobWithFixedPixelFont` as it is not used anymore.
Previously it was delegated to by the now removed method
`Knob::buildKnobWithSmallPixelFont`.
* Constructor for knobs with pixel size labels
Remove `TempoSyncKnob::TempoSyncKnob` and add an equivalent constructor.
Make `buildKnobWithSmallPixelFont` use the new constructor.
* Remove TempoSyncKnob::buildKnobWithSmallPixelFont
Remove `TempoSyncKnob::buildKnobWithSmallPixelFont` and make clients use
the new constructor.
* Remove CustomTextKnob::buildKnobWithSmallPixelFont
Remove `CustomTextKnob::buildKnobWithSmallPixelFont` and extend the
constructor so that it also takes a label. Previously all constructions
went through the build method and now all constructions use the extended
constructor.
* Knob constructors whichKnob constructors which take labels
Add constructors for `Knob` and `TempoSyncKnob` which also take the label
text. Make setLabel protected as most knobs should know their labels at
construction time.
This prevents "chatty" code like the following example:
```
Knob* knob = new Knob(KnobType::Bright26, this);
knob->setLabel("My label");
```
This now becomes a simple a one-liner:
```
Knob* knob = new Knob(KnobType::Bright26, "My label", this);
```
The constructor of `KnobControl` also had to be extended with the label
text because it cannot access the setLabel of the Knob that it manages.
However, it can pass the text during construction. Its implementation of
the virtual method `Control::setText` becomes empty due to this. The
`KnobControl` is currently only used in `Lv2ViewProc::Lv2ViewProc`. Here
the `KnobControl` is created by passing the port name into the
constructor. However, the virtual method `setText` is still called in
line 91 for all other implementations.
Add documentation for the constructors.
* Remove Knob::buildLegacyKnob
Remove `Knob::buildLegacyKnob` by extending the very similar constructor
with an enum that indicates whether the constructed `Knob` should be in
legacy mode or not. The default is to build a non-legacy `Knob`.
Wherever `Knob::buildLegacyKnob` was called the constructor is now called
with `Knob::Mode::Legacy` as the parameter. In some places where the
onject name is set `Knob::Mode::NonLegacy` has to be added explicitly.
* Remove TempoSyncKnob::buildLegacyKnob
Remove `TempoSyncKnob::buildLegacyKnob` by extending the very similar
constructor with an enum that indicates whether the constructed
`TempoSyncKnob` should be in legacy mode or not. The default is to
build a non-legacy `TempoSyncKnob`.
Wherever `TempoSyncKnob::buildLegacyKnob` was called the constructor is
now called with `Knob::Mode::Legacy` as the parameter.
* Vertical spacing for Peak Controller
Add a vertical spacing of 10 pixel between the knobs of the Peak Controller.
* Peak Controller: use default margins
Remove the specific call to `setContentsMargins` from the Peak Controller so that the main layout uses Qt's default margins.
Also remove the spacing again.
* Rename the enum `Knob::Mode`
Rename the enum `Knob::Mode` and its values so that they better describe
what they influence.
`Knob::Mode` is renamed to `Knob::LabelRendering` to indicate that its
value affects the label rendering.
The value `NonLegacy` is now called `WidgetFont` to indicate that the
knob uses the font settings of the widget when rendering the label.
The value `Legacy` is now called `LegacyFixedFontSize` to indicate that
it's a legacy behavior which uses a fixed font size that does not adhere
to the font size that's set for the widget's font.
Adjust all callers accordingly.
* Add TempoSyncKnob documentation
Document the constructor of `TempoSyncKnob` that can be used to set the
label rendering to lecacy mode.
* Name adjustments and parameter removal
Rename `m_legacyMode` to `m_fixedFontSizeLabelRendering`.
Rename the method `legacyMode` to `fixedFontSizeLabelRendering`.
Rename `setLegacyMode` to `setFixedFontSizeLabelRendering`. Also remove
the boolean parameter from the method as it was only called with `true`
anyway.
| void KnobControl::setText(const QString& text) | ||
| { | ||
| // For KnobControls the text is set in the constructor | ||
| // so we do nothing here | ||
| } |
There was a problem hiding this comment.
@michaelgregorius Is there a reason for this discrepancy? Why can't Knob::setLabel() be public rather than protected?
There was a problem hiding this comment.
The idea is to prevent lots of code that looks as follows:
auto knob = new Knob(...);
knob->setLabel("foo");
If you search the diff of this PR for setLabel you will find that there were lots and lots of these "noisy" calls.
The text of a knob is one of its defining properties, i.e. it can for example be seen directly in the user interface. Hence it should be provided in the constructor. Technically the setter could of course be made public, and one might argue that it should be for something what I called a "defining property" some lines ago, but so far it does not really seem to be necessary.
There was a problem hiding this comment.
I can see it being temporarily useful for finding and fixing instances of those "noisy" calls, but now that they are converted to set the label via the constructor, I think setLabel should be public again.
Benefits of doing this:
- The label can be changed without needing to construct a new Knob
- The incorrect behavior in
KnobControl::setTextcan be fixed - The related method
Knob::setHtmlTextis not the only one that is public
There was a problem hiding this comment.
I agree.
Your comment reminded me of the unused method Knob::setHtmlText which had a TODO for removal. I have just created pull request #7993 which removes it.
9 participants
What is this PR about?
Up until now it was only possible to change the size of the knob labels in the actual implementation of
Knob. This means that the changes affected all places where knobs are used and it was not possible to fix a specific instance of a knob. Also, if the font size was increase the label started to "bleed" into the knob because the size of the font was not taken into account when determining the size of theKnobinstance.This pull request enables the
Knobclass to render its label correctly at arbitrary sizes using the font that's set for the widget. This means that it's now also possible to render the labels using the application font and size. The knob can also be set into a legacy mode so that it will render itself like before, i.e. as "buggy" as described above.The left knob in the following image shows how the label rendering looked before this PR if the font size was increased (and it still looks like that in legacy mode):
The middle knob shows a knob with a "regular" font size and the right knob shows rendering at increased font size.
Click for (much) more details (including screenshots)
Knobs using the application font
Several elements now make use of the fact that the knob label can be rendered using the application font.
Plugins
The following plugins were adjusted so that they now use the application font to render their labels:
Other components
The following components now render the labels of their knobs with the application font size:
LadspaControlView(which is not in used anymore though)Non-legacy knobs with small font sizes
There are several places where the implementation of the knobs was switched to the non-legacy mode which gives better separation between the knob and the label. The font size was kept at a small size though. This was mostly done in places with manual layouts where there's enough space to have the bit of extra space between the knob and the label.
In most places this was accomplished by using the builder method
buildKnobWithSmallPixelFontwhich can be found forKnobandTempoSyncKnob.Plugins
The following plugins use these knobs:
Other components
They are also used in the following component:
buildKnobWithSmallPixelFont.Styled knobs
Styled knobs do not use pixmaps and have no labels. Their size is set from the outside and they are painted within these limits. Hence we should not compute a new size from a pixmap and/or label in
Knob::updateFixedSize.This mostly applies to the following plugins: FreeBoy, Kicker, Monstro, Nescaline, Opulenz, Organic, Sf2 Player, sfxr, SID, SlicerT, TripleOscillator, Watsyn, Xpressive.
The best solution would likely be to create an own class for styled knobs and to pull that functionality out of Knob because they somewhat clash in their handling.
Legacy knobs
Everything related to the instrument window for now uses the legacy knobs.
The Carla plugin also uses the legacy knobs as I was not able to test it due to crashes that existed before. The LMMS Delay also uses the legacy knobs because it is rather "crammed" already.
You can find the code that uses legacy knobs if you search for calls to the method
buildLegacyKnob. It's provided byKnob,CustomTextKnobandTempoSyncKnob.Other changes
The Vectorscope now shows the "Persist." label in the same size as the label of the check boxes.
Some vertical spacing was added to the MIDI CC Rack for slightly improved separation of the elements. The knobs are center aligned in the layout so that the transition between element under and over "CC 100" is cleaner. See the screenshot above. Setting the models in an explicit loop was removed and is now done when the knobs are created.
The "IN" and "OUT" label of the Bitcrush plugin use the default fixed font size now because the plugin uses a pixel based layout. Using the point based application font looked off. See the screenshot above for the updated view.
Technical details
Changes in the Knob class
Legacy mode
Add the member
m_legacyModeas it is needed in several places to do the right thing, e.g. in the update of the fixed size or the paining of the label. Add the getterlegacyModeand the settersetLegacyMode.Size updates
Make sure that the Knob updates its size in the following situations:
The handling of font changes has been added to Knob::changeEvent. The update in case of a changed label is added to Knob::setLabel.
Extract the method
updateFixedSize.Paining code
The painting code now always renders the label with the font that's set for the widget.
Extract the method
Knob::drawLabelwhich draws the label. It is called frompaintEvent.Use descent to calculate base line
Use the descent of the font to calculate the distance of the base line from the bottom of the knob widget if we are not in legacy mode. In legacy mode we still assume the descent to have a value of 2, i.e. the base line will always have a distance of 2 from the bottom of the knob widget regardless of the used font.
Other
Setting an empty label was removed for Lb302.