Tooltips
ClosedPublic

Authored by McPain on Aug 11 2017, 8:02 AM.

Details

Reviewers
graesslin
Group Reviewers
Breeze
Plasma
Commits
R129:24a859b356ef: Tooltips
Summary

BUG: 383040

KWin part of the patch:
https://phabricator.kde.org/D9294

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
McPain created this revision.Aug 11 2017, 8:02 AM
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptAug 11 2017, 8:02 AM

Thanks for your patch, however I think this should rather to into KDecoration itself so every decoration, not just Breeze, takes advantage of it. However, from what I can tell KDecoration is QtGui-only (not QtWidgets), I looked into it once but didn't want to pull in widgets just for QToolTip.

@McPain, any update?

Not now. I don't get what to do: rewrite entire patch or write it for oxygen?

ngraham edited the summary of this revision. (Show Details)Nov 22 2017, 2:22 PM

Probably redo the patch so that it can apply to any theme.

McPain updated this revision to Diff 22872.Nov 24 2017, 9:09 AM
McPain retitled this revision from Tooltips in Breeze theme to Tooltips.
McPain changed the repository for this revision from R31 Breeze to R129 Window Decoration Library.
McPain updated this revision to Diff 22886.Nov 24 2017, 11:10 AM

I'm sorry, but Widgets is a dependency which cannot be used for it. This creates problems in KWin. Tooltip support must be implemented directly in KWin, exposed through the KDecoration API. Yes I know that makes it rather complex, but that's the reason why tooltip support wasn't added to KDecoration through QWidget in the first place.

I'm sorry, but Widgets is a dependency which cannot be used for it. This creates problems in KWin. Tooltip support must be implemented directly in KWin, exposed through the KDecoration API. Yes I know that makes it rather complex, but that's the reason why tooltip support wasn't added to KDecoration through QWidget in the first place.

I thought a little bit more about it. It might be possible to use QWidgets if it's done inside KWin. That might work correctly. If it's not directly controlled by KWin, it's problematic and will most likely not work correctly. To explain: KWin has difficulties creating windows. I know it sounds stupid, that the window manager is not able to create windows, but that's how it is. KWin has to do quite some tricks and track windows internally. If a window gets created by a library loaded into KWin and KWin doesn't know about it, such internal tracking breaks. It now has a window it doesn't know about. That's especially a problem on Wayland, on X11 it works better for windows such as a tooltip.

But long story short: for KWin the best is a request from KDecoration "showTooltip(const QString &)" and KWin creates the tooltip window, positions it and ensures it works correctly.

McPain added a comment.Dec 8 2017, 8:52 AM

But long story short: for KWin the best is a request from KDecoration "showTooltip(const QString &)" and KWin creates the tooltip window, positions it and ensures it works correctly.

Any idea where exactly in KWin it should be?

But long story short: for KWin the best is a request from KDecoration "showTooltip(const QString &)" and KWin creates the tooltip window, positions it and ensures it works correctly.

Any idea where exactly in KWin it should be?

In KWin sources there is a subdirectory called decorations. Inside that is the complete decoration integration code. I guess the best place might be in the bridge or in the decorated client impl. So a request from KDecoration could go through the bridge where KWin can create the tooltip.

One more question:

can this cause a memory leak?

void Workspace::showTooltip(const QString& text, const QPoint &pos, bool show){
    show ? QToolTip::showText(pos, text) : QToolTip::hideText();
}

One more question:

can this cause a memory leak?

void Workspace::showTooltip(const QString& text, const QPoint &pos, bool show){
    show ? QToolTip::showText(pos, text) : QToolTip::hideText();
}

I think that's safe.

McPain updated this revision to Diff 23794.Dec 12 2017, 9:38 AM

Moved QToolTip::showText to KWin

McPain edited the summary of this revision. (Show Details)Dec 12 2017, 9:39 AM
McPain updated this revision to Diff 23854.Dec 13 2017, 12:22 PM

Removed "bool trap"

McPain updated this revision to Diff 23857.Dec 13 2017, 12:46 PM
McPain updated this revision to Diff 23862.Dec 13 2017, 1:06 PM
McPain updated this revision to Diff 23866.Dec 13 2017, 1:32 PM
graesslin requested changes to this revision.Dec 27 2017, 11:52 AM
graesslin added inline comments.
kdecoration-5.11.3/CMakeLists.txt
27

I don't see Widgets used anywhere.

kdecoration-5.11.3/src/CMakeLists.txt
2

This is not kcmkwin, but kdecoration

22

I don't see Widgets used anywhere

kdecoration-5.11.3/src/decorationbutton.cpp
302

If I see correctly this could become a lambda function which would not require to add a showTooltip method

512–513

Instead of emitting the signal please invoke the method requestShowTooltip directly (I don't think we need to queue it)

kdecoration-5.11.3/src/decorationbutton.h
155–156

I think this method is not needed (see comment about the lambda connection)

158

This should not be in the public header. Please move to private

167–168

I don't think we need those signals.

kdecoration-5.11.3/src/private/decoratedclientprivate.h
79–80

If we add pure virtuals we need to increase the so version of the private libraru.

This revision now requires changes to proceed.Dec 27 2017, 11:52 AM
McPain added inline comments.Dec 28 2017, 11:53 AM
kdecoration-5.11.3/src/private/decoratedclientprivate.h
79–80

I need to avoid sover increasing, right?

graesslin added inline comments.Dec 29 2017, 4:55 PM
kdecoration-5.11.3/src/private/decoratedclientprivate.h
79–80

No, increasing the soversion of the private library is fine. It's only used by KWin and by KDecoration public library. The interface to the decoration plugins (like breeze) is still stable. As KDecoration and KWin are released together it's not a problem. But we should try to get it in before the beta release as distros might need to adjust packaging.

McPain updated this revision to Diff 25083.Jan 10 2018, 12:23 PM
McPain marked 11 inline comments as done.
McPain updated this revision to Diff 25084.Jan 10 2018, 12:32 PM

fix build

graesslin accepted this revision.Jan 10 2018, 9:18 PM
graesslin added subscribers: jriddell, davidedmundson.

We are close to release and this might create hassles for distributions. Should we include it or wait for master being open again? @davidedmundson, @jriddell: what's your opinion?

My personal opinion is that we should try to get it into 5.12 given that this was in the known issues section since the introduction of kdecoration2.

This revision is now accepted and ready to land.Jan 10 2018, 9:18 PM
cfeck added a subscriber: cfeck.Jan 10 2018, 9:40 PM
cfeck added inline comments.
src/decoration.h
179 ↗(On Diff #25084)

const QString &text

src/decorationbutton.cpp
292 ↗(On Diff #25084)

empty line

src/decorationbutton_p.h
69 ↗(On Diff #25084)

Does kwin use this "spaces inside parentheses" style?

src/private/decoratedclientprivate.h
79 ↗(On Diff #25084)

const QString &

McPain updated this revision to Diff 25145.Jan 11 2018, 10:15 AM
McPain marked 4 inline comments as done.

const QString &
cleanup

graesslin requested changes to this revision.Jan 16 2018, 5:20 PM

I get compile errors:

/home/martin/src/kf5/kde/workspace/kdecoration/autotests/mockbridge.cpp: In member function ‘virtual std::unique_ptr<KDecoration2::DecoratedClientPrivate> MockBridge::createClient(KDecoration2::DecoratedClient*, KDecoration2::Decoration*)’:
/home/martin/src/kf5/kde/workspace/kdecoration/autotests/mockbridge.cpp:27:77: error: invalid new-expression of abstract class type ‘MockClient’
     auto ptr = std::unique_ptr<MockClient>(new MockClient(client, decoration));
                                                                             ^
In file included from /home/martin/src/kf5/kde/workspace/kdecoration/autotests/mockbridge.cpp:21:0:
/home/martin/src/kf5/kde/workspace/kdecoration/autotests/mockclient.h:27:7: note:   because the following virtual functions are pure within ‘MockClient’:
 class MockClient : public QObject, public KDecoration2::ApplicationMenuEnabledDecoratedClientPrivate
       ^~~~~~~~~~
In file included from /home/martin/src/kf5/kde/workspace/kdecoration/autotests/mockclient.h:23:0,
                 from /home/martin/src/kf5/kde/workspace/kdecoration/autotests/mockbridge.cpp:21:
/home/martin/src/kf5/kde/workspace/kdecoration/autotests/../src/private/decoratedclientprivate.h:79:18: note:   virtual void KDecoration2::DecoratedClientPrivate::requestShowToolTip(const QString&)
     virtual void requestShowToolTip(const QString &text) = 0;
                  ^~~~~~~~~~~~~~~~~~
/home/martin/src/kf5/kde/workspace/kdecoration/autotests/../src/private/decoratedclientprivate.h:80:18: note:   virtual void KDecoration2::DecoratedClientPrivate::requestHideToolTip()
     virtual void requestHideToolTip() = 0;
                  ^~~~~~~~~~~~~~~~~~
[ 72%] Building CXX object autotests/CMakeFiles/decorationButtonTest.dir/mockdecoration.cpp.o
autotests/CMakeFiles/decorationTest.dir/build.make:62: recipe for target 'autotests/CMakeFiles/decorationTest.dir/mockbridge.cpp.o' failed
make[2]: *** [autotests/CMakeFiles/decorationTest.dir/mockbridge.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
This revision now requires changes to proceed.Jan 16 2018, 5:20 PM
McPain updated this revision to Diff 25539.Jan 17 2018, 1:40 PM

Fix autotests build

graesslin accepted this revision.Jan 18 2018, 6:45 PM
This revision is now accepted and ready to land.Jan 18 2018, 6:45 PM
This revision was automatically updated to reflect the committed changes.