Currently working on a patch to address deprecations that trigger -Werror with Qt 5.14.1

[Scott Kostyshak]

On Mon, Mar 02, 2020 at 10:00:18AM -0500, Scott Kostyshak wrote:
> On Sun, Mar 01, 2020 at 08:04:47PM +0100, Jean-Marc Lasgouttes wrote:
> > Le 29/02/2020 à 18:27, Scott Kostyshak a écrit :
> > > In Qt 5.11, horizontalAdvance() was added "to replace the confusingly
> > > named width() function", which was deprecated in the same release. [1]
> > 
> > Do you know whether they are equivalent, or subtly different?
> 
> The entry in the change log hints (from my understanding) that it is a
> renaming, but the blog article I linked to suggests things could be more
> complicated.

Looking at the code, it does seem to be equivalent:

  qreal QFontMetricsF::width(const QString &text) const
  {
      return horizontalAdvance(text);
  }

For the code, see:
https://code.qt.io/cgit/qt/qtbase.git/tree/src/gui/text/qfontmetrics.cpp#n1499

So it seems it would not be a regression for us to rename every instance
to horizontalAdvance(), although I would have to check the equivalence
for all variants of the method. However, I guess the idea here is it's a
good opportunity to catch a mistake in our code?

> > >    "I am pretty sure that in most cases QFontMetrics::boundingRect() is
> > >    what you want, unless you are writing custom text shaping/layouting
> > >    code".
> > 
> > This will be the case where we also measure ascent and descent indeed.
> > 
> > >  From what I understand, we are indeed writing custom text
> > > shaping/layouting code.
> > 
> > Yes, but mostly for the code in GuiFontMetrics.
> > 
> > > Attached is a patch. I don't propose to commit this patch; I just attach
> > > it to give an idea of how many replacements are needed and in case
> > > someone can check that we do indeed want horizontalAdvance() as the
> > > replacement in all cases and not boundingRect().
> > 
> > The best is to apply small bits by small bits. As I said, only
> > GuiFontMetrics should be problematic.

Let's take the first case then, which is CCItemDelegate::drawCategoryHeader:

  // draw the centered text
  QFontMetrics fm(font);
  int w = fm.width(category);
  int x = opt.rect.x() + (opt.rect.width() - w) / 2;
  int y = opt.rect.y() + 3 * fm.ascent() / 2;
  int left = x;
  int right = x + w;
  painter->drawText(x, y, category);

From what I understand, this code wants to center the category header.
Although I've read up on the differences of boundingRect().width() and
horizontalAdvance(), I don't know which one is correct here. In some
sense, we are not really drawing text after the string---we just want to
know where to start drawing it so that it is centered. So that makes me
think boundingRect().width() would be more appropriate. On the other
hand, I wonder if horizontalAdvance() would provide a centering that is
more natural to the eye. If I had to guess, I would say that
boundingRect().width() is the correct choice here: I'm picturing a
string with weird bearings on the first and last character, and I think
my eyes would prefer for the bearings to not be taken into account while
centering. Is my approach to thinking about this reasonable?

Scott

> > > JMarc, I'm guessing you're the one that I should bother about taking a
> > > look at this specific change. Can you take a look when you have time? If
> > > you prefer to not think about this or don't have time, it can wait; it's
> > > just a compilation warning message.
> > 
> > Mostly for font metrics, but I can have a look. For the ones that are in
> > dialogs and such; you guess is as good as mine.
> 
> Sounds good. I'll wait for you to take a look at font metrics then. If
> you prefer, I could take a naive brute force approach of using extra
> code that calculates all three of horizontalAdvance(),
> boundingRect().width(), and the deprecated width() and then seeing which
> are equal. I guess I would need to find a case where horizontalAdvance()
> and boundingRect().width are different in order to be sure which should
> be the replacement, but I suppose I could play around.
> 
> > > After figuring out the above, the next question would be how to
> > > implement the changes in a way that preserves compatibility with Qt
> > > versions < 5.11. Should we use a directive to condition on the Qt
> > > version around each instance or since there are several instances should
> > > we use the approach of a macro as is done at [3]?
> > 
> > I think that the only places where we need horizontal advance is when we do
> > layout things, so mainly what is in GuiFontMetrics. In the other cases, we
> > want the size of a box that contains the string, so the
> > boundingBox().width() approach is fine
> 
> Good to know, that does simplify things since it seems that
> QFontMetrics::boundingRect has been in Qt for a while so no need to
> do a conditional with directives.
> 
> > (although it seems to me tht it
> > computes ascent and descent unnecessarily).
> 
> Ah good point. I wonder if the "old" width() method actually avoided
> that or not (maybe both were calculated and stored as members and width
> was just a get() method?). In any case, I guess it is not worth it to
> make a custom method to increase the efficiency.
> 
> Thanks for your comments.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.lyx.org/pipermail/lyx-devel/attachments/20200319/5ddbd792/attachment.asc>