#1940 Qt terminal label textbox placement
Development version.
There was discussion on the gnuplot-beta list regarding the placement of textbox for labels in Qt terminal.
The attached patch are some changes to improve layout a little bit. I think it is going in the right direction, but it still needs a little adjustment. The main changes are
1) The use of QFontMetrics.width() rather than using the QGraphicsTextItem boundingRect. The documentation states that width() is more accurate.
2) Made the use of Qt:AlignRight, etc. more consistent and proper, which obviates the use of fudge factors 0.67 and 0.25.
http://doc.qt.io/qt-4.8/qfontmetrics.html#boundingRect-2
"If you want to know the advance width of the string (to layout a set of strings next to each other), use width() instead."
Note that width() takes a string input, but height() does not. Height is fixed. The consequence is that with a margin of 0, there is more space above/below text within the box than to the left/right. But I believe this could be adjusted if desired with use of ascent() and descent():
Here's the test script. Note that I'm just concentrating on non-enhanced text initially:
I claim this is an improvement because of the consistency of the right edge of the box. It is always a little smaller rather than randomly bigger/smaller as before the change. Also notice for my example there is seemingly extra space prior to the first character. If that space were removed, I think there'd be good alignement with the start of the string and the start box.
In some sense, it seems that the position of the box is more proper than that of the string (which I've not altered). The box is almost aligned with x=0, i.e., the margin turns out to be one pixel. If I force the margin to be 0 in the code, then the box does line up with x=0. Is there some way of controlling the margin from gnuplot command line?
1) "advance width" is jargon. It means "where the next character will start". That is different from "the farthest to the right that the current string extends".
2) The incremental bounding rectangle really does need to be composited with the previous state, not to replace it.
Try this for a test:
Last edit: Ethan Merritt 2017-07-05
Oh, that's what the |= is. I was thinking that was binary, but this is C++ and |= can be defined incremental/expansive bounding. Got it. Attached is patch that puts that back in place.
In doing a comparison of Qt and WXT terms, the WXT term does not have so much white space to the left of the text, as does Qt. I wondered for quite a while why this is. Commenting out the following in GEPutText case:
shows that the text doesn't even come close to lining up with 0,0 of the screen. I don't know if that is significant, but it seems odd.
There is this element of QtGnuplotScene::QtGnuplotScene:
But that doesn't seem to have much of an influence, as 10 on the expanded gnuplot scale amounts to no more than a pixel, typically.
Here's an added test case:
For me, the above example now has the extra white space on the right edge of the text. It doesn't seem like the text width is being reported too small, as that should result in the same box-to-text placement regardless of alignment.
Could there be an issue with the fact that positionText() is manual alignment, whereas the box is using Qt's alignment mechanism? Do we need to use Qt's alignment mechanism for fonts as well? See here (seems complicated):
http://www.cesarbs.org/blog/2011/05/30/aligning-text-in-qgraphicstextitem/
Attached is a screen shot of a similar example, but using "center" alignment:
This looks quite good to me. Do we agree that this kind of horizontal box-to-text ratio is acceptable, if only we could figure out the right/left alignment white space removal? I mean, it looks like it could use some more interior white space on the edges, horizontally, but that's a margin issue, right? Is there a way of controlling the interior margin from the command line? If not, we can always add a little extra via some margin fudge factor, but right now, it seems to me that alignment is the main problem.
For me the bottom line is that retrieving the bounding rectangle of an item is a primitive operation provided by Qt itself. If the bounding rectangle is wrong, that's a Qt bug.
Nothing here or in the long thread on the mailing list indicates a gnuplot bug to me.
But yes it is unfortunate that the text sample with a box around it produced by the "test" command does not reflect the true capabilities of the current terminal. Maybe it is time to revise or replace the elements shown by "test".
I think there are definitely several things not quite right here with Qt terminal. Here are two images of unpatched and patched code results.
Notice two things:
1) The patched code, i.e., using "width()" results in a much more accurate width for the box. Compare against the result of WXT terminal. I think the patched result is more in line with WXT behavior.
2) The unpatched code results in a bad fit for descending characters. Again, compare against WXT terminal. WXT terminal is doing something that Qt terminal currently isn't doing, it's adjusting the box based on the actual glyph contents, not just the fixed "height" that is being used in Qt patched code. So, one could argue that the patched code is merely adding more height--true, but it's better than current boxed behavior for descending glyphs.
So, I think what we are after for Qt is a behavior similar to WXT, that the actual contents of the text is used to compute width and height. Qt's "width()" seems to do that, but height() not. However, beyond that, the manner in which qt terminal is doing the alignment poses a problem. That is, because we have a separate alignment for the text and a separate alignment for the box, the two might not match by perfectly. Anyway...
3) Then there is the definite bug of Qt text alignment, i.e., the space to the left or to the right of the text string compared to the box, depending on the alignment used. That is the main visual problem.
EDIT: Adding test code for these plots:
Last edit: Dan Sebald 2017-07-05
Let me show an extreme case example of why QFontMetrics(m_font).width(text) is not the correct calculation. Here are the "as is" and "patched" outputs from your test script after modifying it to use a font for which "advance width" is very different from "rightmost extent".
As to the placement of the bottom of the box relative to the baseline or relative to the lowest descender - I understand your point. But to the best of my knowledge the convention is to position all text relative to the baseline. Otherwise if you place multiple text fragments at the same y coordinate the vertical position of the box wll vary depending on what characers appear in the text. If the height of the text box is not sufficient to clear the descenders then I think the best option is to increase the vertical margin until it does. Your example shows that maybe the default vertical margin should be larger.
Last edit: Ethan Merritt 2017-07-05
Good example. I would think that Qt font metrics should figure that out for the script font. Are you able to do a similar plot for the WXT terminal with this font? The extra space from "boundingRect()" is making this your counter-example look better; I don't think there is any added accuracy...
In an attempt to understand the left/right white space offset, I investigated positionText(textItem, point) and how that might be done differently. I think I see what a core issue is here regarding left/right alignment.
First, let me point out that positionText() is actually operating on QGraphicsItem (as opposed to QGraphicsTextItem), so that is can also use the nice object QtGnuplotEnhanced. I wondered why it seems that the enhanced text seems to do a bit better alignment. Well, this custom object looks at fragments and for each it uses the font metrics to derive the boundingRect() as opposed to Qt's QGraphicsTextItem mechanism:
QRectF QtGnuplotEnhancedFragment::boundingRect() const { QFontMetricsF metrics(m_font); return metrics.boundingRect(m_text); }positionText() changes the position of the item. And in the case of the QGraphicsTextItem, for which item->boundingRect() appears to be wider compared to using the font metric to get that quantity, the alignment left/right is going to add a little extra white space.
This would all be fine if it were the case that doing
item->setTransformOriginPoint(cx, cy); item->setRotation(-m_textAngle); item->setPos(point.x() - cx, point.y() - cy);would update the text item's boundingRect() and then we could get the text box dimensions right from boundingRect(). But it doesn't automatically do so. To do so would require (according to documentation):
item->prepareGeometryChange(); item->setTransformOriginPoint(cx, cy); item->setRotation(-m_textAngle); item->setPos(point.x() - cx, point.y() - cy);but prepareGeometryChange() is a protected function so won't compile properly.
Hence, as I said, rather than get the text box directly from the QGraphicsTextItem's, i.e.,
and draw that box (doing so results in all the text boxes appearing in the upper-left corner, i.e., 0,0), we have to do the following:
QRectF rect = textItem->boundingRect(); if (m_textAlignment == Qt::AlignCenter) { rect.moveCenter(point); rect.moveBottom(point.y()); } else if (m_textAlignment == Qt::AlignRight) rect.moveBottomRight(point); else rect.moveBottomLeft(point);But that extra alignment does not match the alignment done in positionText--just think about it a bit. What we really want is the new boundingRect() from the re-positioned text item. More than that, the above approach to aligning the text box from textItem->boundingRect() at this point in the code is improper in the sense that
without first doing
is undefined.
That is, technically, the text item's boundingRect is "undefined" by this action as described in the Qt documentation.
So, there is a core conflict here. The custom enhanced text item is sort of the way to address this. A proper solution here would be to derive another simple object from QGraphicsTextItem to allow proper repositioning and boundingRect. Maybe we could always use the current custom enhanced text item instead. Another thought is that maybe the proper way of repositioning the QGraphicsTextItem alignment is to place the text within one of the container classes that does alignment. Maybe that alignment container class internally will do the prepareGeometryChange() so that boundingRect is updated correctly and we can just use that.
I'll investigate...
You are missing the point. It's not that Qt gets the advance width wrong. It's that advance width is not the property you are looking for. See this diagram:
http://www.rpmseattle.com/of_note/wp-content/uploads/2016/07/violin-clef-metrics.jpg
I get it. We want the extent of the whole area, not just the width at the baseline which is useful for text layout. I pointed out such a thing with the descending characters. Neither the unpatched nor patched Qt terminal is computing that desired dimension in a tight sense (say, the way WXT is), but I think that information can be obtained.
However, the points above still hold, and the bigger issue is that no matter if there is some means of getting the proper extent of the string/font, i.e., the correct width and height, the method currently used for aligning/justifying the associated text box is not going to result in proper layout in the general sense. That is, we can't get the proper x,y position unless the QGraphicsTextItem indicates very accurately where the extent is.
Experimenting with qtcreator and its GUI builder, I don't see any Qt widgets that fit the bill. QLabel comes close alignment-wise, but there's no nice way of placing a box accurately around that text. This has to be done as a custom widget, i.e., a child object. I know what to do, but I'll have to come back to this.
You wrote:
"Otherwise if you place multiple text fragments at the same y coordinate the vertical position of the box wll vary depending on what characers appear in the text."
From the looks of it, WXT terminal is doing the above.
As for the test sample, yes, text with descending glyphs and italic or script would be nice. Maybe use a label mechanism. Probably too much change for current release.
Just posting some notes for reference here, just to confirm some simple method isn't being overlooked. There are some other border options at our disposal.
A QGraphicsTextItem will accept HTML via the setHTML(). That's interesting. Attached is a one line change that uses HTML to change the background color, and I also put an italic code in there to see what happens to the background border at the edges. The screen capture looks good. If the information of that yellow box could be gotten easily, it would be a good basis for a box placement. The information is there, we just need to know how to access it. (A derivative class is probably easiest.) Unfortunately, box placement around text (like the yellow backaground) isn't possible. There is box placement around paragraphs, then the text item gets placed within the box...that isn't quite what we want though--we'd like the text to be the anchor, not the box.
I was mistaken about QLabels. In qycreator it is possible to place a box around the QLabel. But it is similar to HTML in the sense that the text inside is not really the anchor for positioning. That text floats around inside the box, even though it can be aligned.
Last edit: Dan Sebald 2017-07-06
Update: I confirm that Qt 5.6.2 is less reliable for text bounding boxes than Qt 5.4. The amount of error varies with the font but I have not figured out if the bad cases are due to a particular font property. Note that I'm testing on a different OS version, so I suppose it is possible that the true difference is something other than the newer Qt version. But either way I don't see how we can do any better in gnuplot.
Give the attached patch a try. It works pretty well for me.
The main issue, as I see it, is that positioning a bounding box, no matter how its height and width are determined separately from the text as currently done isn't going to work. The graphics systems knows about font positioning, kerning, etc., so it is the one that can get a precise fit of the frame.
QGraphicsItem is a somewhat limited class, but it turns out there is a very convenient proxy system whereby widgets can be turned into QGraphicsWidget and Qt does the appropriate translation. This enables the use of the following important function:
which does away with all the rectangle alignment. Hence, a QLabel widget--which is derived from QFrame--can be put in a proxy. It's a relatively small change, and the nature of the QLabel is very good interior alignment, but naturally only center-alignment within the frame is of interest. What's more, being a QLabel there are some other resources like controlling the interior margin. However, the QLabel's frame is never used because of the |= rect design. It's the geometry of that frame that is important.
I'm curious to see if this works with your scripted font example, i.e., whether the QLabel is nice enough to situate a balanced fit.
A screenshot for the following script:
That's very clever. In my testing so far I see that this approach does include the descenders in the bounding box (like cairo), although just barely. It also includes slightly more horizontal space for some but not all fonts I've tried. But other than that I don't see any difference to the current code.
Of course, I'm not the one complaining about bounding box problems. I get essentially perfect results with Qt 5.4.2 and slightly poorer but still reasonable results with 5.6.2. The fonts where the latter is imperfect are still imperfect with your patch. Do you have a test case where your new approach produces a noticeably different result than current cvs (other than the font descenders)?
I can't seem to produce a plot near as bad as what I was seeing a week ago when the box was off so much that it landed on top of characters in a random fashion. Nor is there as much variance vertically either. Was there some kind of change made in the last week? My guess was that maybe font substitution caused computation problems.
The only difference I now see is much more consistent balance with the patch versus the current version.
The descenders, yes looks like just a single pixel of white between the character and the frame. I wonder if the extra room at the top is for umlauts and such. Anyway, that can be adjusted aposteriori once the aligned box geometry is known. It could be dynamic, too. We know what characters are the descenders and can search for those...but that might not be desirable in the case of enhanced text layout where there are multiple text fragments.
I'm at a loss as to why there seems to have been an improvement in alignment for the CVS version. I rolled back to the code of
but can't reproduce the text box offset. I noticed in the log that qt5 is in use, but I sort of remember qt4. I wondered if having done a re-configuration in the past week was an issue. But
still didn't produce plots with the text box overlapping the text.
Well, there's a patch here with a possible alternative if this issue re-emerges for someone else.
Hypothesis: the problem either comes from or is hidden by a stale font cache. Maybe a system cache, maybe gnuplot's own cache. E.g. these lines in qt_term.cpp:
// Try to find the font metric in the cache or ask the GUI for the font metrics if (fontMetricCache.contains(currentFont)) metric = fontMetricCache[currentFont]; else {I can't think of a good way to test this idea.
You put into words what I was thinking, i.e., that maybe re-configuring with qt5 somehow cleared up a font issue. I distinctly recall a big gap of extra whitespace at the front of 1234567890 and a font of slightly different appearance.