QCustomPlot Discussion and Comments

Hi

Many thanks for your work on QCustomPlot!
I have noticed a few things that could be improved, which I am sharing here:
- The text and icon of QCPPlottableLegendItem are not properly aligned.
- TickLabels can overlap
- default values are not indicated in the documentation

The text and icon of QCPPlottableLegendItem are not properly aligned:
If you change void QCPPlottableLegendItem::draw(QCPPainter *painter) into this, then they become properly aligned. Note that this could be further improved by allowing the user to set the alignment itself, here I just put vertical and horizontal alignment by default.

void QCPPlottableLegendItem::draw(QCPPainter *painter)
{
  if (!mPlottable) return;
  painter->setFont(getFont());
  painter->setPen(QPen(getTextColor()));
  QSize iconSize = mParentLegend->iconSize();
  QRect textRect = painter->fontMetrics().boundingRect(0, 0, 0, 0, Qt::TextDontClip, mPlottable->name());
  int textHeight = qMax(textRect.height(), iconSize.height());
  int TopCoord = mRect.y() + (mRect.height() - textHeight)/2*1;  // *1 == Qt::AlignVCenter, *0 == Qt::AlignTop, *2 == Qt::AlignBottom
  int LeftCoord = mRect.x() + (mRect.width() - iconSize.width() - mParentLegend->iconTextPadding() - textRect.width())/2*1; // *1 == Qt::AlignHCenter, *0 == Qt::AlignLeft, *2 == Qt::AlignRight
  QRect iconRect(LeftCoord, TopCoord, iconSize.width(), iconSize.height());
  painter->drawText(LeftCoord+iconSize.width()+mParentLegend->iconTextPadding(), TopCoord, textRect.width(), textHeight, Qt::TextDontClip, mPlottable->name());
  // draw icon:
  painter->save();
  painter->setClipRect(iconRect, Qt::IntersectClip);
  mPlottable->drawLegendIcon(painter, iconRect);
  painter->restore();
  // draw icon border:
  if (getIconBorderPen().style() != Qt::NoPen)
  {
    painter->setPen(getIconBorderPen());
    painter->setBrush(Qt::NoBrush);
    int halfPen = qCeil(painter->pen().widthF()*0.5)+1;
    painter->setClipRect(mOuterRect.adjusted(-halfPen, -halfPen, halfPen, halfPen)); // extend default clip rect so thicker pens (especially during selection) are not clipped
    painter->drawRect(iconRect);
  }
}

TickLabels can overlap:
If you set the TickLabel font at 60 points or if you have a QCPAxisTickerDateTime with a format like "HH:mm:ss.zzz dddd dd MMMM yyyy", you will notice that TickLabels can overlap. One way to solve this is to change:

void QCPAxisPainterPrivate::placeTickLabel(QCPPainter *painter, double position, int distanceToAxis, const QString &text, QSize *tickLabelsSize, int& PreviousLabelPosition)
{
  // warning: if you change anything here, also adapt getMaxTickLabelSize() accordingly!
  if (text.isEmpty()) return;
  QSize finalSize;
  QPointF labelAnchor;
  switch (type)
  {
    case QCPAxis::atLeft:   labelAnchor = QPointF(axisRect.left()-distanceToAxis-offset, position); break;
    case QCPAxis::atRight:  labelAnchor = QPointF(axisRect.right()+distanceToAxis+offset, position); break;
    case QCPAxis::atTop:    labelAnchor = QPointF(position, axisRect.top()-distanceToAxis-offset); break;
    case QCPAxis::atBottom: labelAnchor = QPointF(position, axisRect.bottom()+distanceToAxis+offset); break;
  }
  if (mParentPlot->plottingHints().testFlag(QCP::phCacheLabels) && !painter->modes().testFlag(QCPPainter::pmNoCaching)) // label caching enabled
  {
    CachedLabel *cachedLabel = mLabelCache.take(text); // attempt to get label from cache
    if (!cachedLabel)  // no cached label existed, create it
    {
      cachedLabel = new CachedLabel;
      TickLabelData labelData = getTickLabelData(painter->font(), text);
      cachedLabel->offset = getTickLabelDrawOffset(labelData)+labelData.rotatedTotalBounds.topLeft();
      if (!qFuzzyCompare(1.0, mParentPlot->bufferDevicePixelRatio()))
      {
        cachedLabel->pixmap = QPixmap(labelData.rotatedTotalBounds.size()*mParentPlot->bufferDevicePixelRatio());
#ifdef QCP_DEVICEPIXELRATIO_SUPPORTED
#  ifdef QCP_DEVICEPIXELRATIO_FLOAT
        cachedLabel->pixmap.setDevicePixelRatio(mParentPlot->devicePixelRatioF());
#  else
        cachedLabel->pixmap.setDevicePixelRatio(mParentPlot->devicePixelRatio());
#  endif
#endif
      } else
        cachedLabel->pixmap = QPixmap(labelData.rotatedTotalBounds.size());
      cachedLabel->pixmap.fill(Qt::transparent);
      QCPPainter cachePainter(&cachedLabel->pixmap);
      cachePainter.setPen(painter->pen());
      drawTickLabel(&cachePainter, -labelData.rotatedTotalBounds.topLeft().x(), -labelData.rotatedTotalBounds.topLeft().y(), labelData);
    }
    // if label would be partly clipped by widget border on sides, don't draw it (only for outside tick labels):
    bool labelClippedByBorder = false;
    if (tickLabelSide == QCPAxis::lsOutside)
    {
      if (QCPAxis::orientation(type) == Qt::Horizontal)
        labelClippedByBorder = labelAnchor.x()+cachedLabel->offset.x()+cachedLabel->pixmap.width()/mParentPlot->bufferDevicePixelRatio() > viewportRect.right() || labelAnchor.x()+cachedLabel->offset.x() < PreviousLabelPosition;
      else
        labelClippedByBorder = labelAnchor.y()+cachedLabel->offset.y()+cachedLabel->pixmap.height()/mParentPlot->bufferDevicePixelRatio() > PreviousLabelPosition || labelAnchor.y()+cachedLabel->offset.y() < viewportRect.top();
    }
    if (!labelClippedByBorder)
    {
      painter->drawPixmap(labelAnchor+cachedLabel->offset, cachedLabel->pixmap);
      finalSize = cachedLabel->pixmap.size()/mParentPlot->bufferDevicePixelRatio();
	  if (QCPAxis::orientation(type) == Qt::Horizontal) {
		  PreviousLabelPosition = labelAnchor.x()+cachedLabel->offset.x()+cachedLabel->pixmap.width()/mParentPlot->bufferDevicePixelRatio();
	  } else {
		  PreviousLabelPosition = labelAnchor.y()+cachedLabel->offset.y();
	  };
    }
    mLabelCache.insert(text, cachedLabel); // return label to cache or insert for the first time if newly created
  } else // label caching disabled, draw text directly on surface:
  {
    TickLabelData labelData = getTickLabelData(painter->font(), text);
    QPointF finalPosition = labelAnchor + getTickLabelDrawOffset(labelData);
    // if label would be partly clipped by widget border on sides, don't draw it (only for outside tick labels):
     bool labelClippedByBorder = false;
    if (tickLabelSide == QCPAxis::lsOutside)
    {
      if (QCPAxis::orientation(type) == Qt::Horizontal)
        labelClippedByBorder = finalPosition.x()+(labelData.rotatedTotalBounds.width()+labelData.rotatedTotalBounds.left()) > viewportRect.right() || finalPosition.x()+labelData.rotatedTotalBounds.left() < PreviousLabelPosition;
      else
        labelClippedByBorder = finalPosition.y()+(labelData.rotatedTotalBounds.height()+labelData.rotatedTotalBounds.top()) > PreviousLabelPosition || finalPosition.y()+labelData.rotatedTotalBounds.top() < viewportRect.top();
    }
    if (!labelClippedByBorder)
    {
      drawTickLabel(painter, finalPosition.x(), finalPosition.y(), labelData);
      finalSize = labelData.rotatedTotalBounds.size();
	  if (QCPAxis::orientation(type) == Qt::Horizontal) {
		  PreviousLabelPosition = finalPosition.x()+(labelData.rotatedTotalBounds.width()+labelData.rotatedTotalBounds.left());
	  } else {
		  PreviousLabelPosition = finalPosition.y()+labelData.rotatedTotalBounds.top();
	  };
    }
  }
  
  // expand passed tickLabelsSize if current tick label is larger:
  if (finalSize.width() > tickLabelsSize->width())
    tickLabelsSize->setWidth(finalSize.width());
  if (finalSize.height() > tickLabelsSize->height())
    tickLabelsSize->setHeight(finalSize.height());
}

And in void QCPAxisPainterPrivate::draw(QCPPainter *painter):

    int PreviousLabelPosition;
	if(QCPAxis::orientation(type) == Qt::Horizontal){
		PreviousLabelPosition = viewportRect.left();
	} else {
		PreviousLabelPosition = viewportRect.bottom();
	};
	for (int i=0; i<maxLabelIndex; ++i)
      placeTickLabel(painter, tickPositions.at(i), distanceToAxis, tickLabels.at(i), &tickLabelsSize, PreviousLabelPosition);

And in the headers:

virtual void placeTickLabel(QCPPainter *painter, double position, int distanceToAxis, const QString &text, QSize *tickLabelsSize, int& PreviousLabelPosition);

Default values are not indicated in the documentation:
I think it would really help if default values were indicated in the documentation. For example, I struggled for a while to understand why my legend was not properly aligned at the top, until I realised that the insetLayout had margins of 12 by default. I think it would be better for this one to put a margin of 0 by default:
In QCustomPlot::QCustomPlot(QWidget *parent):

defaultAxisRect->insetLayout()->setMargins(QMargins(0, 0, 0, 0));

I hope my suggestions help!
Thank you again for developping QCustomPlot!

I realised I made a mistake for QCPPlottableLegendItem. It works correctly with this:

void QCPPlottableLegendItem::draw(QCPPainter *painter)
{
  if (!mPlottable) return;
  painter->setFont(getFont());
  painter->setPen(QPen(getTextColor()));
  QSize iconSize = mParentLegend->iconSize();
  QRect textRectMax = painter->fontMetrics().boundingRect(0, 0, 0, 0, Qt::AlignBottom, mPlottable->name());
  int maxHeight = qMax(textRectMax.height(), iconSize.height());
  int LeftCoord = mRect.x() + (mRect.width() - iconSize.width() - mParentLegend->iconTextPadding() - textRectMax.width())/2*0; // *1 == Qt::AlignHCenter, *0 == Qt::AlignLeft, *2 == Qt::AlignRight
  QRect iconRect(LeftCoord, mRect.y() + (maxHeight - iconSize.height())/2*1, iconSize.width(), iconSize.height()); // *1 == Qt::AlignVCenter, *0 == Qt::AlignTop, *2 == Qt::AlignBottom
  QRect textRect(LeftCoord+iconSize.width()+mParentLegend->iconTextPadding(), mRect.y() + (maxHeight - textRectMax.height())/2*1, textRectMax.width(), textRectMax.height()); // *1 == Qt::AlignVCenter, *0 == Qt::AlignTop, *2 == Qt::AlignBottom
  painter->drawText(textRect, Qt::AlignBottom, mPlottable->name());
  // draw icon:
  painter->save();
  painter->setClipRect(iconRect, Qt::IntersectClip);
  mPlottable->drawLegendIcon(painter, iconRect);
  painter->restore();
  // draw icon border:
  if (getIconBorderPen().style() != Qt::NoPen)
  {
    painter->setPen(getIconBorderPen());
    painter->setBrush(Qt::NoBrush);
    int halfPen = qCeil(painter->pen().widthF()*0.5)+1;
    painter->setClipRect(mOuterRect.adjusted(-halfPen, -halfPen, halfPen, halfPen)); // extend default clip rect so thicker pens (especially during selection) are not clipped
    painter->drawRect(iconRect);
  }
}

And one last suggestion for a custom number formatting of axis tick labels. I believe the current method of subclassing QCPAxisTicker and re-implementing getTickLabel() is not very user-friendly.
One way to make it better could be to add a QString mformatString member variable to the QCPAxis class and changing getTickLabel() into:

QString QCPAxisTicker::getTickLabel(double tick, const QLocale &locale, QChar formatChar, int precision, QString &formatString)
{
  return QString(formatString).arg(locale.toString(tick, formatChar.toLatin1(), precision));
}

and redefining createLabelVector() as:
QVector<QString> QCPAxisTicker::createLabelVector(const QVector<double> &ticks, const QLocale &locale, QChar formatChar, int precision, QString &formatString), which calls getTickLabel(tickCoord, locale, formatChar, precision, formatString)

and generate() as:
void QCPAxisTicker::generate(const QCPRange &range, const QLocale &locale, QChar formatChar, int precision, QString &formatString, QVector<double> &ticks, QVector<double> *subTicks, QVector<QString> *tickLabels), which calls createLabelVector(ticks, locale, formatChar, precision, formatString)

and with setupTickVectors() calling generate(mRange, mParentPlot->locale(), mNumberFormatChar, mNumberPrecision, mformatString, mTickVector, mSubTicks ? &mSubTickVector : nullptr, mTickLabels ? &mTickVectorLabels : nullptr)

The getter would be:

QString QCPAxis::formatString() const
{
  return mformatString;
};

The setter would be:

void QCPAxis::setFormatString(const QString &formatString){
	mformatString = formatString;
	return;
};

And with the following declarations in the QCPAxis class:

Q_PROPERTY(QString formatString READ formatString WRITE setFormatString)
QString mformatString;
QString formatString() const;
void setFormatString(const QString &formatString);

And finally in setupFullAxesBox():

xAxis2->setFormatString(xAxis->formatString());
yAxis2->setFormatString(yAxis->formatString());

That way, if you do for example customPlot->yAxis->setFormatString("$%1"), you easily get a custom tick label representing dollars.