Add custom widget for speed button in the status bar #7427

zeule · 2017-09-11T14:08:30Z

Since a proportional font is used usually for GUI texts, the width
of speed status labels can fluctuate significantly due to speed changes
and digit variability. Because of that the download speed button jitters
in the status bar from left to right, creating unpleasant visual
experience.

Here we replace it with a custom widget, that attempts to allocate
constant-sized placeholders for its text labels, thus eliminating the
jitter.

Example of the problem: https://streamable.com/jhsqz

zeule · 2017-09-11T14:26:19Z

src/gui/statusspeedbutton.h

+public:
+    StatusSpeedButton(QWidget *parent = nullptr);
+
+    void setCurrentSpeed(quint64 currentSpeed);


Maybe joining these 3 into a single call would be better, because each of them calls QWidget::updateGeometry()

From the current use POV it's better. But not from the abstract design POV. Although you can track appropriate value changes (ie. have cached value in your class and compare it with new one).

Although you can track appropriate value changes (ie. have cached value in your class and compare it with new one).

Incidentally, this will prevent unnecessary calculations for those values which are updated rarely (e.g. speed limits).

zeule · 2017-09-11T15:00:10Z

@LordNyriox: there is no need in that, I believe, because status bar will not provide updates in that case.

zeule · 2017-09-11T15:08:24Z

@LordNyriox: thanks! Corrected.

Chocobo1 · 2017-09-11T15:38:49Z

src/base/utils/misc.cpp

+    QString numberPart;
+    if (unit == SizeUnit::Byte) {
+        numberPart = QString(QLatin1String("%1")).arg(bytesValue, fieldWidth, 10, fill);
+    } else {


coding style

thalieht

Coding style

thalieht · 2017-09-11T16:12:44Z

src/gui/statusspeedbutton.cpp

+
+#include <QApplication>
+#include <QStylePainter>
+#include <QStyleOptionButton>


thalieht · 2017-09-11T16:17:27Z

src/gui/statusspeedbutton.h

+* @brief Status bar button with speed text
+*
+*/
+class StatusSpeedButton final: public QPushButton


New rule: spaces around the colon.

glassez · 2017-09-11T16:39:08Z

FYI, StatusBar instance isn't created if it isn't showed and it is deleted when it becomes unused (unvisible from user POV).

glassez · 2017-09-11T16:44:52Z

src/gui/statusbar.cpp

@@ -69,23 +70,13 @@ StatusBar::StatusBar(QWidget *parent)
                .arg(tr("No direct connections. This may indicate network configuration problems.")));
    connect(m_connecStatusLblIcon, &QAbstractButton::clicked, this, &StatusBar::connectionButtonClicked);

-    m_dlSpeedLbl = new QPushButton(this);
+    m_dlSpeedLbl = new StatusSpeedButton(this);


m_dlSpeedLbl -> m_dlSpeedButton

glassez · 2017-09-11T16:45:14Z

src/gui/statusbar.cpp


-    m_upSpeedLbl = new QPushButton(this);
+    m_upSpeedLbl = new StatusSpeedButton(this);


m_upSpeedLbl -> m_upSpeedButton

glassez · 2017-09-11T16:55:06Z

src/gui/statusspeedbutton.cpp

+#include "base/unicodestrings.h"
+#include "base/utils/misc.h"
+
+StatusSpeedButton::StatusSpeedButton(QWidget* parent)


QWidget *parent

glassez · 2017-09-11T16:59:42Z

src/gui/statusspeedbutton.cpp

+}
+
+void StatusSpeedButton::updateSection(QStaticText &section, quint64 bytes, bool isSpeed,
+                                      int minMantissaDigits, const QString& templateText)


Please realign all pointers and references.

glassez · 2017-09-11T17:02:11Z

src/gui/statusspeedbutton.h

+    using base = QPushButton;
+
+public:
+    StatusSpeedButton(QWidget *parent = nullptr);


glassez · 2017-09-11T17:02:58Z

src/gui/statusspeedbutton.h

+    void setSpeedLimit(int speedLimit);
+    void setTotalPayload(quint64 totalPayload);
+
+protected:


protected is meaningless in final class

It's not final now.

glassez · 2017-09-11T17:06:48Z

src/gui/statusspeedbutton.h

+public:
+    StatusSpeedButton(QWidget *parent = nullptr);
+
+    void setCurrentSpeed(quint64 currentSpeed);


From the current use POV it's better. But not from the abstract design POV. Although you can track appropriate value changes (ie. have cached value in your class and compare it with new one).

glassez · 2017-09-11T17:09:08Z

src/gui/statusspeedbutton.h

+public:
+    StatusSpeedButton(QWidget *parent = nullptr);
+
+    void setCurrentSpeed(quint64 currentSpeed);


Although you can track appropriate value changes (ie. have cached value in your class and compare it with new one).

Incidentally, this will prevent unnecessary calculations for those values which are updated rarely (e.g. speed limits).

zeule · 2017-09-11T22:15:13Z

Review comments addressed. Thanks!

Chocobo1 · 2017-09-12T07:02:56Z

src/gui/statusspeedbutton.cpp

 #include <QTextOption>

 #include "base/unicodestrings.h"
 #include "base/utils/misc.h"

+StatusSpeedButton::Section::Section(const QString& templateText, int minDigits,


const QString &templateText

Chocobo1 · 2017-09-12T07:12:41Z

src/gui/statusspeedbutton.cpp

    }
 }

-QStaticText StatusSpeedButton::emptyText()
+QChar StatusSpeedButton::widestDecimalDigit()


I would add const even used internally.

Chocobo1 · 2017-09-12T07:15:57Z

src/gui/statusspeedbutton.cpp

-    res.setTextFormat(Qt::PlainText);
-    res.setTextOption({Qt::AlignRight});
-    return res;
+    const QString digits = QLocale().toString(1234567890);


why QLocale here, does it do anything special?

I need localised digits as would be produced by QString::arg(), but here I don't need the template string.

I understand it now.
Not related to this PR, just my opinion, do you really expect to see a localized number string in the button? I mean I prefer plain number, commas or anything else for separator is redundant IMO.

I'm weak in locale-related things. Just wanted to avoid any assumptions that might fail.

Chocobo1 · 2017-09-12T07:24:03Z

src/gui/statusspeedbutton.cpp

+    qreal maxWidth = 0.;
+    QChar digit;
+    QFontMetricsF fm {font(), this};
+    for (int i = 0; i < digits.size(); ++i) {


minor thing, you can use range based for loop here.

Chocobo1 · 2017-09-12T07:33:52Z

Asking a naive question here: in git master code, what about recording the largest width of the speed limit button and set it every time when it updates, well... assuming the width size don't explode.

zeule · 2017-09-12T08:16:30Z

what about recording the largest width of the speed limit button and set it every time when it updates,

I actually do exactly that for speed label, just allocate that space a bit in advance. But to get a stable position of the label (each of them), you also have to right-alight it. From there we are only a step away from the approach of this PR. Namely: should we try to predict the possible widest label or not.

Perhaps we simply measure width of the unit strings (once on initialisation), select the widest and just allocate space for "1023.99 "? But that labels will look quite strangely spaced most of the time.

Chocobo1 · 2017-09-12T08:45:03Z

Thanks for the explain!
I've a bit more concerns:

what about RTL locales? will it work?
I think maybe the overall approach is a bit complex for such a UI element (both code & memory), I hope there could be a simpler solution
Although you mentioned jitter and I understand what it means, I cannot observe it on my system (Sans Regular 10pt font here), can you describe it a bit more?
So the jitter is caused by different glyph widths in font, so instead of using fixed-width spaces for separating fields, maybe you should use tabs instead, but I cannot verify this as I didn't experience the jitter symptom.
another bug found: the up arrow icon & the down arrow icon is shifted a few pixels down:

zeule · 2017-09-12T08:59:29Z

what about RTL locales? will it work?

Oh, thanks! Need to use left-alignment for them.

I think maybe the overall approach is a bit complex

Agree with you. But can't help myself, want smooth UI so much :)

Although you mentioned jitter and I understand what it means, I cannot observe it on my system (Sans Regular 10pt font here), can you describe it a bit more?

Here is a screen record, please: https://streamable.com/jhsqz

another bug found:

What QStyle do you use, please? I don't see anything like that.

Chocobo1 · 2017-09-12T13:14:47Z

Agree with you. But can't help myself, want smooth UI so much :)

Let me experiment a little...

Here is a screen record, please: https://streamable.com/jhsqz

OK, thanks!

What QStyle do you use, please? I don't see anything like that.

Fusion I guess.

zeule · 2017-09-12T13:48:09Z

What QStyle do you use, please? I don't see anything like that.

Fusion I guess.

Wrong metric was used. Should be correct now.

zeule · 2017-09-14T17:36:06Z

Do I correctly understand from your comments, that this PR contains too much of code and is too complicated for the task?

Since a proportional font is used usually for GUI texts, the width of speed status labels can fluctuate significantly due to speed changes and digit variability. Because of that the download speed button jitters in the status bar from left to right, creating unpleasant visual experience. Here we replace it with a custom widget, that attempts to allocate constatn-sized placeholders for its text labels, thus eliminating the jitter.

d-s-x · 2017-10-13T19:04:42Z

@evsh,

this PR contains too much of code and is too complicated for the task?

Here is a screen record, please: https://streamable.com/jhsqz

When no limit is set there is a plenty of free space for both download and upload speeds.
That means that there is a minimal width applied to the labels.
When a limit is enabled (easy to reproduce with 100 KiB/s upload limit) width of the text exceeds the minimal width and thus the status bar "jitters".

Simplest solution is to increase width for the labels and made it fixed (not dependent on content).
This usually requires 2 lines changed (not 372 as in this PR)

glassez · 2019-12-10T08:48:29Z

@Chocobo1, @sledgehammer999, are we interested in this feature? (given the fact that the author has been inactive for a long time)

Chocobo1 · 2019-12-10T12:06:53Z

@Chocobo1, @sledgehammer999, are we interested in this feature? (given the fact that the author has been inactive for a long time)

IIRC I did still see some resizing quirks, although much less often. And I think the implementation is maybe a little bit too complex.

glassez · 2019-12-12T11:06:21Z

And I think the implementation is maybe a little bit too complex.

As I said before the implementation is too complex for this job.

zeule requested review from sledgehammer999, glassez and Chocobo1 September 11, 2017 14:08

zeule added the GUI GUI-related issues/changes label Sep 11, 2017

zeule commented Sep 11, 2017

View reviewed changes

Chocobo1 reviewed Sep 11, 2017

View reviewed changes

thalieht reviewed Sep 11, 2017

View reviewed changes

glassez requested changes Sep 11, 2017

View reviewed changes

Chocobo1 reviewed Sep 12, 2017

View reviewed changes

FranciscoPombal added the PR abandoned label Mar 7, 2020

FranciscoPombal closed this Mar 7, 2020


		m_upSpeedLbl = new QPushButton(this);
		m_upSpeedLbl = new StatusSpeedButton(this);

Add custom widget for speed button in the status bar #7427

Add custom widget for speed button in the status bar #7427

Conversation

zeule commented Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule commented Sep 11, 2017

zeule commented Sep 11, 2017

Choose a reason for hiding this comment

thalieht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassez commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 commented Sep 12, 2017 • edited Loading

zeule commented Sep 12, 2017

Chocobo1 commented Sep 12, 2017 • edited Loading

zeule commented Sep 12, 2017

Chocobo1 commented Sep 12, 2017

zeule commented Sep 12, 2017

zeule commented Sep 14, 2017

d-s-x commented Oct 13, 2017 • edited Loading

glassez commented Dec 10, 2019

Chocobo1 commented Dec 10, 2019

glassez commented Dec 12, 2019

zeule commented Sep 11, 2017 •

edited

Loading

Chocobo1 Sep 12, 2017 •

edited

Loading

Chocobo1 commented Sep 12, 2017 •

edited

Loading

Chocobo1 commented Sep 12, 2017 •

edited

Loading

d-s-x commented Oct 13, 2017 •

edited

Loading