Changeset View
Standalone View
src/ktooltipwidget.cpp
Show All 23 Lines | |||||
24 | #include <QApplication> | 24 | #include <QApplication> | ||
25 | #include <QPaintEvent> | 25 | #include <QPaintEvent> | ||
26 | #include <QScreen> | 26 | #include <QScreen> | ||
27 | #include <QStylePainter> | 27 | #include <QStylePainter> | ||
28 | #include <QStyleOptionFrame> | 28 | #include <QStyleOptionFrame> | ||
29 | #include <QTimer> | 29 | #include <QTimer> | ||
30 | #include <QVBoxLayout> | 30 | #include <QVBoxLayout> | ||
31 | #include <QWindow> | 31 | #include <QWindow> | ||
32 | 32 | | |||
cfeck: remove unneeded include | |||||
elvisangelaccio: Please remove this include. | |||||
33 | class Q_DECL_HIDDEN KToolTipWidget::KToolTipWidgetPrivate | 33 | class Q_DECL_HIDDEN KToolTipWidget::KToolTipWidgetPrivate | ||
34 | { | 34 | { | ||
35 | public: | 35 | public: | ||
36 | KToolTipWidgetPrivate(KToolTipWidget *parent) | 36 | KToolTipWidgetPrivate(KToolTipWidget *parent) | ||
37 | : q(parent), | 37 | : q(parent), | ||
38 | layout(nullptr), | 38 | layout(nullptr), | ||
39 | content(nullptr), | 39 | content(nullptr), | ||
40 | contentParent(nullptr) | 40 | contentParent(nullptr) | ||
▲ Show 20 Lines • Show All 74 Lines • ▼ Show 20 Line(s) | |||||
115 | } | 115 | } | ||
116 | 116 | | |||
117 | QPoint KToolTipWidget::KToolTipWidgetPrivate::centerBelow(const QRect &rect, QScreen *screen) const | 117 | QPoint KToolTipWidget::KToolTipWidgetPrivate::centerBelow(const QRect &rect, QScreen *screen) const | ||
118 | { | 118 | { | ||
119 | // It must be assured that: | 119 | // It must be assured that: | ||
120 | // - the content is fully visible | 120 | // - the content is fully visible | ||
121 | // - the content is not drawn inside rect | 121 | // - the content is not drawn inside rect | ||
122 | 122 | | |||
123 | const QSize size = content->sizeHint(); | 123 | const QSize size = q->sizeHint(); | ||
124 | const int margin = q->style()->pixelMetric(QStyle::PM_ToolTipLabelFrameWidth); | 124 | const int margin = q->style()->pixelMetric(QStyle::PM_ToolTipLabelFrameWidth); | ||
centerBelow() is const, but this actually changes the tooltip, right? elvisangelaccio: `centerBelow()` is `const`, but this actually changes the tooltip, right?
Maybe we can move… | |||||
Not needed. Dolphin calls adjustSize() on the widget before calling tooltip->showBelow(). michaelh: Not needed. Dolphin calls `adjustSize() ` on the widget before calling `tooltip->showBelow()`. | |||||
125 | const QRect screenGeometry = screen->geometry(); | 125 | const QRect screenGeometry = screen->geometry(); | ||
126 | 126 | | |||
elvisangelaccio: Is there a reason why you moved this line? | |||||
127 | const bool hasRoomToLeft = (rect.left() - size.width() - margin >= screenGeometry.left()); | 127 | const bool hasRoomToLeft = (rect.left() - size.width() - margin >= screenGeometry.left()); | ||
128 | const bool hasRoomToRight = (rect.right() + size.width() + margin <= screenGeometry.right()); | 128 | const bool hasRoomToRight = (rect.right() + size.width() + margin <= screenGeometry.right()); | ||
129 | const bool hasRoomAbove = (rect.top() - size.height() - margin >= screenGeometry.top()); | 129 | const bool hasRoomAbove = (rect.top() - size.height() - margin >= screenGeometry.top()); | ||
130 | const bool hasRoomBelow = (rect.bottom() + size.height() + margin <= screenGeometry.bottom()); | 130 | const bool hasRoomBelow = (rect.bottom() + size.height() + margin <= screenGeometry.bottom()); | ||
131 | if (!hasRoomAbove && !hasRoomBelow && !hasRoomToLeft && !hasRoomToRight) { | 131 | if (!hasRoomAbove && !hasRoomBelow && !hasRoomToLeft && !hasRoomToRight) { | ||
elvisangelaccio: unrelated whitespace change | |||||
132 | return QPoint(); | 132 | return QPoint(); | ||
133 | } | 133 | } | ||
134 | 134 | | |||
135 | int x = 0; | 135 | int x = 0; | ||
136 | int y = 0; | 136 | int y = 0; | ||
137 | if (hasRoomBelow || hasRoomAbove) { | 137 | if (hasRoomBelow || hasRoomAbove) { | ||
138 | x = qMax(screenGeometry.left(), rect.center().x() - size.width() / 2); | 138 | x = qMax(screenGeometry.left(), rect.center().x() - size.width() / 2); | ||
139 | if (x + size.width() >= screenGeometry.right()) { | 139 | if (x + size.width() >= screenGeometry.right()) { | ||
140 | x = screenGeometry.right() - size.width() + 1; | 140 | x = screenGeometry.right() - size.width() + 1; | ||
141 | } | 141 | } | ||
This looks unrelated to this patch. I don't see a testcase for it and if I revert this change, the new tests still pass. Ideally we need a failing test case that is fixed by this line of code. Btw, don't we have a similar "negative y" problem? elvisangelaccio: This looks unrelated to this patch. I don't see a testcase for it and if I revert this change… | |||||
In addition I cannot reproduce the behaviour depicted here anymore. michaelh: In addition I cannot reproduce the behaviour depicted [[ https://phabricator.kde. | |||||
Too happy too soon. This (partial) offscreen display is still happening. The tests cannot catch this, they are much too simple. A genuine baloowidget is needed. I've been told this is not possible because baloo-widgets is tier-3. michaelh: Too happy too soon. This (partial) offscreen display is still happening. The tests cannot catch… | |||||
142 | if (hasRoomBelow) { | 142 | if (hasRoomBelow) { | ||
elvisangelaccio: unrelated whitespace change | |||||
143 | y = rect.bottom() + margin; | 143 | y = rect.bottom() + margin; | ||
144 | } else { | 144 | } else { | ||
145 | y = rect.top() - size.height() - margin; | 145 | y = rect.top() - size.height() - margin + 1; | ||
This one is giving me headaches. By all logic it should be -margin. The resulting space is 3x too large in that case. michaelh: This one is giving me headaches. By all logic it should be -margin. The resulting space is 3x… | |||||
146 | } | 146 | } | ||
147 | } else { | 147 | } else { | ||
148 | Q_ASSERT(hasRoomToLeft || hasRoomToRight); | 148 | Q_ASSERT(hasRoomToLeft || hasRoomToRight); | ||
149 | if (hasRoomToRight) { | 149 | if (hasRoomToRight) { | ||
150 | x = rect.right() + margin; | 150 | x = rect.right() + margin; | ||
151 | } else { | 151 | } else { | ||
152 | x = rect.left() - size.width() - margin; | 152 | x = rect.left() - size.width() - margin + 1; | ||
Does this also need to be adjusted to ' + margin'? Or was the change above accidential? cfeck: Does this also need to be adjusted to ' + margin'? Or was the change above accidential? | |||||
As depicted here long comments also get truncated. As a consequence this part was never reached during testing. michaelh: As depicted [[ https://phabricator.kde.org/file/data/qpklv3xikitbovdlrpbo/PHID-FILE… | |||||
I don't understand. Either + or - is right here, but not both. Are you saying that you don't want to correct it at the second place, just because tests did not reveal a bug? cfeck: I don't understand. Either + or - is right here, but not both. Are you saying that you don't… | |||||
Yes, but it is rather like this: Due to my lack of my experience I don't introduce chances which I can't test or see the results of. More a question of 'dare' than 'want' :) As margin is not included in sizeHint() I think it is correct to do +margin when tooltip is displayed right of rect and -margin when displayed left of rect. Both would result in a margin-wide space between rect and tooltip which is necessary to dismiss the tooltip. michaelh: > Are you saying that you don't want to correct it at the second place, just because tests did… | |||||
Sorry but I'm not the original author of this code, so I don't really know what's going on here. elvisangelaccio: Sorry but I'm not the original author of this code, so I don't really know what's going on here. | |||||
153 | } | 153 | } | ||
154 | // Put the tooltip at the bottom of the screen. The x-coordinate has already | 154 | // Put the tooltip at the bottom of the screen. The x-coordinate has already | ||
155 | // been adjusted, so that no overlapping with rect occurs. | 155 | // been adjusted, so that no overlapping with rect occurs. | ||
156 | y = screenGeometry.bottom() - size.height() + 1; | 156 | y = screenGeometry.bottom() - size.height() + 1; | ||
157 | } | 157 | } | ||
158 | 158 | | |||
159 | return QPoint(x, y); | 159 | return QPoint(x, y); | ||
160 | } | 160 | } | ||
▲ Show 20 Lines • Show All 82 Lines • Show Last 20 Lines |
remove unneeded include