Partially fix Bug 388583
Display tooltip at correct position in dolphin when
- Tooltip is displayed above file item, or
- Content width exceeds screen width
CCBUG: 388583
Details
Details
Visual inspection specially for equal distance to file item (3px)
Patch applied:
Currently partial display:
Patch applied:
Currently invisible tooltip (displayed off-screen).
Patch applied:
Diff Detail
Diff Detail
- Repository
- R236 KWidgetsAddons
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Everything seems to work in testing, though I'll admit I have a hard time reproducing the original bug, not being a heavy user of Dolphin's Tooltips.
Comment Actions
Shall I upload some example files here? Truncated videos of course. If so single files or a tgz pack?
Comment Actions
If you upload individual .webm videos, they'll play inline. I like SimpleScreenRecorder for my screen recording needs.
| src/ktooltipwidget.cpp | ||
|---|---|---|
| 152 | 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? | |
| src/ktooltipwidget.cpp | ||
|---|---|---|
| 152 |
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. | |
| src/ktooltipwidget.cpp | ||
|---|---|---|
| 145 | This one is giving me headaches. By all logic it should be -margin. The resulting space is 3x too large in that case. | |
Comment Actions
@michaelh Can you please describe exactly how many bugs are there and how to reproduce them?
Also, ideally each bug should be reproduced by a test case in ktooltipwidgettest.cpp, but I understand that's a lot to ask (so feel free to ignore it).
Comment Actions
Can you please describe exactly how many bugs are there and how to reproduce them?
I can't exactly. There at least to phenomenona
- 2 stages of metadata loading affecting tooltip sizing without position recalculation resulting in partially offscreen display.
This is a very extreme case specific to my setup. This delay albeit much smaller is noticable when accessing e.g. a remote smb-share.
- Truncation as depicted in some screenshots above
I believe baloo or kfilemetadata is responsible for this. I'll have to investigate.
I also believe the positioning issue I'm trying to fix here is quite independent from (to?) the phenomena above.
I understand that's a lot to ask (so feel free to ignore it).
NP, learning to unit test is on my agenda anyway. But it comes at a price :)
- Patience on your side. I will take days.
- Your opinon on the weird arithmetic in line 147. It works for me but I don't know why.
| src/ktooltipwidget.cpp | ||
|---|---|---|
| 152 | Sorry but I'm not the original author of this code, so I don't really know what's going on here. | |
| autotests/ktooltippositiontest.cpp | ||
|---|---|---|
| 84 ↗ | (On Diff #26717) | Any hints? |
| autotests/ktooltippositiontest.cpp | ||
|---|---|---|
| 141 ↗ | (On Diff #26717) | QVERIFY(QTest::qWaitForWindowActive(&tooltip)); ? |
Comment Actions
Not being a heavy Tooltip user like you (and also not having my files as well-tagged), I'm not able to consistently reproduce the problem this is meant to solve.
But in casual use, I also can't detect any negative changes from this patch. Michael, I assume you've been running with this patch for a month and you haven't seen any issues, right?
Comment Actions
Zip does not preserve extended attributes. You need to run ./cmd:tag them in the samples directory.
Ahh I forgot. Tooltip behviour depends on the position of the hovered file relative to the screen. So move the dolphin window around, specially to the lower edge.
| autotests/ktooltippositiontest.cpp | ||
|---|---|---|
| 39–52 ↗ | (On Diff #26940) | Imho these variables could be local to shouldNotObscureTarget(), so that we don't need to define init() and cleanup() and we don't need to make them class members. |
| 65 ↗ | (On Diff #26940) | Weird comma position :p |
| 71 ↗ | (On Diff #26940) | Weird, I usually use qPrintable() for this, but QTest::addRow(qPrintable(QStringLiteral("small/%1").arg(i.key())))still gives me the compiler warning. Not sure what is going on here. |
| 85 ↗ | (On Diff #26940) | Did you mean if (m_screenGeometry.width() >= 1600 && m_screenGeometry.height() >= 900) {
...
}? |
| 111 ↗ | (On Diff #26940) | Weird comma position. Maybe two local QPoint variables could also improve the readability here. |
| 122 ↗ | (On Diff #26940) | Unusual style, we usually do QLabel* contentWidget = new QLabel(content); Also, where is this label deleted? |
| 125 ↗ | (On Diff #26940) | Same as above about the style. But are we sure we want to create a new QWindow? Why not just pass m_container->windowHandle() to showBelow() ? Also, what if the transient parent is null? We should probably add a QVERIFY(m_container->windowHandle()) |
| 131 ↗ | (On Diff #26940) | We can actually keep (and maybe improve) this debug output. qDebug() will only print something if the test fails. |
| 136 ↗ | (On Diff #26940) | This should be a QCOMPARE |
| 139 ↗ | (On Diff #26940) | remove space before ) |
| 148 ↗ | (On Diff #26940) | What do you mean with "test is wrong"? Can this branch ever happen? |
| 151 ↗ | (On Diff #26940) | QVERIFY(tooltip.isHidden()); ? |
| autotests/ktooltippositiontest.h | ||
| 38–39 ↗ | (On Diff #26940) | Is there a reason why we are creating a new test binary only for this test function? Why not use ktooltipwidgettest.cpp? |
| src/ktooltipwidget.cpp | ||
| 124 | centerBelow() is const, but this actually changes the tooltip, right? | |
| 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? | |
| autotests/ktooltippositiontest.cpp | ||
|---|---|---|
| 71 ↗ | (On Diff #26940) | QTest::addRow() requires Qt 5.9. You probably want newRow() instead, which doesn't have printf-style syntax, so it won't give you a printf-style warning. |
Comment Actions
- Apply suggested changes
- Join ktooltippositiontest and ktooltipwidgettest
- Remove negative x constraint
- Make some variables const
| autotests/ktooltippositiontest.cpp | ||
|---|---|---|
| 65 ↗ | (On Diff #26940) | Old habits, sorry. |
| 136 ↗ | (On Diff #26940) | We're letting go of the description with QCOMPARE |
| 148 ↗ | (On Diff #26940) | A better message would have been "michaelh was not thinking correctly" ;-)
Not unless we add conditions which are expected to fail. In that case testing the margin makes no sense. |
| src/ktooltipwidget.cpp | ||
| 124 | Not needed. Dolphin calls adjustSize() on the widget before calling tooltip->showBelow(). | |
| 141 | In addition I cannot reproduce the behaviour depicted here anymore. | |
Comment Actions
If had known I would mess up the inline comments, I would not have moved the code in this step. Sorry.
| src/ktooltipwidget.cpp | ||
|---|---|---|
| 141 | 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. | |
Comment Actions
Almost there! Just minor comments.
| autotests/ktooltipwidgettest.cpp | ||
|---|---|---|
| 195 | Don't use the {} constructor syntax only here, it breaks consistency. | |
| src/ktooltipwidget.cpp | ||
| 32 | Please remove this include. | |
| 126 | Is there a reason why you moved this line? | |
| 131 | unrelated whitespace change | |
| 142 | unrelated whitespace change | |
Comment Actions
Yeah, same to you: Thanks! I'm learning a lot.
Wrt tooltips offscreen display: As I cannot use baloo-widgets here, I'm trying to mock a KFileMetadataWidget. That is extremely tedious and may even not work out. I was told that an integration test would be needed in separate repo. Maybe that is a more solid solution. But I have no idea what to do or where to start. Could you please give me some hints? And, is it worth it?
Also with e.g. D10803 sooner or later tooltips will get in trouble. To make tooltips resilient to info overloads we need a good test and this one is much too simple. To use baloo-widget in a unit/intergration test would be the most flexible solution, I think.