fix (kcm): correct output identifier position on wayland
ClosedPublic

Authored by bport on Apr 14 2020, 10:58 AM.

Details

Summary

Wayland don't allow to update client position so we use PlasmaQuick/Dialog
PlasmaQuick/Dialog will talk to kwin throught plasma shell protocol to set dialog position

BUG: 385672

Diff Detail

Repository
R104 KScreen
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 25201
Build 25219: arc lint + arc unit
bport created this revision.Apr 14 2020, 10:58 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 14 2020, 10:58 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bport requested review of this revision.Apr 14 2020, 10:58 AM
bport updated this revision to Diff 80085.Apr 14 2020, 11:19 AM

update commit message

anthonyfieroni added inline comments.
kcm/output_identifier.cpp
58

view leaks, no? I see it's not a problem in this patch.

Technically this introduces a visual change.

Dialog draws a border and shadow by default which appears outside our red border from the QML.

I'm pretty sure you can disable it in Dialog, but frankly this looks way better so maybe it's a feature.

kcm/output_identifier.cpp
58

Not part of your change, but we would leak view here.

I don't think we have a leak, on destructor we delete all view
qDeleteAll(m_views);

bport updated this revision to Diff 80090.Apr 14 2020, 11:45 AM

Remove border and shadow

I don't think we have a leak, on destructor we delete all view
qDeleteAll(m_views);

At that point, when rootObj is nullptr, view is not added to m_views.

I don't think we have a leak, on destructor we delete all view
qDeleteAll(m_views);

At that point, when rootObj is nullptr, view is not added to m_views.

Indeed...

romangg requested changes to this revision.Apr 14 2020, 1:06 PM
romangg removed reviewers: meven, ervin.
romangg added a subscriber: romangg.

Message guideline must be adhered. Summary must be more extensive.

This revision now requires changes to proceed.Apr 14 2020, 1:06 PM

Please explain your solution in the summary. I don't directly understand how this change fixes the bug. Thanks.

kcm/CMakeLists.txt
32

lexicographic order

kcm/output_identifier.cpp
68–69

When you're at it correct the grammar.

kcm/output_identifier.h
24–26

style

bport retitled this revision from Fix KScreen output identifier position on wayland to fix (kcm): correct output identifier position on wayland.Apr 14 2020, 2:05 PM
bport edited the summary of this revision. (Show Details)

Conceptually makes sense. +1 form me on that. Good solution. Thanks for outlining it in the summary.

This patch can land with a fix for the view leak or without. I accept when my inline comments are fixed.

@davidedmundson: can you give a quick accept on the solution from PlasmaQuick pov?

kcm/output_identifier.cpp
54

This part could use some empty lines.

davidedmundson accepted this revision.Apr 14 2020, 2:19 PM

Using the protocol makes sense, using a class that does it already makes more sense than duplicating/

Especially as the other OSD is already a dialog.

From a plasma side, ship it

bport updated this revision to Diff 80120.Apr 14 2020, 2:36 PM

fix leak and style

bport marked 3 inline comments as done.Apr 14 2020, 2:37 PM
romangg accepted this revision.Apr 14 2020, 2:39 PM

Thanks. Some minor remaining nitpicks you can change on push.

kcm/output_identifier.cpp
28

Lexicographical order. Also add an empty line to differentiate include groups:

#include <QStandardPaths>
#include <QTimer>
#include <QQuickItem>
#include <QQuickView>

#include <KDeclarative/kdeclarative/qmlobject.h>
#include <PlasmaQuick/Dialog>
68–69

You wanted to add a . but it became a "

This revision is now accepted and ready to land.Apr 14 2020, 2:39 PM
bport updated this revision to Diff 80121.Apr 14 2020, 2:42 PM

Fix import order (remove unused one)
Remove QTimer m_timer (field not used). Let me know if you want this one in a separate commit.

romangg added a comment.EditedApr 14 2020, 2:53 PM

Yes, please put the m_timer removal in a separate patch. And (in this separate patch) if you removed the m_timer the QTimer include in the header file can be removed as well I assume. Thanks.

This revision was automatically updated to reflect the committed changes.