KWin: tooltips
ClosedPublic

Authored by McPain on Dec 12 2017, 9:39 AM.

Details

Summary

BUG: 383040

KDecoration part of the patch:
https://phabricator.kde.org/D7246

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
McPain created this revision.Dec 12 2017, 9:39 AM
Restricted Application added a project: KWin. · View Herald TranscriptDec 12 2017, 9:39 AM
Restricted Application added subscribers: KWin, kwin. · View Herald Transcript
McPain requested review of this revision.Dec 12 2017, 9:39 AM
McPain updated this revision to Diff 23797.

Remove accidently attached russian translation patch

broulik added inline comments.
kwin-5.11.3/decorations/decoratedclient.cpp
203

I would prefer to have two dedicated methods to avoid a boolean trap. requestTooltip(false) is hardly descriptive without knowing the code.

Perhaps requestShowToolTip(const QString &text) and requestHideToolTip()

Shouldn't it be ToolTip (capital Tip)?

Also pass argument as const QString &

kwin-5.11.3/useractions.cpp
1667

Coding style, brace on its on line (for functions)

requestToolTip(QString) and treat an empty string reasonably?

requestToolTip(QString) and treat an empty string reasonably?

That's even better.

McPain updated this revision to Diff 23855.Dec 13 2017, 12:22 PM

Removed "bool trap"

McPain updated this revision to Diff 23859.Dec 13 2017, 12:48 PM

Pass QString as "const QString &"
(noticed that after sending previous patch)

McPain updated this revision to Diff 23863.Dec 13 2017, 1:07 PM
McPain updated this revision to Diff 23867.Dec 13 2017, 1:32 PM
graesslin added inline comments.
kwin-5.11.3/decorations/decoratedclient.h
75

What's the review id for the change in KDecoration?

kwin-5.11.3/kcmkwin/kwindecoration/declarative-plugin/previewclient.cpp
378–386

we don't need that. Just remove the methods.

kwin-5.11.3/useractions.cpp
1669

You cannot use QCursor in KWin. We have a dedicated cursor position class called "Cursor".

1673

coding style: { goes into new line

kwin-5.11.3/workspace.h
281–282

I don't see a need for going through the Workspace. That can be done in DecoratedClientImpl directly.

McPain edited the summary of this revision. (Show Details)Dec 27 2017, 7:27 AM
McPain added inline comments.Dec 28 2017, 11:53 AM
kwin-5.11.3/kcmkwin/kwindecoration/declarative-plugin/previewclient.cpp
378–386

It fails to build without them:

https://pastebin.com/L6cpKsnf

McPain updated this revision to Diff 25079.Jan 10 2018, 12:12 PM
McPain marked 6 inline comments as done.
McPain updated this revision to Diff 25146.Jan 11 2018, 10:15 AM

const QString &

graesslin accepted this revision.Jan 11 2018, 4:36 PM

Looks good. Can go in together with the kdecoration patch.

This revision is now accepted and ready to land.Jan 11 2018, 4:36 PM
McPain added a subscriber: cfeck.Jan 12 2018, 9:29 AM

@graesslin thanks for review.
Can you look at new version of KDecoration part requested by @cfeck ?

@graesslin thanks for review.
Can you look at new version of KDecoration part requested by @cfeck ?

I already did and my accepted still holds

I already did and my accepted still holds

Thanks. How to get commit access or check if it already exists?

I already did and my accepted still holds

Thanks. How to get commit access or check if it already exists?

I can push it for you. But I'll wait till after 5.12 is branched now.

Just ftr: I tried the complete changeset on Wayland and it works very nicely. Good work!

This revision was automatically updated to reflect the committed changes.