Differential D8991

Use kSqueezedTextLabel for the label text on the statusbar
ClosedPublic
Actions

Authored by ahmadsamir on Nov 24 2017, 11:30 PM.

Details

Reviewers
elvisangelaccio
Commits
R318:11e2b186af64: Use kSqueezedTextLabel for the label text on the statusbar
Summary

Following up from D8927; use kSqueezedTextLabel for the label text on the statusbar:

  • This simplifies the code in updateLabelText()
  • Remove the eventFilter as it's not needed any more since kSqueezedTextLabel has a resizeEvent function
  • Specify a stretch factor, 1, for m_label, m_zoomSlider and m_spaceInfo, this prevents the changing of the width of m_label when the label text is updated from changing the widths of the zoomSlider and the spaceInfo widgets as that is a bit too jumpy.

(Thanks to the code of konversation statusbar for the hint about using the stretch factor in addWidget()).

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ahmadsamir created this revision.Nov 24 2017, 11:30 PM
ahmadsamir updated this revision to Diff 22913.Nov 25 2017, 9:31 AM

Re-add #include <QLabel>, this is safer since QLabel is used elsewhere in the code, and KSqueezedTextLabel could change in the future for whatever reasons.

elvisangelaccio added a subscriber: elvisangelaccio.Nov 25 2017, 10:31 AM
elvisangelaccio added inline comments.
src/statusbar/dolphinstatusbar.cpp
130–132

This looks unrelated? Can you explain how to reproduce the "jumpy" issue you mentioned?

315

This cast wouldn't be needed if m_label were declared as KSqueezedTextLabel*.

ahmadsamir added a comment.Nov 25 2017, 3:20 PM
  • To reproduced the issue of the widgets on the statusbar, we just need to remove the stretch factor from the addWidget() call, or set it to zero (default). Here's a screencast showing the issue:

Setting the stretch factor fixes that issue. From http://doc.qt.io/qt-5/qboxlayout.html#addWidget:

If the stretch factor is 0 and nothing else in the QBoxLayout has a stretch factor greater than zero, the space is distributed according to the QWidget:sizePolicy() of each widget that's involved.

  • I'll update the diff; you're right, using KSqueezedTextLabel* works (and I actually missed changing the m_label type in the .h file).
ahmadsamir updated this revision to Diff 22923.Nov 25 2017, 3:21 PM
elvisangelaccio requested changes to this revision.Nov 25 2017, 3:30 PM
In D8991#171872, @ahmadsamir wrote:
  • To reproduced the issue of the widgets on the statusbar, we just need to remove the stretch factor from the addWidget() call, or set it to zero (default).

Ah I see, the problem happens only after porting to KSqueezedTextLabel (which probably has a different size hint).

src/statusbar/dolphinstatusbar.cpp
330–334

Please don't remove the tooltip, we can still show it if m_label->isSqueezed() is true.

This revision now requires changes to proceed.Nov 25 2017, 3:30 PM
ahmadsamir marked 2 inline comments as done.Nov 25 2017, 4:28 PM

I don't understand how phabricator works with inline comments. So just in case my reply to the tooltip part didn't get submitted:

KSqueezedTextLabel does that by default; I just checked my local build, the tooltip is there when the text is squeezed on the statusbar. (And just to be sure https://api.kde.org/frameworks/kwidgetsaddons/html/ksqueezedtextlabel_8cpp_source.html#l00113).

src/statusbar/dolphinstatusbar.cpp
330–334

KSqueezedTextLabel does that by default; I just checked my local build, the tooltip is there when the text is squeezed on the statusbar. (And just to be sure https://api.kde.org/frameworks/kwidgetsaddons/html/ksqueezedtextlabel_8cpp_source.html#l00113).

Thanks :).

elvisangelaccio accepted this revision.Nov 25 2017, 5:27 PM

Oh, you're totally right. Everything looks good then, thanks for the patch!

This revision is now accepted and ready to land.Nov 25 2017, 5:27 PM
Closed by commit R318:11e2b186af64: Use kSqueezedTextLabel for the label text on the statusbar (authored by ahmadsamir, committed by elvisangelaccio). · Explain WhyNov 25 2017, 5:29 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 22928

View Options

src/statusbar/dolphinstatusbar.h

Loading...
View Options

src/statusbar/dolphinstatusbar.cpp

Loading...