Fix #5088: Numbers cut off at bottom of Spectrum Analyzer

[michaelgregorius]

Fix for #5088. Fix the problem of numbers that are cut of at the bottom of the Spectrum Analyzer. The changes consist of the following:

• Measure the maximum height needed to render the frequency ticks and adjust the bottom of the rectangle where the spectrum is rendered accordingly.
• Measure the maximum width needed to render the amplitude ticks and adjust the left and right margin of the rectangle where the spectrum is rendered accordingly.
• Adjust the code that renders the cursor text to measure the text that will be rendered so that it can be positioned correctly.

Technical changes:

• Add the two helper methods renderLogarithmicAmpTics and getAmpTicsToRender.
• Make the bottom margin a member.

[he29-net]

I can confirm this workaround is working on my setup -- both bottom and right labels are no longer cut off because of the Qt scaling bug. Although the same problem is still present in the SaWaterfallView widget:

Also, as illustrated, the workaround only works for relatively low DPI and if the scaling bug does not manifest in the worst possible way; in my case, if I do not force QT_FONT_DPI to 96, the result still looks quite bad.

The one (major) problem I have with this fix / workaround is that SaWaterfallView and SaSpectrumView no longer have matching width. They absolutely must be the same, because a) SaWaterfallView does not have its own frequency labels (they would be redundant) and b) as you can see, the 10 kHz peak does not match the peak in the waterfall. This should be kept in mind when implementing the change for the waterfall widget.

Additionally, the bottom margin may be a tiny bit too large -- the gap between X labels and the waterfall should be kept as small as possible, since they are meant to be shared.

Btw., I did not look at the code in much detail, but I would prefer using better names for fm and fsize etc. -- it's not clear what they mean from the name alone. font_metric and font_size are not that long and it makes the code more readable, since you don't have to look up the variable definitions to figure out what they mean.

For the helper functions: adding renderLogarithmicAmpTics() seems a bit counterproductive -- the name suggests it renders the tics, which is not the case. Accessing m_controls->m_logYModel.value() directly to check of log. scale is enabled was much clearer, IMO.
For getAmpTicsToRender(): if you want to add this one, I suggest also adding the same thing for the FreqTics. Having code behave differently for each axis (one using pointer, one using reference) would be pretty inconsistent and confusing for anyone that opens the file 10 years from now. :)

[michaelgregorius]

Hi @he29-net,

thanks for the code review! I have added some of the changes that you have proposed:

• renderLogarithmicAmpTics was renamed to isRenderLogarithmicAmpTics to better indicate that it does not render the amp tics.
• The method isRenderLogarithmicFrequencyTics and getFrequencyTicsToRender have been added so that the code is consistent with the amp tics.

I have indeed missed the waterfall view. The problem of the shared scale should be solved by giving the waterfall view its own scale in my opinion. The waterfall view should be an independent widget of its own and its correct functionality should not dependend on that there's a spectrum view above it with a frequency range. At one point we might for example decide that it should be possible to show the waterfall view without the spectrum view. I therefore propose the following solution:

• Render frequency labels for the waterfall view.
• Render the labels on top, i.e. inside, of the spectrum and waterfall views. This way there'd be more space for the spectrum and waterfall.
• Use smaller fonts for the labels.

[he29-net]

It was meant mainly as a functional review.. ^^; But the changes look good. 👍 Just the names may be at the opposite extreme now -- too long? :D E.g. isRenderLogarithmicFrequencyTics() could have been shortened to isFreqScaleLogarithmic(). But that's a small detail.

Even if waterfall gets its own scale, it will still visually look bad if the edges and peaks do not line up, so I'm not fan of "solving" it just using extra set of labels. These widgets were made as components of one plugin, they are not meant to be used individually. That's why I used fixed sizes to get pixel-perfect alignment and shared axis labels to save space -- I tried to optimize for the final result, not the individual widgets.

Not having scale labels for waterfall display is a valid point though -- even now, you can use the QSplitter handles to hide the config and spectrum display section, so there could be spectrogram with no labels. I did not consider it a big issue, since not many people will do it and it seemed more important to keep the analyzer as compact as possible, because it already takes more space than the old one.

If there should be frequency labels for waterfall, I would vote for adding them to the bottom. Vast majority of all graphs uses the left + bottom labels convention, and then maybe additional ones at the right and, sometimes, at the top.

Rendering labels inside the graph seem like especially bad idea to me -- it can a) cover parts of the graph if there is a weak signal and b) make the labels unreadable if there is any signal (white text on light blue background etc.).

When scaled properly, the fonts are already quite small and they match other font sizes, like track names. Making them smaller may hurt readability, although it's true that single digits are probably easy to read so it may not be a big problem.

[michaelgregorius]

Hi @he29-net,

the size of the labels is relative. 😃 Here's how the same area looks on my machine:

On my machine the labels of the spectrum analyzer are bigger than the labels of the tracks. I haven't explicitly checked the code but I assume that the difference comes from the fact that the font size of the tracks is defined in absolute pixels which also makes them look rather small on my screen. The spectrum analyzer on the other hand defines the font size as points which is why the fonts get scaled accordingly on displays with higher DPI.

Concerning the rendering of labels on the spectrum I guess pictures are worth a thousand words. 😃 Here's the result of some experimentation:

As you can see I have added some transparent areas to give the labels some contrast with regards to the spectrum itself. I think this approach might also look nicer without the right amplitude labels. The rendering is done in the following order by the way:

1. Grid
2. Spectrum
3. Labels

The labels become a bit harder to read if the stereo option is active:

But I think this could be fixed.

And here's a mono display with a bit less transparency:

The advantage of such an approach would be that the spectrum and the waterfall would be of the same size and therefore their frequency ranges would line up again (albeit still implicitly). The only thing that might differ is the width of the transparent areas.

[he29-net]

the size of the labels is relative

Hmm.. 🤔 The Qt scaling is really quite a mess. In my case, when I have the QT_FONT_DPI set to 96, everything scales all right, including the track names. Maybe it's as you say -- some fonts are in "pixels", that are by default treated as "logical pixels", so Qt just scales it up based on DPI of the monitor. And some fonts are using a different metric, that gets first scaled using the logical → device-dependent pixel conversion, and then again the metric itself is scaled by the ratio given by QT_FONT_DPI, producing the double-scale bug..

Here's the result of some experimentation:

The labels-inside version actually looks much better than I imagined, nice work! It will probably need some refinement, but I really like how it uses all available space. And as you say, it even fixes the width variability, so no objections for taking this route. 👍

The 10 and 20k Hz extremes are probably not much concern overall. The right side is probably more important than the low-end, so hiding the right axis may be beneficial, but there is still the problem of clearly (but not too in-your-face) indicating the left/right channel colors..
I'm still a bit worried about small signals (and linear scale) covered at the bottom, though. I would definitely prefer the lighter, more transparent version -- maybe the font readability could be protected by making it a bit thicker? Like something between normal and bold, if it exists?

Also the numbers in corner look pretty bad and it's not clear to which axis they belong; maybe it would be better to even drop them completely. Given that they require special alignment behavior, it may even make the code less complicated. And cursor behavior should be probably also checked after all the changes. That's currently all the issues I can think of..

[Reflexe]

Since you use std::pair<int, std::string> many times, I suggest you make a typedef of it (something like FrequencyAndText), and the same for std::pair<float, std::string>.

Also, is there a reason for using std::string instead of QString?

[he29-net]

Using std::string would be mainly my doing, I used it everywhere in the code. I never worked with Qt before writing the analyzer and I didn't see any reason to use some weird non-standard data types when the std library works just fine. The same goes for containers (e.g. std::vector and not QVector). Maybe there is some reason why Qt had to reinvent everything (to the point you could almost call the language Qt++), but to this day the reason eludes me. :)

[Reflexe]

In most cases it has the same interface (and more), In this case you have to manually convert it using QString::fromStdString which is just redundant.

[Reflexe]

Auto?

[Reflexe]

Is there any reason for not using auto?

[michaelgregorius]

Closing this pull request as the problem cannot be reproduced anymore in master (see here).