qt: Add workaround for QProgressDialog bug on macOS #15040

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20181226-fix-macos-qprogressdialog changing 4 files +32 −23

hebasto commented at 8:27 PM on December 26, 2018: member
Fix #15016.

Refs:
- QTBUG-65750: QProgressDialog too small width at larger font size on Mac
- QTBUG-70357: QProgressDialog is too narrow to fit the text of its label
With this PR:
fanquake added the label GUI on Dec 26, 2018
HashUnlimited commented at 9:57 AM on December 27, 2018: contributor

As a workaround this seems to work for the specific case, tried with extra long wallet names.

Thanks for the links btw, out of curiosity I have also tried to apply the same to a different project where Qt's dialog margins are overruled by style sheets - no effect there (this just for info).
jonasschnelli commented at 7:21 PM on December 30, 2018: contributor

Thanks for the fix... But isn't it possible to add a QLayout and have the dialogue autoresize like other windows? However, this seems to work as well.
donaloconnor approved
donaloconnor commented at 12:09 PM on December 31, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/15040/commits/1f7ac92cf88ff7e6b10aaa49e174805a5823bfa2

hebasto commented at 2:22 PM on January 1, 2019: member

@jonasschnelli Thank you for your review.

But isn't it possible to add a QLayout and have the dialogue autoresize like other windows?

Yes, it is possible to do something like this:

QLabel* label = new QLabel(title);
progressDialog->setLabel(label);
QProgressBar* bar = new QProgressBar;
progressDialog->setBar(bar);
QPushButton* cancel_button = new QPushButton(tr("Cancel"));
progressDialog->setCancelButton(cancel_button);

QHBoxLayout* h_layout = new QHBoxLayout;
h_layout->addStretch();
h_layout->addWidget(cancel_button);
QVBoxLayout* v_layout = new QVBoxLayout;
v_layout->addWidget(label);
v_layout->addWidget(bar);
v_layout->addLayout(h_layout);
progressDialog->setLayout(v_layout);

But such implementation introduces new issues: e.g., content margins of QLayout are ignored on resize event on Linux. Is it better to make 3-lines-of-code workaround and just wait when buggy QProgressDialog get fixed by Qt?

jonasschnelli commented at 8:06 PM on January 3, 2019: contributor

utACK 1f7ac92cf88ff7e6b10aaa49e174805a5823bfa2

in src/qt/bitcoingui.cpp:1246 in 1f7ac92cf8 outdated

1242 | @@ -1242,6 +1243,12 @@ void BitcoinGUI::showProgress(const QString &title, int nProgress)
1243 |      if (nProgress == 0)
1244 |      {
1245 |          progressDialog = new QProgressDialog(title, "", 0, 100);
1246 | +#ifdef Q_OS_MAC

promag commented at 11:10 PM on January 4, 2019:

Move the implementation to something like:

void GUIUtil::PolishProgressDialog(QProgressDialog* dialog)
{
#ifdef Q_OS_MAC
    // Workaround for macOS-only Qt bug; see: QTBUG-65750, QTBUG-70357.
    const int margin = (dialog->fontMetrics()).width("X");
    progressDialog->resize(dialog->width() + (2 * margin), progressDialog->height());
#else
    Q_UNUSED(dialog);
#endif
}

hebasto commented at 7:42 AM on January 5, 2019:

Thank you @promag. Will do.

hebasto force-pushed on Jan 5, 2019
hebasto commented at 10:20 AM on January 5, 2019: member

@promag your comment has been addressed.

Also a little of obvious code styling added.

in src/qt/bitcoingui.cpp:1247 in ce89a5c72d outdated

1245 | +    if (nProgress == 0) {
1246 | +        progressDialog = new QProgressDialog(title, QString(), 0, 100);
1247 | +        GUIUtil::PolishProgressDialog(progressDialog);
1248 |          progressDialog->setWindowModality(Qt::ApplicationModal);
1249 |          progressDialog->setMinimumDuration(0);
1250 | -        progressDialog->setCancelButton(0);

hebasto commented at 10:24 AM on January 5, 2019:

Note for reviewers. QProgressDialog::QProgressDialog() docs:

If QString() is passed then no cancel button is shown.

DrahtBot commented at 10:56 PM on January 9, 2019: member



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
DrahtBot added the label Needs rebase on Jan 14, 2019
hebasto force-pushed on Jan 14, 2019
hebasto commented at 5:28 PM on January 14, 2019: member

Rebased. @promag would you mind re-reviewing?
DrahtBot removed the label Needs rebase on Jan 14, 2019

in src/qt/guiutil.cpp:942 in 2b0688124e outdated

 934 | @@ -933,4 +935,16 @@ bool ItemDelegate::eventFilter(QObject *object, QEvent *event)
 935 |      return QItemDelegate::eventFilter(object, event);
 936 |  }
 937 |  
 938 | +void PolishProgressDialog(QProgressDialog* dialog)
 939 | +{
 940 | +#ifdef Q_OS_MAC
 941 | +    // Workaround for macOS-only Qt bug; see: QTBUG-65750, QTBUG-70357.
 942 | +    const int margin = (dialog->fontMetrics()).width("X");

promag commented at 10:47 PM on January 14, 2019:

nit const int margin = dialog->fontMetrics().width("X");

in src/qt/guiutil.cpp:943 in 2b0688124e outdated

 934 | @@ -933,4 +935,16 @@ bool ItemDelegate::eventFilter(QObject *object, QEvent *event)
 935 |      return QItemDelegate::eventFilter(object, event);
 936 |  }
 937 |  
 938 | +void PolishProgressDialog(QProgressDialog* dialog)
 939 | +{
 940 | +#ifdef Q_OS_MAC
 941 | +    // Workaround for macOS-only Qt bug; see: QTBUG-65750, QTBUG-70357.
 942 | +    const int margin = (dialog->fontMetrics()).width("X");
 943 | +    dialog->resize(dialog->width() + (2 * margin), dialog->height());

promag commented at 10:47 PM on January 14, 2019:

nit, dialog->width() + 2 * margin.

promag commented at 10:49 PM on January 14, 2019: member

utACK 2b06881, just 2 minor nits.
Add workaround for QProgressDialog bug on macOS
See: QTBUG-65750, QTBUG-70357.
7c572c488d
hebasto force-pushed on Jan 14, 2019
hebasto commented at 11:31 PM on January 14, 2019: member

@promag all nits are fixed.
hebasto commented at 2:30 PM on January 17, 2019: member

@Sjors would you mind reviewing this PR?
Sjors commented at 3:59 PM on January 17, 2019: member

tACK 7c572c4 on macOS 10.14.2
promag commented at 4:13 PM on January 17, 2019: member

utACK 7c572c4.
jonasschnelli commented at 9:16 PM on January 17, 2019: contributor

Tested ACK 7c572c488dcf84438d64da4ca920a48810044a72
jonasschnelli merged this on Jan 17, 2019
jonasschnelli closed this on Jan 17, 2019
jonasschnelli referenced this in commit cd42553b11 on Jan 17, 2019
hebasto deleted the branch on Jan 17, 2019
Sjors commented at 6:16 PM on January 18, 2019: member

@HashUnlimited that's probably too much OS-specific customisation. I don't think the benefits of that feature would outweigh the cost of maintaining it. Feel free to make a Github issue with that feature suggestion if you feel strongly about it though.
deadalnix referenced this in commit 93424b1647 on Jul 16, 2020
Munkybooty referenced this in commit 768ec04389 on Aug 21, 2021
Munkybooty referenced this in commit e96f57cf0b on Aug 23, 2021
Munkybooty referenced this in commit 5a5fe18ef9 on Aug 24, 2021
Munkybooty referenced this in commit 17129dce68 on Aug 24, 2021
Munkybooty referenced this in commit 22f4521288 on Aug 24, 2021
UdjinM6 referenced this in commit 900a851427 on Aug 24, 2021
Munkybooty referenced this in commit 2153f8a74a on Aug 24, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15016 [GUI] progress window size

qt: Add workaround for QProgressDialog bug on macOS #15040

Conflicts