D9973: ktooltipwidget: Fix tooltip positioning

elvisangelaccio added a comment. In D9973#223039 , @michaelh wrote: > In D9973#223017 , @elvisangelaccio wrote: > > > Thanks! :) > > > Yeah, same to you: Thanks! I'm learning a lot. > Wrt

D9973: ktooltipwidget: Fix tooltip positioning

This revision was automatically updated to reflect the committed changes. Closed by commit R236:f94c55fb190f: ktooltipwidget: Fix tooltip positioning (authored by michaelh). REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9973?vs=29205=29213 REVISION

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added a comment. In D9973#223017 , @elvisangelaccio wrote: > Thanks! :) 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

D9973: ktooltipwidget: Fix tooltip positioning

elvisangelaccio removed reviewers: Frameworks, Dolphin. Restricted Application added a subscriber: Frameworks. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio, ngraham, #dolphin Cc: #frameworks, dfaure, cfeck, michaelh

D9973: ktooltipwidget: Fix tooltip positioning

elvisangelaccio accepted this revision. elvisangelaccio added a comment. This revision is now accepted and ready to land. Thanks! :) REPOSITORY R236 KWidgetsAddons BRANCH tooltip_and_test (branched from master) REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh,

D9973: ktooltipwidget: Fix tooltip positioning

michaelh updated this revision to Diff 29205. michaelh added a comment. - Reinsert white space REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9973?vs=29204=29205 BRANCH tooltip_and_test (branched from master) REVISION DETAIL

D9973: ktooltipwidget: Fix tooltip positioning

michaelh updated this revision to Diff 29204. michaelh added a comment. - Apply suggested changes REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9973?vs=28731=29204 BRANCH tooltip_and_test (branched from master) REVISION DETAIL

D9973: ktooltipwidget: Fix tooltip positioning

elvisangelaccio added a comment. Almost there! Just minor comments. INLINE COMMENTS > ktooltipwidgettest.cpp:195 > +QLabel* contentWidget = new QLabel(content); > +const QSize shrinkSize{2 * m_offset, 2 * m_offset}; > +contentWidget->setMaximumSize(m_screenGeometry.size() -

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added inline comments. INLINE COMMENTS > michaelh wrote in ktooltipwidget.cpp:142 > In addition I cannot reproduce the behaviour depicted here > > anymore. Too happy

D9973: ktooltipwidget: Fix tooltip positioning

michaelh marked 2 inline comments as done. michaelh added a comment. If had known I would mess up the inline comments, I would not have moved the code in this step. Sorry. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio,

D9973: ktooltipwidget: Fix tooltip positioning

michaelh marked 2 inline comments as done. michaelh added inline comments. INLINE COMMENTS > elvisangelaccio wrote in ktooltippositiontest.cpp:65 > Weird comma position :p Old habits, sorry. > elvisangelaccio wrote in ktooltippositiontest.cpp:136 > This should be a QCOMPARE We're letting go

D9973: ktooltipwidget: Fix tooltip positioning

michaelh updated this revision to Diff 28731. michaelh marked 12 inline comments as done. michaelh added a comment. - Apply suggested changes - Join ktooltippositiontest and ktooltipwidgettest - Remove negative x constraint - Make some variables const REPOSITORY R236 KWidgetsAddons

D9973: ktooltipwidget: Fix tooltip positioning

dfaure added inline comments. INLINE COMMENTS > elvisangelaccio wrote in ktooltippositiontest.cpp:71 > 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

D9973: ktooltipwidget: Fix tooltip positioning

elvisangelaccio requested changes to this revision. elvisangelaccio added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > ktooltippositiontest.cpp:39-52 > +void KTooltipPositionTest::init() > +{ > +m_container = new QWidget(); > +m_target = new

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added a comment. In D9973#217482 , @ngraham wrote: > 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. F5738074:

D9973: ktooltipwidget: Fix tooltip positioning

elvisangelaccio added a comment. @michaelh I'll try to have a look this weekend. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio, #frameworks, #dolphin, ngraham Cc: cfeck, michaelh

D9973: ktooltipwidget: Fix tooltip positioning

ngraham added a comment. 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

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added a comment. Ping. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio, #frameworks, #dolphin, ngraham Cc: cfeck, michaelh

D9973: ktooltipwidget: Fix tooltip positioning

michaelh marked an inline comment as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio, #frameworks, #dolphin, ngraham Cc: cfeck, michaelh

D9973: ktooltipwidget: Fix tooltip positioning

michaelh updated this revision to Diff 26940. michaelh added a comment. - Clean code REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9973?vs=26717=26940 BRANCH tooltip_and_test REVISION DETAIL https://phabricator.kde.org/D9973 AFFECTED FILES

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added inline comments. INLINE COMMENTS > ktooltippositiontest.cpp:141 > +KToolTipWidget tooltip; > +tooltip.showBelow(targetRect, contentWidget, transientParent); > +QRect tooltipRect(tooltip.frameGeometry()); `QVERIFY(QTest::qWaitForWindowActive());` ? ​ REPOSITORY R236

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added inline comments. INLINE COMMENTS > ktooltippositiontest.cpp:84 > +i.next(); > +//FIXME: Compose names w/o compiler warning -Wformat-security > + > QTest::addRow(QStringLiteral("small/%1").arg(i.key()).toLatin1().constData()) Any hints? REPOSITORY R236

D9973: ktooltipwidget: Fix tooltip positioning

michaelh marked 3 inline comments as done. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio, #frameworks, #dolphin, ngraham Cc: cfeck, michaelh, ngraham

D9973: ktooltipwidget: Fix tooltip positioning

michaelh updated this revision to Diff 26717. michaelh added a comment. - Correct author email REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9973?vs=26715=26717 BRANCH tooltip_and_test REVISION DETAIL https://phabricator.kde.org/D9973

D9973: ktooltipwidget: Fix tooltip positioning

michaelh updated this revision to Diff 26715. michaelh edited the test plan for this revision. michaelh added a comment. - Add KTooltipPositionTest to autotests - Make KToolTipWidget pass test REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE

D9973: ktooltipwidget: Fix tooltip positioning

michaelh edited the summary of this revision. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio, #frameworks, #dolphin, ngraham Cc: cfeck

D9973: ktooltipwidget: Fix tooltip positioning

elvisangelaccio added inline comments. INLINE COMMENTS > michaelh wrote in ktooltipwidget.cpp:155 > > Are you saying that you don't want to correct it at the second place, just > > because tests did not reveal a bug? > > Yes, but it is rather like this: Due to my lack of my experience I don't

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added a comment. > Can you please describe exactly how many bugs are there and how to reproduce them? I can't exactly. There at least to phenomenona 1. 2 stages of metadata loading most likely affecting tooltip sizing F5665675: tooltip.mkv

D9973: ktooltipwidget: Fix tooltip positioning

elvisangelaccio added a comment. @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).

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added inline comments. INLINE COMMENTS > ktooltipwidget.cpp:147 > } else { > -y = rect.top() - size.height() - margin; > +y = rect.top() - size.height() + margin; > } This one is giving me headaches. By all logic it should be -margin. The

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added inline comments. INLINE COMMENTS > cfeck wrote in ktooltipwidget.cpp:155 > 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? > Are you saying that you

D9973: ktooltipwidget: Fix tooltip positioning

cfeck added inline comments. INLINE COMMENTS > michaelh wrote in ktooltipwidget.cpp:155 > As depicted here > > long comments also get truncated. As a consequence this

D9973: ktooltipwidget: Fix tooltip positioning

ngraham added a comment. If you upload individual .webm videos, they'll play inline. I like SimpleScreenRecorder for my screen recording needs. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio, #frameworks, #dolphin, ngraham

D9973: ktooltipwidget: Fix tooltip positioning

michaelh added a comment. Shall I upload some example files here? Truncated videos of course. If so single files or a zipped pack? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio, #frameworks, #dolphin, ngraham Cc: cfeck

D9973: ktooltipwidget: Fix tooltip positioning

ngraham added a comment. 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. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio,

D9973: ktooltipwidget: Fix tooltip positioning

michaelh marked an inline comment as done. michaelh added inline comments. INLINE COMMENTS > cfeck wrote in ktooltipwidget.cpp:155 > Does this also need to be adjusted to ' + margin'? Or was the change above > accidential? As depicted here

D9973: ktooltipwidget: Fix tooltip positioning

michaelh updated this revision to Diff 25626. michaelh added a comment. Removed obsolete include REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9973?vs=25613=25626 BRANCH tooltip (branched from master) REVISION DETAIL

D9973: ktooltipwidget: Fix tooltip positioning

michaelh retitled this revision from "[kwidgetsaddons] Fix tooltip positioning" to "ktooltipwidget: Fix tooltip positioning". michaelh edited the summary of this revision. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio,