BUG: 383040
KWin part of the patch:
https://phabricator.kde.org/D9294
graesslin |
Breeze | |
Plasma |
BUG: 383040
KWin part of the patch:
https://phabricator.kde.org/D9294
Lint Skipped |
Unit Tests Skipped |
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.
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.
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(); }
kdecoration-5.11.3/CMakeLists.txt | ||
---|---|---|
27 ↗ | (On Diff #23866) | I don't see Widgets used anywhere. |
kdecoration-5.11.3/src/CMakeLists.txt | ||
1 ↗ | (On Diff #23866) | This is not kcmkwin, but kdecoration |
22 ↗ | (On Diff #23866) | I don't see Widgets used anywhere |
kdecoration-5.11.3/src/decorationbutton.cpp | ||
302 ↗ | (On Diff #23866) | If I see correctly this could become a lambda function which would not require to add a showTooltip method |
512–513 ↗ | (On Diff #23866) | 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 ↗ | (On Diff #23866) | I think this method is not needed (see comment about the lambda connection) |
158 ↗ | (On Diff #23866) | This should not be in the public header. Please move to private |
167–168 ↗ | (On Diff #23866) | I don't think we need those signals. |
kdecoration-5.11.3/src/private/decoratedclientprivate.h | ||
79–80 ↗ | (On Diff #23866) | If we add pure virtuals we need to increase the so version of the private libraru. |
kdecoration-5.11.3/src/private/decoratedclientprivate.h | ||
---|---|---|
79–80 ↗ | (On Diff #23866) | I need to avoid sover increasing, right? |
kdecoration-5.11.3/src/private/decoratedclientprivate.h | ||
---|---|---|
79–80 ↗ | (On Diff #23866) | 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. |
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.
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....