QPdfPageSelector: port from is-a to has-a QSpinBox

Yes, the class needs to inherit from QSpinBox in order to override the
protected virtuals, but that doesn't mean a QPdfPageSelector should
model is-a QSpinBox. E.g. the range of the QPdfPageSelector is taken
from the QPdfDocument, and no good can come from a user changing it
through the use of QSpinBox API, esp. if one thinks about the class
being displayed as a native widget in QtDesigner.

In a similar vein, the inheritance from QSpinBox leaves users
wondering (and the docs didn't do anything to enlighten them) what
properties are pertinent to the task at hand. setValue() to set the
page index is ... meh, but still somewhat discoverable. But that
text() is only QPdfDocument::pageLabel() if no affixes are set takes
quite some digging.

Part of this could be fixed by providing domain-specific properties
like currentPage for value, but that would just increase the
confusion, because those two properties would must needs exist on the
same object.

Instead, fix by moving the old QPdfPageSelector as
QPdfPageSelectorSpinBox into qpdfpageselector_p.h, removing its pimpl
and having QPdfPageSelector inherit QWidget instead, aggregating a
QPdfPageSelectorSpinBox. This involves a few more objects (the widget,
the layout), but gives QPdfPageSelector full control over its
interface.

Add a few salient properties (more can be added if needed, by copying
them from QSpinBox or QAbstractSpinBox).

Note how well the new API rhymes with QPdfPageNavigator in the
connect() in pdfviewer example's MainWindow.

Since we still store everything in QPdfPageSelectorSpinBox, but don't
pimpl it anymore, QPdfPageSelectorPrivate can lose the q_ptr now, and
merely contains the pointer to its QPdfPageSelectorSpinBox. This
could be optimized further, by making QPdfPageSelectorSpinBox and
QPdfPageSelectorPrivate the same class, but that's neither BC- nor
SC-relevant anymore, and can be done later (or never, seeing as this
widget is unlikely to be used more than a few times per application).

Found in API-review.

As a drive-by, port to the std-compatible subset of the QPointer API.

Change-Id: I0d82d098d38d5f2fcf7f1c8c9aed6e792a8deb2d
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>
(cherry picked from commit 8b20e0636a89dc3e6b06d174286ee16aa427887a)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

4 files changed, 145 insertions, 61 deletions

diff --git a/examples/pdfwidgets/pdfviewer/mainwindow.cpp b/examples/pdfwidgets/pdfviewer/mainwindow.cpp
index 5c4554ca2..ce19359fc 100644
--- a/examples/pdfwidgets/pdfviewer/mainwindow.cpp
+++ b/examples/pdfwidgets/pdfviewer/mainwindow.cpp
@@ -38,10 +38,10 @@ MainWindow::MainWindow(QWidget *parent)
     ui->mainToolBar->insertWidget(ui->actionZoom_In, m_zoomSelector);
 
     ui->mainToolBar->insertWidget(ui->actionForward, m_pageSelector);
-    connect(m_pageSelector, &QPdfPageSelector::valueChanged, this, &MainWindow::pageSelected);
+    connect(m_pageSelector, &QPdfPageSelector::currentPageChanged, this, &MainWindow::pageSelected);
     m_pageSelector->setDocument(m_document);
     auto nav = ui->pdfView->pageNavigator();
-    connect(nav, &QPdfPageNavigator::currentPageChanged, m_pageSelector, &QPdfPageSelector::setValue);
+    connect(nav, &QPdfPageNavigator::currentPageChanged, m_pageSelector, &QPdfPageSelector::setCurrentPage);
     connect(nav, &QPdfPageNavigator::backAvailableChanged, ui->actionBack, &QAction::setEnabled);
     connect(nav, &QPdfPageNavigator::forwardAvailableChanged, ui->actionForward, &QAction::setEnabled);
 
@@ -116,7 +116,7 @@ void MainWindow::pageSelected(int page)
     setWindowTitle(!documentTitle.isEmpty() ? documentTitle : QStringLiteral("PDF Viewer"));
     setWindowTitle(tr("%1: page %2 (%3 of %4)")
                    .arg(documentTitle.isEmpty() ? u"PDF Viewer"_qs : documentTitle,
-                        m_pageSelector->text(), QString::number(page + 1), QString::number(m_document->pageCount())));
+                        m_pageSelector->currentPageLabel(), QString::number(page + 1), QString::number(m_document->pageCount())));
 }
 
 void MainWindow::searchResultSelected(const QModelIndex &current, const QModelIndex &previous)
diff --git a/src/pdfwidgets/qpdfpageselector.cpp b/src/pdfwidgets/qpdfpageselector.cpp
index 542f79996..0c42573d8 100644
--- a/src/pdfwidgets/qpdfpageselector.cpp
+++ b/src/pdfwidgets/qpdfpageselector.cpp
@@ -6,30 +6,20 @@
 
 #include <QPdfDocument>
 
-QT_BEGIN_NAMESPACE
+#include <QtWidgets/qboxlayout.h>
 
-QPdfPageSelectorPrivate::QPdfPageSelectorPrivate(QPdfPageSelector *q)
-    : q_ptr(q)
-{
-}
+using namespace Qt::StringLiterals;
 
-void QPdfPageSelectorPrivate::documentStatusChanged()
-{
-    Q_Q(QPdfPageSelector);
-    if (!document.isNull() && document->status() == QPdfDocument::Status::Ready) {
-        q->setMaximum(document->pageCount());
-        q->setValue(0);
-    }
-}
+QT_BEGIN_NAMESPACE
 
 /*!
     \class QPdfPageSelector
     \inmodule QtPdf
     \since 6.6
-    \brief A QSpinBox for selecting a PDF page.
+    \brief A widget for selecting a PDF page.
 
-    QPdfPageSelector is a QSpinBox specialized for selecting a page label
-    from a QPdfDocument.
+    QPdfPageSelector is a widget for selecting a page label from a
+    QPdfDocument.
 
     \sa QPdfDocument::pageLabel()
 */
@@ -38,70 +28,138 @@ void QPdfPageSelectorPrivate::documentStatusChanged()
     Constructs a PDF page selector with parent widget \a parent.
 */
 QPdfPageSelector::QPdfPageSelector(QWidget *parent)
-    : QSpinBox(parent)
-    , d_ptr(new QPdfPageSelectorPrivate(this))
+    : QWidget(parent), d_ptr(new QPdfPageSelectorPrivate)
 {
+    Q_D(QPdfPageSelector);
+    d->spinBox = new QPdfPageSelectorSpinBox(this);
+    d->spinBox->setObjectName(u"_q_spinBox"_s);
+    auto vlay = new QVBoxLayout(this);
+    vlay->setContentsMargins({});
+    vlay->addWidget(d->spinBox);
+
+    connect(d->spinBox, &QPdfPageSelectorSpinBox::_q_documentChanged,
+            this, &QPdfPageSelector::documentChanged);
+    connect(d->spinBox, &QSpinBox::valueChanged, this, &QPdfPageSelector::currentPageChanged);
+    connect(d->spinBox, &QSpinBox::textChanged, this, &QPdfPageSelector::currentPageLabelChanged);
 }
 
 /*!
     Destroys the page selector.
 */
 QPdfPageSelector::~QPdfPageSelector()
-{
-}
+    = default;
 
 /*!
     \property QPdfPageSelector::document
 
     This property holds the document to be viewed.
 */
-void QPdfPageSelector::setDocument(QPdfDocument *document)
+
+void QPdfPageSelector::setDocument(QPdfDocument *doc)
 {
     Q_D(QPdfPageSelector);
+    d->spinBox->setDocument(doc);
+}
 
-    if (d->document == document)
-        return;
+QPdfDocument *QPdfPageSelector::document() const
+{
+    Q_D(const QPdfPageSelector);
+    return d->spinBox->document();
+}
 
-    if (d->document)
-        disconnect(d->documentStatusChangedConnection);
+/*!
+    \property QPdfPageSelector::currentPage
 
-    d->document = document;
-    emit documentChanged(d->document);
+    This property holds the index (\c{0}-based) of the current page in the
+    document.
+*/
 
-    if (d->document)
-        d->documentStatusChangedConnection =
-                connect(d->document.data(), &QPdfDocument::statusChanged, this,
-                        [d](){ d->documentStatusChanged(); });
+int QPdfPageSelector::currentPage() const
+{
+    Q_D(const QPdfPageSelector);
+    return d->spinBox->value();
+}
 
-    d->documentStatusChanged();
+void QPdfPageSelector::setCurrentPage(int index)
+{
+    Q_D(QPdfPageSelector);
+    d->spinBox->setValue(index);
 }
 
-QPdfDocument *QPdfPageSelector::document() const
+/*!
+    \property QPdfPageSelector::currentPageIndex
+
+    This property holds the page label corresponding to the current page index
+    in the document.
+
+    This is the text presented to the user.
+
+    \sa QPdfDocument::pageLabel()
+*/
+
+QString QPdfPageSelector::currentPageLabel() const
 {
     Q_D(const QPdfPageSelector);
+    return d->spinBox->text();
+}
 
-    return d->document;
+//
+// QPdfPageSelectorSpinBox:
+//
+
+void QPdfPageSelectorSpinBox::documentStatusChanged()
+{
+    if (m_document && m_document->status() == QPdfDocument::Status::Ready) {
+        setMaximum(m_document->pageCount());
+        setValue(0);
+    }
 }
 
-int QPdfPageSelector::valueFromText(const QString &text) const
+void QPdfPageSelectorSpinBox::setDocument(QPdfDocument *document)
 {
-    Q_D(const QPdfPageSelector);
-    if (d->document.isNull())
+    if (m_document == document)
+        return;
+
+    if (m_document)
+        disconnect(m_documentStatusChangedConnection);
+
+    m_document = document;
+    emit _q_documentChanged(document);
+
+    if (m_document) {
+        m_documentStatusChangedConnection =
+                connect(m_document.get(), &QPdfDocument::statusChanged,
+                        this, &QPdfPageSelectorSpinBox::documentStatusChanged);
+    }
+
+    documentStatusChanged();
+}
+
+QPdfPageSelectorSpinBox::QPdfPageSelectorSpinBox(QWidget *parent)
+    : QSpinBox(parent)
+{
+}
+
+QPdfPageSelectorSpinBox::~QPdfPageSelectorSpinBox()
+    = default;
+
+int QPdfPageSelectorSpinBox::valueFromText(const QString &text) const
+{
+    if (!m_document)
         return 0;
 
-    return d->document->pageIndexForLabel(text.trimmed());
+    return m_document->pageIndexForLabel(text.trimmed());
 }
 
-QString QPdfPageSelector::textFromValue(int value) const
+QString QPdfPageSelectorSpinBox::textFromValue(int value) const
 {
-    Q_D(const QPdfPageSelector);
-    if (d->document.isNull())
+    if (!m_document)
         return {};
 
-    return d->document->pageLabel(value);
+    return m_document->pageLabel(value);
 }
 
-QValidator::State QPdfPageSelector::validate(QString &text, int &pos) const
+QValidator::State QPdfPageSelectorSpinBox::validate(QString &text, int &pos) const
 {
     Q_UNUSED(pos);
     return valueFromText(text) >= 0 ? QValidator::Acceptable : QValidator::Intermediate;
@@ -109,4 +167,5 @@ QValidator::State QPdfPageSelector::validate(QString &text, int &pos) const
 
 QT_END_NAMESPACE
 
+#include "moc_qpdfpageselector_p.cpp"
 #include "moc_qpdfpageselector.cpp"
diff --git a/src/pdfwidgets/qpdfpageselector.h b/src/pdfwidgets/qpdfpageselector.h
index a2036be21..d779f54cd 100644
--- a/src/pdfwidgets/qpdfpageselector.h
+++ b/src/pdfwidgets/qpdfpageselector.h
@@ -5,7 +5,8 @@
 #define QPDFPAGESELECTOR_H
 
 #include <QtPdfWidgets/qtpdfwidgetsglobal.h>
-#include <QtWidgets/qspinbox.h>
+
+#include <QtWidgets/qwidget.h>
 
 #include <memory>
 
@@ -14,12 +15,13 @@ QT_BEGIN_NAMESPACE
 class QPdfDocument;
 class QPdfPageSelectorPrivate;
 
-class Q_PDF_WIDGETS_EXPORT QPdfPageSelector : public QSpinBox
+class Q_PDF_WIDGETS_EXPORT QPdfPageSelector : public QWidget
 {
     Q_OBJECT
 
     Q_PROPERTY(QPdfDocument* document READ document WRITE setDocument NOTIFY documentChanged)
-
+    Q_PROPERTY(int currentPage READ currentPage WRITE setCurrentPage NOTIFY currentPageChanged USER true)
+    Q_PROPERTY(QString currentPageLabel READ currentPageLabel NOTIFY currentPageLabelChanged)
 public:
     QPdfPageSelector() : QPdfPageSelector(nullptr) {}
     explicit QPdfPageSelector(QWidget *parent);
@@ -28,13 +30,16 @@ public:
     void setDocument(QPdfDocument *document);
     QPdfDocument *document() const;
 
+    int currentPage() const;
+    QString currentPageLabel() const;
+
+public Q_SLOTS:
+    void setCurrentPage(int index);
+
 Q_SIGNALS:
     void documentChanged(QPdfDocument *document);
-
-protected:
-    int valueFromText(const QString &text) const override;
-    QString textFromValue(int value) const override;
-    QValidator::State validate(QString &text, int &pos) const override;
+    void currentPageChanged(int index);
+    void currentPageLabelChanged(const QString &label);
 
 private:
     Q_DECLARE_PRIVATE(QPdfPageSelector)
diff --git a/src/pdfwidgets/qpdfpageselector_p.h b/src/pdfwidgets/qpdfpageselector_p.h
index 6954f39cd..8e961f1d2 100644
--- a/src/pdfwidgets/qpdfpageselector_p.h
+++ b/src/pdfwidgets/qpdfpageselector_p.h
@@ -17,22 +17,42 @@
 
 #include "qpdfpageselector.h"
 
+#include <QtWidgets/qspinbox.h>
+
 #include <QPointer>
 
 QT_BEGIN_NAMESPACE
 
-class QPdfPageSelectorPrivate
+class QPdfPageSelectorSpinBox : public QSpinBox
 {
-    Q_DECLARE_PUBLIC(QPdfPageSelector)
-
+    Q_OBJECT
 public:
-    QPdfPageSelectorPrivate(QPdfPageSelector *q);
+    QPdfPageSelectorSpinBox() : QPdfPageSelectorSpinBox(nullptr) {}
+    explicit QPdfPageSelectorSpinBox(QWidget *parent);
+    ~QPdfPageSelectorSpinBox();
+
+    void setDocument(QPdfDocument *document);
+    QPdfDocument *document() const { return m_document.get(); }
+
+Q_SIGNALS:
+    void _q_documentChanged(QPdfDocument *document);
 
+protected:
+    int valueFromText(const QString &text) const override;
+    QString textFromValue(int value) const override;
+    QValidator::State validate(QString &text, int &pos) const override;
+
+private:
     void documentStatusChanged();
+private:
+    QPointer<QPdfDocument> m_document;
+    QMetaObject::Connection m_documentStatusChangedConnection;
+};
 
-    QPdfPageSelector *q_ptr;
-    QPointer<QPdfDocument> document;
-    QMetaObject::Connection documentStatusChangedConnection;
+class QPdfPageSelectorPrivate
+{
+public:
+    QPdfPageSelectorSpinBox *spinBox;
 };
 
 QT_END_NAMESPACE