More fixes when fitting image with display scaling

Extend logical pixel "rounding" to handle case where menu bar is visible at top of window which influences the calculation.

src/logicalpixelfitter.cpp

@@ -0,0 +1,57 @@
+#include "logicalpixelfitter.h"
+#include <QtMath>
+
+LogicalPixelFitter::LogicalPixelFitter(const qreal logicalScale, const QPoint offset)
+    : logicalScale(logicalScale), offset(offset)
+{
+}
+
+int LogicalPixelFitter::snapWidth(const qreal value) const
+{
+    return snap(value + offset.x(), logicalScale) - offset.x();
+}
+
+int LogicalPixelFitter::snapHeight(const qreal value) const
+{
+    return snap(value + offset.y(), logicalScale) - offset.y();
+}
+
+QSize LogicalPixelFitter::snapSize(const QSizeF size) const
+{
+    return QSize(snapWidth(size.width()), snapHeight(size.height()));
+}
+
+qreal LogicalPixelFitter::unsnapWidth(const int value) const
+{
+    return unsnap(value + offset.x(), logicalScale) - offset.x();
+}
+
+qreal LogicalPixelFitter::unsnapHeight(const int value) const
+{
+    return unsnap(value + offset.y(), logicalScale) - offset.y();
+}
+
+QSizeF LogicalPixelFitter::unsnapSize(const QSize size) const
+{
+    return QSizeF(unsnapWidth(size.width()), unsnapHeight(size.height()));
+}
+
+int LogicalPixelFitter::snap(const qreal value, const qreal logicalScale)
+{
+    const int valueRoundedDown = qFloor(qAbs(value));
+    const int valueRoundedUp = valueRoundedDown + 1;
+    const int physicalPixelsDrawn = qRound(qAbs(value) * logicalScale);
+    const int physicalPixelsShownIfRoundingUp = qRound(valueRoundedUp * logicalScale);
+    const int result = physicalPixelsDrawn >= physicalPixelsShownIfRoundingUp ? valueRoundedUp : valueRoundedDown;
+    return result * (value >= 0 ? 1 : -1);
+}
+
+qreal LogicalPixelFitter::unsnap(const int value, const qreal logicalScale)
+{
+    // For a given input value, its physical pixels fall within [value-0.5,value+0.5), so
+    // calculate the first physical pixel of the next value (rounding up if between pixels),
+    // and the pixel prior to that is the last one within the current value.
+    const int maxPhysicalPixelForValue = qCeil((qAbs(value) + 0.5) * logicalScale) - 1;
+    const qreal result = maxPhysicalPixelForValue / logicalScale;
+    return result * (value >= 0 ? 1 : -1);
+}

src/logicalpixelfitter.h

@@ -0,0 +1,33 @@
+#ifndef LOGICALPIXELFITTER_H
+#define LOGICALPIXELFITTER_H
+
+#include <QPoint>
+#include <QSize>
+
+class LogicalPixelFitter
+{
+public:
+    LogicalPixelFitter(const qreal logicalScale, const QPoint offset);
+
+    int snapWidth(const qreal value) const;
+
+    int snapHeight(const qreal value) const;
+
+    QSize snapSize(const QSizeF size) const;
+
+    qreal unsnapWidth(const int value) const;
+
+    qreal unsnapHeight(const int value) const;
+
+    QSizeF unsnapSize(const QSize size) const;
+
+    static int snap(const qreal value, const qreal logicalScale);
+
+    static qreal unsnap(const int value, const qreal logicalScale);
+
+private:
+    const qreal logicalScale;
+    const QPoint offset;
+};
+
+#endif // LOGICALPIXELFITTER_H

src/mainwindow.cpp

@@ -286,30 +286,24 @@ void MainWindow::paintEvent(QPaintEvent *event)
     QPainter painter(this);
 
     const QColor &backgroundColor = customBackgroundColor.isValid() ? customBackgroundColor : painter.background().color();
-
-    // Find the top of the viewport to account for the menu bar if it's inside the window
-    // and/or the label that displays titlebar text in full screen mode.
-    const int viewportY = graphicsView->mapTo(this, QPoint()).y();
-    // On macOS, part of the viewport may be additionally covered with the window's translucent
-    // titlebar due to full size content view.
-    const int unobscuredViewportY = qMax(getTitlebarOverlap(), viewportY);
+    const ViewportPosition viewportPos = getViewportPosition();
 
     // Erase the area above the viewport, i.e. fill it with the painter's default color.
-    const QRect headerRect = QRect(0, 0, width(), viewportY);
+    const QRect headerRect = QRect(0, 0, width(), viewportPos.widgetY);
     if (headerRect.isValid())
     {
         painter.eraseRect(headerRect);
     }
 
     // Fill the viewport with the background color.
-    const QRect viewportRect = rect().adjusted(0, viewportY, 0, 0);
+    const QRect viewportRect = rect().adjusted(0, viewportPos.widgetY, 0, 0);
     if (viewportRect.isValid())
     {
         painter.fillRect(viewportRect, backgroundColor);
     }
 
     // If there's an error message, draw it centered inside the unobscured area of the viewport.
-    const QRect unobscuredViewportRect = rect().adjusted(0, unobscuredViewportY, 0, 0);
+    const QRect unobscuredViewportRect = rect().adjusted(0, viewportPos.widgetY + viewportPos.obscuredHeight, 0, 0);
     if (getCurrentFileDetails().errorData.hasError && unobscuredViewportRect.isValid())
     {
         const QVImageCore::ErrorData &errorData = getCurrentFileDetails().errorData;
@@ -591,31 +585,18 @@ void MainWindow::setWindowSize()
     const QSize minWindowSize = (screenSize * minWindowResizedPercentage).boundedTo(hardLimitSize);
     const QSize maxWindowSize = (screenSize * qMax(maxWindowResizedPercentage, minWindowResizedPercentage)).boundedTo(hardLimitSize);
     const QSizeF imageSize = graphicsView->getEffectiveOriginalSize();
-    const qreal logicalPixelScale = graphicsView->devicePixelRatioF();
+    const LogicalPixelFitter fitter = graphicsView->getPixelFitter();
     const bool enforceMinSizeBothDimensions = false;
 
-    const auto gvRoundSizeF = [logicalPixelScale](const QSizeF value) {
-        return QSize(
-            QVGraphicsView::roundToCompleteLogicalPixel(value.width(), logicalPixelScale),
-            QVGraphicsView::roundToCompleteLogicalPixel(value.height(), logicalPixelScale)
-        );
-    };
-    const auto gvReverseRoundSize = [logicalPixelScale](const QSize value) {
-        return QSizeF(
-            QVGraphicsView::reverseLogicalPixelRounding(value.width(), logicalPixelScale),
-            QVGraphicsView::reverseLogicalPixelRounding(value.height(), logicalPixelScale)
-        );
-    };
-
-    QSize targetSize = gvRoundSizeF(imageSize);
+    QSize targetSize = fitter.snapSize(imageSize);
 
     const bool limitToMin = targetSize.width() < minWindowSize.width() && targetSize.height() < minWindowSize.height();
     const bool limitToMax = targetSize.width() > maxWindowSize.width() || targetSize.height() > maxWindowSize.height();
     if (limitToMin || limitToMax)
     {
-        const QSizeF enforcedSize = gvReverseRoundSize((limitToMin ? minWindowSize : maxWindowSize));
+        const QSizeF enforcedSize = fitter.unsnapSize(limitToMin ? minWindowSize : maxWindowSize);
         const qreal fitRatio = qMin(enforcedSize.width() / imageSize.width(), enforcedSize.height() / imageSize.height());
-        targetSize = gvRoundSizeF(imageSize * fitRatio);
+        targetSize = fitter.snapSize(imageSize * fitRatio);
     }
 
     if (enforceMinSizeBothDimensions)
@@ -1225,3 +1206,15 @@ int MainWindow::getTitlebarOverlap() const
 
     return 0;
 }
+
+MainWindow::ViewportPosition MainWindow::getViewportPosition() const
+{
+    ViewportPosition result;
+    // This accounts for anything that may be above the viewport such as the menu bar (if it's inside
+    // the window) and/or the label that displays titlebar text in full screen mode.
+    result.widgetY = windowHandle() ? graphicsView->mapTo(this, QPoint()).y() : 0;
+    // On macOS, part of the viewport may be additionally covered with the window's translucent
+    // titlebar due to full size content view.
+    result.obscuredHeight = qMax(getTitlebarOverlap() - result.widgetY, 0);
+    return result;
+}

src/mainwindow.h

@@ -26,6 +26,12 @@ class MainWindow : public QMainWindow
         QString previousPath;
     };
 
+    struct ViewportPosition
+    {
+        int widgetY;
+        int obscuredHeight;
+    };
+
     explicit MainWindow(QWidget *parent = nullptr);
     ~MainWindow() override;
 
@@ -113,6 +119,8 @@ class MainWindow : public QMainWindow
 
     int getTitlebarOverlap() const;
 
+    ViewportPosition getViewportPosition() const;
+
     const QVImageCore::FileDetails& getCurrentFileDetails() const { return graphicsView->getCurrentFileDetails(); }
 
 public slots:

src/qvgraphicsview.cpp

@@ -402,21 +402,14 @@ void QVGraphicsView::zoomToFit()
     if (viewSize.isEmpty())
         return;
 
-    const qreal logicalPixelScale = devicePixelRatioF();
-    const auto gvRound = [logicalPixelScale](const qreal value) {
-        return roundToCompleteLogicalPixel(value, logicalPixelScale);
-    };
-    const auto gvReverseRound = [logicalPixelScale](const int value) {
-        return reverseLogicalPixelRounding(value, logicalPixelScale);
-    };
-
-    const qreal fitXRatio = gvReverseRound(viewSize.width()) / imageSize.width();
-    const qreal fitYRatio = gvReverseRound(viewSize.height()) / imageSize.height();
+    const LogicalPixelFitter fitter = getPixelFitter();
+    const qreal fitXRatio = fitter.unsnapWidth(viewSize.width()) / imageSize.width();
+    const qreal fitYRatio = fitter.unsnapHeight(viewSize.height()) / imageSize.height();
 
     // Each mode will check if the rounded image size already produces the desired fit,
     // in which case we can use exactly 1.0 to avoid unnecessary scaling
-    const int imageOverflowX = gvRound(imageSize.width()) - viewSize.width();
-    const int imageOverflowY = gvRound(imageSize.height()) - viewSize.height();
+    const int imageOverflowX = fitter.snapWidth(imageSize.width()) - viewSize.width();
+    const int imageOverflowY = fitter.snapHeight(imageSize.height()) - viewSize.height();
 
     qreal targetRatio;
 
@@ -449,8 +442,8 @@ void QVGraphicsView::zoomToFit()
             // If the fit ratios are extremely close, it's possible that both are sufficient to
             // contain the image, but one results in the opposing dimension getting rounded down
             // to just under the view size, so use the larger of the two ratios in that case.
-            const bool isOverallFitToXRatio = gvRound(imageSize.height() * fitXRatio) == viewSize.height();
-            const bool isOverallFitToYRatio = gvRound(imageSize.width() * fitYRatio) == viewSize.width();
+            const bool isOverallFitToXRatio = fitter.snapHeight(imageSize.height() * fitXRatio) == viewSize.height();
+            const bool isOverallFitToYRatio = fitter.snapWidth(imageSize.width() * fitYRatio) == viewSize.width();
             if (isOverallFitToXRatio || isOverallFitToYRatio)
                 targetRatio = qMax(fitXRatio, fitYRatio);
             else
@@ -590,6 +583,12 @@ QSizeF QVGraphicsView::getEffectiveOriginalSize() const
     return getTransformWithNoScaling().mapRect(QRectF(QPoint(), getCurrentFileDetails().loadedPixmapSize)).size() / getDpiAdjustment();
 }
 
+LogicalPixelFitter QVGraphicsView::getPixelFitter() const
+{
+    const MainWindow::ViewportPosition viewportPos = getMainWindow()->getViewportPosition();
+    return LogicalPixelFitter(devicePixelRatioF(), QPoint(0, viewportPos.widgetY + viewportPos.obscuredHeight));
+}
+
 QRect QVGraphicsView::getContentRect() const
 {
     // Avoid using loadedPixmapItem and the active transform because the pixmap may have expensive scaling applied
@@ -600,36 +599,18 @@ QRect QVGraphicsView::getContentRect() const
     const qreal effectiveTransformScale = zoomLevel / appliedDpiAdjustment;
     const QTransform effectiveTransform = getTransformWithNoScaling().scale(effectiveTransformScale, effectiveTransformScale);
     const QRectF contentRect = effectiveTransform.mapRect(loadedPixmapBoundingRect);
-    const qreal logicalPixelScale = devicePixelRatioF();
-    const auto gvRound = [logicalPixelScale](const qreal value) {
-        return roundToCompleteLogicalPixel(qAbs(value), logicalPixelScale) * (value >= 0 ? 1 : -1);
-    };
-    return QRect(gvRound(contentRect.left()), gvRound(contentRect.top()), gvRound(contentRect.width()), gvRound(contentRect.height()));
+    return QRect(contentRect.topLeft().toPoint(), getPixelFitter().snapSize(contentRect.size()));
 }
 
 QRect QVGraphicsView::getUsableViewportRect() const
 {
-#ifdef COCOA_LOADED
-    int obscuredHeight = QVCocoaFunctions::getObscuredHeight(window()->windowHandle());
-#else
-    int obscuredHeight = 0;
-#endif
     QRect rect = viewport()->rect();
-    rect.setTop(obscuredHeight);
+    rect.setTop(getMainWindow()->getViewportPosition().obscuredHeight);
     return rect;
 }
 
 void QVGraphicsView::setTransformScale(qreal value)
 {
-#ifndef Q_OS_MACOS
-    // If fractional display scaling is in use, when attempting to target a given size, the resulting error
-    // can be [0,1) unlike the typical [0,0.5] without scaling or integer scaling. This is because the
-    // image origin which is always at an integer logical pixel becomes potentially a fractional physical
-    // pixel due to the display scaling, adding to the overall error. As a result, tiny rounding errors can
-    // cause us to miss the size we were targetting, so increase the scale just a hair to compensate.
-    if (value != std::floor(value))
-        value *= 1.0 + std::numeric_limits<double>::epsilon();
-#endif
     setTransform(getTransformWithNoScaling().scale(value, value));
 }
 
@@ -651,24 +632,6 @@ qreal QVGraphicsView::getDpiAdjustment() const
     return isOneToOnePixelSizingEnabled ? devicePixelRatioF() : 1.0;
 }
 
-int QVGraphicsView::roundToCompleteLogicalPixel(const qreal value, const qreal logicalScale)
-{
-    const int valueRoundedDown = qFloor(value);
-    const int valueRoundedUp = valueRoundedDown + 1;
-    const int physicalPixelsDrawn = qRound(value * logicalScale);
-    const int physicalPixelsShownIfRoundingUp = qRound(valueRoundedUp * logicalScale);
-    return physicalPixelsDrawn >= physicalPixelsShownIfRoundingUp ? valueRoundedUp : valueRoundedDown;
-}
-
-qreal QVGraphicsView::reverseLogicalPixelRounding(const int value, const qreal logicalScale)
-{
-    // For a given input value, its physical pixels fall within [value-0.5,value+0.5), so
-    // calculate the first physical pixel of the next value (rounding up if between pixels),
-    // and the pixel prior to that is the last one within the current value.
-    int maxPhysicalPixelForValue = qCeil((value + 0.5) * logicalScale) - 1;
-    return maxPhysicalPixelForValue / logicalScale;
-}
-
 void QVGraphicsView::handleDpiAdjustmentChange()
 {
     if (appliedDpiAdjustment == getDpiAdjustment())
@@ -682,6 +645,11 @@ void QVGraphicsView::handleDpiAdjustmentChange()
         expensiveScaleTimer->start();
 }
 
+MainWindow* QVGraphicsView::getMainWindow() const
+{
+    return qobject_cast<MainWindow*>(window());
+}
+
 void QVGraphicsView::settingsUpdated()
 {
     auto &settingsManager = qvApp->getSettingsManager();

src/qvgraphicsview.h

@@ -2,13 +2,16 @@
 #define QVGRAPHICSVIEW_H
 
 #include "qvimagecore.h"
+#include "logicalpixelfitter.h"
 #include <QGraphicsView>
 #include <QImageReader>
 #include <QMimeData>
 #include <QDir>
 #include <QTimer>
 #include <QFileInfo>
 
+class MainWindow;
+
 class QVGraphicsView : public QGraphicsView
 {
     Q_OBJECT
@@ -63,15 +66,13 @@ class QVGraphicsView : public QGraphicsView
 
     QSizeF getEffectiveOriginalSize() const;
 
+    LogicalPixelFitter getPixelFitter() const;
+
     const QVImageCore::FileDetails& getCurrentFileDetails() const { return imageCore.getCurrentFileDetails(); }
     const QPixmap& getLoadedPixmap() const { return imageCore.getLoadedPixmap(); }
     const QMovie& getLoadedMovie() const { return imageCore.getLoadedMovie(); }
     qreal getZoomLevel() const { return zoomLevel; }
 
-    static int roundToCompleteLogicalPixel(const qreal value, const qreal logicalScale);
-
-    static qreal reverseLogicalPixelRounding(const int value, const qreal logicalScale);
-
 signals:
     void cancelSlideshow();
 
@@ -116,6 +117,8 @@ class QVGraphicsView : public QGraphicsView
 
     void handleDpiAdjustmentChange();
 
+    MainWindow* getMainWindow() const;
+
 private slots:
     void animatedFrameChanged(QRect rect);
 

src/src.pri

@@ -12,6 +12,7 @@ SOURCES += \
     $$PWD/qvimagecore.cpp \
     $$PWD/qvshortcutdialog.cpp \
     $$PWD/actionmanager.cpp \
+    $$PWD/logicalpixelfitter.cpp \
     $$PWD/settingsmanager.cpp \
     $$PWD/shortcutmanager.cpp
 
@@ -34,6 +35,7 @@ HEADERS += \
     $$PWD/qvimagecore.h \
     $$PWD/qvshortcutdialog.h \
     $$PWD/actionmanager.h \
+    $$PWD/logicalpixelfitter.h \
     $$PWD/settingsmanager.h \
     $$PWD/shortcutmanager.h