D12388: Output device color curves correction

[Vlad Zagorodniy]

zzag added a comment.


const uint16_t *pos = (uint16_t*)array->data;
  
  Well, now you throw away const qualifiers. I think it should be something 
like this
  
const uint16_t *pos = reinterpret_cast(array->data);
  
  I suggest to get rid of C style casts because they aren't checked by the 
compiler.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D12388

To: romangg, #frameworks, davidedmundson, graesslin
Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns

D12388: Output device color curves correction

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> zzag wrote in outputconfiguration_interface.cpp:221
> constify wl_array?
> constify pos, e.g. `uint16_t const *pos`?

You can resize v and call memcpy (or use std::copy?)

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D12388

To: romangg, #frameworks, davidedmundson, graesslin
Cc: graesslin, davidedmundson, zzag, cfeck, michaelh, bruns

D12820: Add KWayland virtual desktop protocol

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> plasmavirtualdesktop_interface.cpp:36
> +
> +class PlasmaVirtualDesktopInterface::Private
> +{

class Q_DECL_HIDDEN PlasmaVirtualDesktopInterface::Private

Same for the server part.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D12820

To: mart, #kwin, #plasma, graesslin, hein
Cc: zzag, bshah, romangg, kde-frameworks-devel, michaelh, ngraham, bruns

D13034: Add mechanism to notify other clients of config changes over DBus

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> kconfig.cpp:526
> +QDBusConnection::sessionBus().send(message);
> +#endif
> +}

#else
  Q_UNUSED(changes);
  Q_UNUSED(path);
  #endif

> kconfigwatcher.cpp:82
> +KConfigGroup group = d->m_config->group(QString());//top level group
> +for(const QString &groupName: it.key().split(QLatin1Char('\x1d'))) { 
> //magic char, see KConfig
> +group = group.group(groupName);

Won't this detach?

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D13034

To: davidedmundson
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag created this revision.
zzag added a reviewer: Plasma.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  In some cases, when rendering frame svg background, measures & margins
  do not correspond to `enabledBorders`. I.e. `bottomHeight` may be equal to 5,
  but the bottom border is disabled. This causes visual artifacts like this
  
  <%= image %>
  
  There are several approaches to fix this problem:
  
  - Rewrite the `contentGeometry` method, so it takes into account whether the 
left/top/right/bottom border is enabled.
  
  - Or, call `updateSizes` before rendering frame svg background.
  
  I took the latter approach in this diff.
  
  Just for reference, this is "after"
  
  <%= image %>
  
  BUG: 382324
  BUG: 390632
  BUG: 391659

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix-framesvg-contentrect

REVISION DETAIL
  https://phabricator.kde.org/D13215

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag added a comment.


  In D13215#270878 , @broulik wrote:
  
  > Wow. I wasted hours in FrameSvg trying to find this. Does this also fix the 
panel mask in non-composited mode, e.g. round corners with Oxygen? (Quickly 
browsing the bug numbers suggests it does \o/)
  
  
  It has nothing to do with compositing. ;-)
  
  I noticed when I was working on this diff that paintCorner is not working 
_really_ correct. I'll check it with Oxygen.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag added a comment.


  @broulik I tested panels with Oxygen(with compositing on and off) and didn't 
see any problems.
  
  > (Quickly browsing the bug numbers suggests it does \o/)
  
  Not sure about that. All of those bugs are "Breeze-related".

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13218: Draw FrameSVG corners only if both borders in both directions are enabled

[Vlad Zagorodniy]

zzag created this revision.
zzag added a reviewer: Plasma.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  FrameSVG documentation states
  
  > ... topleft and topright will be ignored if top does not exist,
  >  and similarly for bottomleft and bottomright ...
  
  Someone would assume that the same behaviour applies to the case
  when left(or right) does not exists.
  
  This change makes FrameSVG to comply with such expected behaviour.
  
  Before
  
  <%= image %>
  
  The bottom border doesn't exist so the bottom-right corner should not
  be drawn.
  
  After
  
  <%= image %>

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix-paintcorner

REVISION DETAIL
  https://phabricator.kde.org/D13218

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13218: Draw FrameSVG corners only if both borders in both directions are enabled

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13218

To: zzag, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13218: Draw FrameSVG corners only if both borders in both directions are enabled

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13218

To: zzag, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag added a reviewer: Frameworks.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13218: Draw FrameSVG corners only if both borders in both directions are enabled

[Vlad Zagorodniy]

zzag added a reviewer: Frameworks.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13218

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13218: Draw FrameSVG corners only if both borders in both directions are enabled

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13218

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13238: [tests] Add pointer constraints test

[Vlad Zagorodniy]

zzag added a comment.


  Some nitpicks. :-)

INLINE COMMENTS

> pointerconstraints.cpp:50
> +
> +void WaylandBackend::init(MainWindow *window) {
> +Backend::init(window);

Coding style nitpick: put `{` on a new line.

> pointerconstraints.cpp:169
> +
> +auto *confinedPointer = 
> m_pointerConstraints->confinePointer(winSurface.data(),
> + m_pointer,

IMHO, that's not the best case to use auto.

> pointerconstraints.cpp:175
> +
> +if (confinedPointer) {
> +m_confinedPointer = confinedPointer;

What about

  if (!confinedPointer) {
  qDebug() << "ERROR when receiving confined pointer!";
  return;
  }
  
  // ...

?

> pointerconstraints.cpp:324
> +return QRect();
> +} else {
> +return QRect(m_testAreaInInput->mapTo(this, QPoint(0,0)), 
> m_testAreaInInput->size());

Coding style nitpick: no else after return.

> pointerconstraints.cpp:402
> +
> +#include "pointerconstraints.moc"

Is there a reason why the moc file is included?

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D13238

To: romangg, #plasma, #frameworks
Cc: zzag, kde-frameworks-devel, ragreen, Pitel, schernikov, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag added subscribers: aseigo, mart.
zzag added a comment.


  In D13215#271983 , @broulik wrote:
  
  > +1
  >
  > Doesn't make a difference in Oxygen's rounded corners on panels but doesn't 
make it worse either. Please also profile whether it causes any needless 
recalculations on Plasma startup
  
  
  I'd like to dig deeper why margins do not correspond to enabled margins. 
Maybe, because of
  
if (!d->repaintBlocked) {
   d->updateFrameData();
}
  
  FrameSVG misses to update margins, and so on.
  
  I really would like to here opinion of @mart and @aseigo . Maybe, there is a 
better way to fix this.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag added a comment.


  In D13215#271994 , @zzag wrote:
  
  > In D13215#271983 , @broulik 
wrote:
  >
  > > +1
  > >
  > > Doesn't make a difference in Oxygen's rounded corners on panels but 
doesn't make it worse either. Please also profile whether it causes any 
needless recalculations on Plasma startup
  >
  >
  > Maybe, because of
  >
  >   if (!d->repaintBlocked) {
  >  d->updateFrameData();
  >   }
  >
  >
  > FrameSVG misses to update margins, and so on.
  
  
  OK, I'm wrong about this part.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag added a comment.


  I think I got it.
  
  FrameSVGPrivate::updateFrameData updates only `frame`, `maskFrame` is updated 
lazily in `alphaMask` method, but the alphaMask method ignores previous state 
of enabledBorders. So, we end up with using stale maskFrame.
  
  E.g.
  
FrameSVGPrivate::updateFrameData() {
if (!fd) {
...
old key = "10_216_768_1_1__dialogs/background"
old borders = 
QFlags(BottomBorder|RightBorder)
new key = "8_216_768_1_1__dialogs/background"
new borders = QFlags(RightBorder)
  
  Now, when we call alphaMask,
  
FrameSVGPrivate::alphaMask() {
if (maskPrefix.isNull()) {
} else {
 ...
 oldKey = "10_216_768_1_1_mask-_dialogs/background"

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag updated this revision to Diff 35336.
zzag added a comment.


  Do not ignore previous maskFrame's enabled borders.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13215?vs=35191&id=35336

BRANCH
  fix-framesvg-contentrect

REVISION DETAIL
  https://phabricator.kde.org/D13215

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks
Cc: mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag updated this revision to Diff 35338.
zzag added a comment.


  Code cleanup.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13215?vs=35336&id=35338

BRANCH
  fix-framesvg-contentrect

REVISION DETAIL
  https://phabricator.kde.org/D13215

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks
Cc: mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> framesvg.cpp:483
>  
>  updateSizes(maskFrame);
>  }

Should it be here?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D13215: FrameSVG: Recache maskFrame if enabledBorders has been changed

[Vlad Zagorodniy]

zzag retitled this revision from "Update sizes before generating frame svg 
background" to "FrameSVG: Recache maskFrame if enabledBorders has been changed".
zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: abetts, mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, 
bruns

D13285: FrameSVG: Use QMargins

[Vlad Zagorodniy]

zzag created this revision.
zzag added reviewers: Plasma, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  Use QMargins to store margins and measures. As a side effect of this
  diff, `updateSizes` and `contentGeometry` are more simpler now,
  which is good, I guess.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  framesvg-modernize

REVISION DETAIL
  https://phabricator.kde.org/D13285

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/framesvg.h
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13286: FrameSVG: Fix QObject include

[Vlad Zagorodniy]

zzag created this revision.
zzag added reviewers: Plasma, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  See Frameworks coding style: 
https://techbase.kde.org/Policies/Frameworks_Coding_Style#Qt_Includes_and_KDE_Frameworks_Includes

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  frameworks-coding-style

REVISION DETAIL
  https://phabricator.kde.org/D13286

AFFECTED FILES
  src/plasma/framesvg.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13287: FrameSVG: Delete redundant checks

[Vlad Zagorodniy]

zzag created this revision.
zzag added reviewers: Plasma, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  There is no need to check whether `cachedBackground` is null, and if
  it's null, return a null pixmap.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  framesvg-redundant-checks

REVISION DETAIL
  https://phabricator.kde.org/D13287

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13285: FrameSVG: Use QMargins

[Vlad Zagorodniy]

zzag updated this revision to Diff 35444.
zzag added a comment.


  Forward-declare QMarginsF. (to be consistent with QRectF, QSizeF, etc)

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13285?vs=35412&id=35444

BRANCH
  framesvg-modernize

REVISION DETAIL
  https://phabricator.kde.org/D13285

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/framesvg.h
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13285: FrameSVG: Use QMargins

[Vlad Zagorodniy]

zzag updated this revision to Diff 35453.
zzag added a comment.


  Fix ::margins() and ::fixedMargins()

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13285?vs=35444&id=35453

BRANCH
  framesvg-modernize

REVISION DETAIL
  https://phabricator.kde.org/D13285

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/framesvg.h
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13285: FrameSVG: Use QMargins

[Vlad Zagorodniy]

zzag abandoned this revision.
zzag added a comment.


  The diff is crap. I'll abandon it.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13285

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag added a comment.


  Answer to my inline question: updateSizes can be omitted (but I would prefer 
to leave it). Also, `maskFrame->enabledBorders = frame->enabledBorders;` above 
is redundant (maskFrame->enabledBorders is already equal to 
frame->enabledBorders; it if wasn't the case, then cacheId works incorrectly or 
FrameData constructor doesn't copy enabledBorders, which isn't the case).
  
  Also,
  
if (s_sharedFrames[q->theme()->d].contains(oldKey)) {
s_sharedFrames[q->theme()->d].remove(oldKey);
s_sharedFrames[q->theme()->d].insert(newKey, maskFrame);
}
  
  ignores refcount, which may cause problems. I won't fix that, because that 
would be unrelated change.
  
  Yet another "also":
  
if (maskFrame->cachedBackground.isNull()) {
return QPixmap();
}
  
  is redundant.
  
  Anyway, I think I finished my work on fixing bugs that are listed in the 
summary and would like to proceed with review.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: abetts, mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, 
bruns

D13215: Update sizes before generating frame svg background

[Vlad Zagorodniy]

zzag added a comment.


  In D13215#271983 , @broulik wrote:
  
  > Doesn't make a difference in Oxygen's rounded corners on panels but doesn't 
make it worse either.
  
  
  Could you please share a screenshot? I still don't know what's wrong with 
Oxygen rounded corners.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: abetts, mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, 
bruns

D13285: FrameSVG: Use QMargins

[Vlad Zagorodniy]

zzag added a comment.


  I'm not sure whether that's a good idea to use QMargins for measures, maybe, 
it would be better to have something like
  
struct {
QSize left;
QSize top;
QSize right;
QSize bottom;
} sizes, fixedSizes;
  
  in FrameData, which then should go in a different diff (if you think it's 
worth it).
  
  Also, I think it will simplify generateFrameBackground a little bit, e.g.
  
paintBorder(p, frame, FrameSvg::RightBorder, frame->sizes.right * 
q->devicePixelRatio, contentRect);
  
  so there won't be a need for `const int leftHeight = 
q->elementSize(frame->prefix % QLatin1String("left")).height();`

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13285

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13287: FrameSVG: Delete redundant checks

[Vlad Zagorodniy]

zzag added a comment.


  @apol Do I need to wait for other reviewers (from plasma)? Or I can land it 
right away?

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  framesvg-redundant-checks

REVISION DETAIL
  https://phabricator.kde.org/D13287

To: zzag, #plasma, #frameworks, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13287: FrameSVG: Delete redundant checks

[Vlad Zagorodniy]

zzag added a comment.


  In D13287#273556 , @apol wrote:
  
  > :( am I not plasma enough?
  
  
  Sorry, I still don't know who is who. :D

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  framesvg-redundant-checks

REVISION DETAIL
  https://phabricator.kde.org/D13287

To: zzag, #plasma, #frameworks, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13286: FrameSVG: Fix QObject include

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:a363815a796a: FrameSVG: Fix QObject include (authored by 
zzag).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13286?vs=35413&id=35540

REVISION DETAIL
  https://phabricator.kde.org/D13286

AFFECTED FILES
  src/plasma/framesvg.h

To: zzag, #plasma, #frameworks, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13287: FrameSVG: Delete redundant checks

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:b169aa50f917: FrameSVG: Delete redundant checks (authored 
by zzag).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13287?vs=35414&id=35541

REVISION DETAIL
  https://phabricator.kde.org/D13287

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13215: FrameSVG: Recache maskFrame if enabledBorders has been changed

[Vlad Zagorodniy]

zzag updated this revision to Diff 35542.
zzag added a comment.


  Rebase.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13215?vs=35338&id=35542

BRANCH
  fix-framesvg-alphamask

REVISION DETAIL
  https://phabricator.kde.org/D13215

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks
Cc: abetts, mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, 
bruns

D13218: FrameSvg: Draw corners only if both borders in both directions are enabled

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13218

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13218: FrameSvg: Draw corners only if both borders in both directions are enabled

[Vlad Zagorodniy]

zzag retitled this revision from "Draw FrameSVG corners only if both borders in 
both directions are enabled" to "FrameSvg: Draw corners only if both borders in 
both directions are enabled".

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13218

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13215: FrameSvg: Recache maskFrame if enabledBorders has been changed

[Vlad Zagorodniy]

zzag retitled this revision from "FrameSVG: Recache maskFrame if enabledBorders 
has been changed" to "FrameSvg: Recache maskFrame if enabledBorders has been 
changed".

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: abetts, mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, 
bruns

D13215: FrameSvg: Recache maskFrame if enabledBorders has been changed

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks
Cc: abetts, mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, 
bruns

D13034: Add mechanism to notify other clients of config changes over DBus

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> kconfigwatcher.cpp:61
> +
> +for(const QString &path: watchedPaths) {
> +QDBusConnection::sessionBus().connect(QString(),

I think you forgot qAsConst. ;-)

  for (const QString &path : qAsConst(watchedPaths)) {

REPOSITORY
  R237 KConfig

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D13034

To: davidedmundson, broulik
Cc: broulik, zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D13218: FrameSvg: Draw corners only if both borders in both directions are enabled

[Vlad Zagorodniy]

zzag added a comment.


  In D13218#274785 , @mart wrote:
  
  > how does this relates with D13215 ?
  
  
  If maskFrame isn't recached after enabledBorders has been changed, I would 
expect the "After" picture rather the "Before" picture because the bottom part 
isn't enabled. (sorry for broken English)

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix-paintcorner

REVISION DETAIL
  https://phabricator.kde.org/D13218

To: zzag, #plasma, #frameworks, mart
Cc: mart, kde-frameworks-devel, ragreen, Pitel, michaelh, ZrenBot, ngraham, 
bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D13215: FrameSvg: Recache maskFrame if enabledBorders has been changed

[Vlad Zagorodniy]

zzag added a comment.


  In D13215#274782 , @mart wrote:
  
  > it has nothing to do with compositing, but it *should* fix the non 
composite mask never the less, as it was coming from a misrendering  of the 
framesvg
  
  
  Could someone please show me what's wrong with Oxygen corners? (I still don't 
know what's wrong with them, sorry)

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  fix-framesvg-alphamask

REVISION DETAIL
  https://phabricator.kde.org/D13215

To: zzag, #plasma, #frameworks, mart
Cc: abetts, mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, 
bruns

D13218: FrameSvg: Draw corners only if both borders in both directions are enabled

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:a46cd14553c2: FrameSvg: Draw corners only if both borders 
in both directions are enabled (authored by zzag).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13218?vs=35198&id=35697

REVISION DETAIL
  https://phabricator.kde.org/D13218

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks, mart
Cc: mart, kde-frameworks-devel, ragreen, Pitel, michaelh, ZrenBot, ngraham, 
bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D13215: FrameSvg: Recache maskFrame if enabledBorders has been changed

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:dce258bee3da: FrameSvg: Recache maskFrame if 
enabledBorders has been changed (authored by zzag).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13215?vs=35542&id=35698

REVISION DETAIL
  https://phabricator.kde.org/D13215

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks, mart
Cc: abetts, mart, aseigo, broulik, kde-frameworks-devel, michaelh, ngraham, 
bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag created this revision.
zzag added reviewers: Plasma, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  It might happen that the maskFrame is shared by several instances of
  FrameSvg. In that case, do not wreck maskFrame and instead act as
  follows:
  
  - try to lookup shared frames
  - if there is matching frame, use it
  - otherwise create a new one

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  dont-wreck-shared-mask-frames

REVISION DETAIL
  https://phabricator.kde.org/D13384

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag added a comment.


  Sorry for the nasty diff. Caching is hard. :(

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag updated this revision to Diff 35704.
zzag added a comment.


  Do not invalidate looked up frames.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13384?vs=35699&id=35704

BRANCH
  dont-wreck-shared-mask-frames

REVISION DETAIL
  https://phabricator.kde.org/D13384

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag updated this revision to Diff 35705.
zzag added a comment.


  Move `maskFrame->cachedBackground = QPixmap();` to the maskFrame->refcount() 
== 1
  branch. Do we really need this assignment?

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13384?vs=35704&id=35705

BRANCH
  dont-wreck-shared-mask-frames

REVISION DETAIL
  https://phabricator.kde.org/D13384

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag added a comment.


  Corner case: what if only need to update cachedBackground(i.e. enabledBorders 
and frameSize are okay)?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag added a comment.


  In D13384#275224 , @zzag wrote:
  
  > Corner case: what if only need to update cachedBackground(i.e. 
enabledBorders and frameSize are okay)?
  
  
  Delete `shouldUpdate` and wrap if (refcount() == 1) {} into another if 
statement, e.g.
  
if (maskFrame->enabledBorders != frame->enabledBorders || 
maskFrame->frameSize != frameSize(frame)) {
if (maskFrame->refcount() == 1) {
// ...
} else {
// ...
}
updateSizes(maskFrame);
}

if (maskFrame->cachedBackground.isNull()) {
generateBackground(maskFrame);
}

return maskFrame->cachedBackground;
  
  ?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13402: FrameSvg: Simplify updateSizes

[Vlad Zagorodniy]

zzag created this revision.
zzag added reviewers: Plasma, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
if (hintXXXMargin > -1) {
// ...
} else {
// ...
}
  
  are redundant because fixedXXXMargin already hold correct margin values.

TEST PLAN
  Manually

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  framesvg-simplify-updatesizes

REVISION DETAIL
  https://phabricator.kde.org/D13402

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag updated this revision to Diff 35749.
zzag added a comment.


  Update sizes only if enabled borders or frame size have been changed

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13384?vs=35705&id=35749

BRANCH
  dont-wreck-shared-mask-frames

REVISION DETAIL
  https://phabricator.kde.org/D13384

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag planned changes to this revision.
zzag added a comment.


  Add comments describing CFG.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag updated this revision to Diff 35806.
zzag added a comment.


  Rename `shouldUpdateSizes` back to `shouldUpdate`
  
  That's for a possible future diff: the mask frame should be updated
  if maskFrame->imagePath != frame->imagePath. That's an unrelated
  change because this diff tries to address the problem of corrupting
  already shared mask frames.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13384?vs=35749&id=35806

BRANCH
  dont-wreck-shared-mask-frames

REVISION DETAIL
  https://phabricator.kde.org/D13384

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag added a comment.


  In D13384#275246 , @zzag wrote:
  
  > Add comments describing CFG.
  
  
  As it turns out, that's not necessary. The code is pretty much 
"self-explanatory".
  
  Anyway, I'd like to proceed with code review.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag added a dependent revision: D13496: FrameSvg: Update mask frame if image 
path has been changed.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13496: FrameSvg: Update mask frame if image path has been changed

[Vlad Zagorodniy]

zzag created this revision.
zzag added reviewers: Plasma, Frameworks.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  A FrameSvg can have wrong mask frame if image path has been changed after
  the mask frame was generated. Take image path into account when deciding
  whether the mask frame should be updated to address that problem.
  
  Depends on D13384 

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  dont-wreck-shared-mask-frames

REVISION DETAIL
  https://phabricator.kde.org/D13496

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag updated this revision to Diff 36056.
zzag added a comment.


  Use `frame`'s imagePath.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13384?vs=35806&id=36056

REVISION DETAIL
  https://phabricator.kde.org/D13384

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13496: FrameSvg: Update mask frame if image path has been changed

[Vlad Zagorodniy]

zzag updated this revision to Diff 36057.
zzag added a comment.


  Compare against frame->imagePath.

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13496?vs=36053&id=36057

BRANCH
  dont-wreck-shared-mask-frames

REVISION DETAIL
  https://phabricator.kde.org/D13496

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13500: Honor BUILD_TESTING

[Vlad Zagorodniy]

zzag created this revision.
zzag added a reviewer: KWin.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  Don't build tests if BUILD_TESTING is OFF.

TEST PLAN
  - Passed BUILD_TESTING=OFF to cmake, no tests have been built;
  - Didn't pass BUILD_TESTING to cmake, all tests have been built.

REPOSITORY
  R127 KWayland

BRANCH
  honor-build-testing

REVISION DETAIL
  https://phabricator.kde.org/D13500

AFFECTED FILES
  CMakeLists.txt

To: zzag, #kwin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13501: Fix 'unused parameter' warnings

[Vlad Zagorodniy]

zzag created this revision.
zzag added a reviewer: KWin.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  The warnings:
  

/home/vlad/KDE/src/frameworks/kwayland/src/server/xdgforeign_v2_interface.cpp: 
In static member function ‘static void

KWayland::Server::XdgExporterUnstableV2Interface::Private::destroyCallback(wl_client*,
 wl_resource*)’:

/home/vlad/KDE/src/frameworks/kwayland/src/server/xdgforeign_v2_interface.cpp:95:95:
 warning: unused parameter ‘resource’
[-Wunused-parameter]
 void XdgExporterUnstableV2Interface::Private::destroyCallback(wl_client 
*client, wl_resource *resource)

  ~^~~~

/home/vlad/KDE/src/frameworks/kwayland/src/server/xdgforeign_v2_interface.cpp: 
In static member function ‘static void

KWayland::Server::XdgImporterUnstableV2Interface::Private::destroyCallback(wl_client*,
 wl_resource*)’:

/home/vlad/KDE/src/frameworks/kwayland/src/server/xdgforeign_v2_interface.cpp:237:95:
 warning: unused parameter ‘resource’
[-Wunused-parameter]
 void XdgImporterUnstableV2Interface::Private::destroyCallback(wl_client 
*client, wl_resource *resource)

  ~^~~~

/home/vlad/KDE/src/frameworks/kwayland/src/server/xdgforeign_v2_interface.cpp: 
In static member function ‘static void

KWayland::Server::XdgImportedUnstableV2Interface::Private::setParentOfCallback(wl_client*,
 wl_resource*, wl_resource*)’:

/home/vlad/KDE/src/frameworks/kwayland/src/server/xdgforeign_v2_interface.cpp:439:78:
 warning: unused parameter ‘client’
[-Wunused-parameter]
 void 
XdgImportedUnstableV2Interface::Private::setParentOfCallback(wl_client *client, 
wl_resource *resource, wl_resource *
surface)
   
~~~^~

/home/vlad/KDE/src/frameworks/kwayland/src/server/xdgoutput_interface.cpp: 
In constructor

‘KWayland::Server::XdgOutputManagerInterface::XdgOutputManagerInterface(KWayland::Server::Display*,
 QObject*)’:

/home/vlad/KDE/src/frameworks/kwayland/src/server/xdgoutput_interface.cpp:88:81:
 warning: unused parameter ‘parent’
[-Wunused-parameter]
 XdgOutputManagerInterface::XdgOutputManagerInterface(Display *display, 
QObject *parent)

~^~

TEST PLAN
  No more 'unused parameter' warnings.

REPOSITORY
  R127 KWayland

BRANCH
  fix-warnings

REVISION DETAIL
  https://phabricator.kde.org/D13501

AFFECTED FILES
  src/server/xdgforeign_v2_interface.cpp
  src/server/xdgoutput_interface.cpp

To: zzag, #kwin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13501: Fix 'unused parameter' warnings

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:1028770d48e5: Fix 'unused parameter' warnings 
(authored by zzag).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13501?vs=36069&id=36075

REVISION DETAIL
  https://phabricator.kde.org/D13501

AFFECTED FILES
  src/server/xdgforeign_v2_interface.cpp
  src/server/xdgoutput_interface.cpp

To: zzag, #kwin, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13510: Add XDG WM Base support to our XDGShell API

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> test_xdg_shell.cpp:120
> +break;
> +}
>  

Missing `default`. Is it okay if version is unknown?

> xdgshell_stable.cpp:28
> +
> +#include 
> +

Seems like QDebug is not used here.

> xdgshell_stable.cpp:307
> +Q_UNUSED(surface)
> +auto s = reinterpret_cast(data);
> +s->q->configureRequested(s->pendingSize, s->pendingState, serial);

Use static_cast. Same down below.

> xdgshell_stable.cpp:336
> +states = states | XdgShellSurface::State::Activated;
> +break;
> +}

Missing `default`. Is it okay if the state is unknown?

> xdgshell_stable.cpp:525
> +
> +const struct xdg_popup_listener 
> XdgShellPopupStable::Private::s_popupListener = {
> +configureCallback,

Shouldn't it be also static?

> xdgshell_stable.cpp:536
> +{
> +Q_UNUSED(xdg_popup);
> +auto s = reinterpret_cast(data);

Nitpick: no semicolons after Q_UNUSED.

> xdgshell_stable_interface.cpp:75
> +Private(XdgPopupStableInterface *q, XdgShellStableInterface *c, 
> SurfaceInterface *surface, wl_resource *parentResource);
> +~Private();
> +

I would also add override, e.g.

  ~Private() override;

> xdgshell_stable_interface.cpp:242-243
> +[surface](XdgSurfaceStableInterface *s) {
> +return false;
> +return surface == s->surface();
> +}

Which one should return?

> xdgshell_stable_interface.cpp:488-493
> +if (parentXdgSurface) {
> +pd->parent = parentXdgSurface->surface();
> +} else {
> +wl_resource_post_error(parentResource, 
> XDG_WM_BASE_ERROR_INVALID_POPUP_PARENT, "Invalid popup parent");
> +return;
> +}

Can be simplified, e.g.

  if (!parentXdgSurface) {
  wl_resource_post_error(parentResource, 
XDG_WM_BASE_ERROR_INVALID_POPUP_PARENT, "Invalid popup parent");
  return;
  }
  
  pd->parent = parentXdgSurface->surface();
  pd->

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D13510

To: davidedmundson, #kwin
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D13510: Add XDG WM Base support to our XDGShell API

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> zzag wrote in xdgshell_stable.cpp:525
> Shouldn't it be also static?

Ignore it.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D13510

To: davidedmundson, #kwin
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D13502: Fix compiler warning about hidden virtual overloads

[Vlad Zagorodniy]

zzag added a comment.


  > Compiles with no warnings \o/
  
  Nope.
  
In file included from 
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_v6.cpp:20:
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:49:13: 
warning: ‘virtual KWayland::Client::XdgShell::Private::operator xdg_shell*() 
const’ was hidden [-Woverloaded-virtual]
 virtual operator xdg_shell*() const {
 ^~~~
: warning:   by ‘operator’ [-Woverloaded-virtual]
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:46:13: 
warning: ‘virtual KWayland::Client::XdgShell::Private::operator xdg_shell*()’ 
was hidden [-Woverloaded-virtual]
 virtual operator xdg_shell*() {
 ^~~~
: warning:   by ‘operator’ [-Woverloaded-virtual]
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:60:28: 
warning: ‘virtual KWayland::Client::XdgShellPopup* 
KWayland::Client::XdgShell::Private::getXdgPopup(KWayland::Client::Surface*, 
KWayland::Client::Surface*, KWayland::Client::Seat*, quint32, const QPoint&, 
QObject*)’ was hidden [-Woverloaded-virtual]
 virtual XdgShellPopup *getXdgPopup(Surface *surface, Surface 
*parentSurface, Seat *seat, quint32 serial, const QPoint &parentPos, QObject 
*parent) {
^~~
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_v6.cpp:45:20: 
warning:   by ‘KWayland::Client::XdgShellUnstableV6::Private::getXdgPopup’ 
[-Woverloaded-virtual]
 XdgShellPopup *getXdgPopup(Surface *surface, XdgShellPopup 
*parentSurface, const XdgPositioner &positioner, QObject *parent) override;
^~~
In file included from 
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_v6.cpp:20:
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:163:13: 
warning: ‘virtual KWayland::Client::XdgShellSurface::Private::operator 
xdg_surface*() const’ was hidden [-Woverloaded-virtual]
 virtual operator xdg_surface*() const {
 ^~~~
: warning:   by ‘operator’ [-Woverloaded-virtual]
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:160:13: 
warning: ‘virtual KWayland::Client::XdgShellSurface::Private::operator 
xdg_surface*()’ was hidden [-Woverloaded-virtual]
 virtual operator xdg_surface*() {
 ^~~~
: warning:   by ‘operator’ [-Woverloaded-virtual]
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:225:13: 
warning: ‘virtual KWayland::Client::XdgShellPopup::Private::operator 
xdg_popup*() const’ was hidden [-Woverloaded-virtual]
 virtual operator xdg_popup*() const {
 ^~~~
: warning:   by ‘operator’ [-Woverloaded-virtual]
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:222:13: 
warning: ‘virtual KWayland::Client::XdgShellPopup::Private::operator 
xdg_popup*()’ was hidden [-Woverloaded-virtual]
 virtual operator xdg_popup*() {
 ^~~~
: warning:   by ‘operator’ [-Woverloaded-virtual]

In file included from 
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_v5.cpp:20:
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:55:13: 
warning: ‘virtual KWayland::Client::XdgShell::Private::operator 
zxdg_shell_v6*() const’ was hidden [-Woverloaded-virtual]
 virtual operator zxdg_shell_v6*() const {
 ^~~~
: warning:   by ‘operator’ [-Woverloaded-virtual]
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:52:13: 
warning: ‘virtual KWayland::Client::XdgShell::Private::operator 
zxdg_shell_v6*()’ was hidden [-Woverloaded-virtual]
 virtual operator zxdg_shell_v6*() {
 ^~~~
: warning:   by ‘operator’ [-Woverloaded-virtual]
In file included from 
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_v5.cpp:20:
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_p.h:80:28: 
warning: ‘virtual KWayland::Client::XdgShellPopup* 
KWayland::Client::XdgShell::Private::getXdgPopup(KWayland::Client::Surface*, 
KWayland::Client::XdgShellPopup*, const KWayland::Client::XdgPositioner&, 
QObject*)’ was hidden [-Woverloaded-virtual]
 virtual XdgShellPopup *getXdgPopup(Surface *surface, XdgShellPopup 
*parentSurface, const XdgPositioner &positioner, QObject *parent) {
^~~
/home/vlad/KDE/src/frameworks/kwayland/src/client/xdgshell_v5.cpp:41:20: 
warning:   by ‘virtual KWayland::Client::XdgShellPopup* 
KWayland::Client::XdgShellUnstableV5::Private::getXdgPopup(KWayland::Client::Surface*,
 KWayland::Client::Surface*, KWayland::Client::Seat*, quint32, const QPoint&, 
QObject*)’ [-Woverloaded-virtual]
 XdgShellPopup *getXdgPopup(Surface *surface, Surface *parentSurface, 
Seat *seat, quint32 serial, const QPoint &parentPos, QObject *parent) override;
^~~
In file included from 
/

D13502: Fix compiler warning about hidden virtual overloads

[Vlad Zagorodniy]

zzag added a comment.


  In D13502#277828 , @davidedmundson 
wrote:
  
  > I meant the class I changed no longer had warnings.
  
  
  Oh, I thought you were talking about all -Woverloaded-virtual warnings. I 
think it would be better to fix those warnings in one go.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D13502

To: davidedmundson, #kwin
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D13502: Fix compiler warning about hidden virtual overloads

[Vlad Zagorodniy]

zzag accepted this revision.
zzag added a comment.
This revision is now accepted and ready to land.


  In D13502#277837 , @davidedmundson 
wrote:
  
  > ...
  >  But if this is accepted as a good solution for the problem, ...
  
  
  Well, yeah, that's pretty good solution to name hiding. Another solution 
would be to implement hidden overloads.

REPOSITORY
  R127 KWayland

BRANCH
  xdg_squash

REVISION DETAIL
  https://phabricator.kde.org/D13502

To: davidedmundson, #kwin, zzag
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag added a comment.


  Ping?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13402: FrameSvg: Simplify updateSizes

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13402

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13510: Add XDG WM Base support to our XDGShell API

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> xdgshell_stable.cpp:305
> +Q_UNUSED(surface)
> +auto s = reinterpret_cast(data);
> +s->q->configureRequested(s->pendingSize, s->pendingState, serial);

Did you forget to change it to static_cast? Or there is a reason why it's still 
reinterpret_cast?

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D13510

To: davidedmundson, #kwin
Cc: romangg, zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D13500: Honor BUILD_TESTING

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D13500

To: zzag, #kwin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13500: Honor BUILD_TESTING

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:2d06b24a86f2: Honor BUILD_TESTING (authored by zzag).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13500?vs=36068&id=36307

REVISION DETAIL
  https://phabricator.kde.org/D13500

AFFECTED FILES
  CMakeLists.txt

To: zzag, #kwin, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13601: OutputManagement fractional scaling

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> outputconfiguration_interface.cpp:194
> +Q_UNUSED(client);
> +qreal scale = wl_fixed_to_double(scale_fixed);
> +

Nitpick: can be const. :-)

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D13601

To: davidedmundson, #kwin
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D13402: FrameSvg: Simplify updateSizes

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13402

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13496: FrameSvg: Update mask frame if image path has been changed

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13496

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13601: OutputManagement fractional scaling

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> outputdevice.cpp:413
>  
> +
>  bool OutputDevice::isValid() const

Extra new line.

> outputconfiguration_interface.cpp:73
> +static void scaleFCallback(wl_client *client, wl_resource *resource,
> +  wl_resource * outputdevice, wl_fixed_t scale);
>  

wl_resource is one space off.

> outputdevice_interface.cpp:491
>  
> +
>  OutputDeviceInterface::SubPixel OutputDeviceInterface::subPixel() const

Extra new line.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D13601

To: davidedmundson, #kwin
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D13402: FrameSvg: Simplify updateSizes

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13402

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13496: FrameSvg: Update mask frame if image path has been changed

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13496

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

zzag added a comment.


  Ping.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D13384

To: zzag, #plasma, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13402: FrameSvg: Simplify updateSizes

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:acdaefa221cc: FrameSvg: Simplify updateSizes (authored by 
zzag).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13402?vs=35748&id=36757

REVISION DETAIL
  https://phabricator.kde.org/D13402

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks, mart
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13384: FrameSvg: Do not wreck shared mask frames

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:412a517576d5: FrameSvg: Do not wreck shared mask frames 
(authored by zzag).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13384?vs=36056&id=36758

REVISION DETAIL
  https://phabricator.kde.org/D13384

AFFECTED FILES
  src/plasma/framesvg.cpp
  src/plasma/private/framesvg_p.h

To: zzag, #plasma, #frameworks, mart
Cc: mart, kde-frameworks-devel, michaelh, ngraham, bruns

D13496: FrameSvg: Update mask frame if image path has been changed

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:0c8dd9dc2b54: FrameSvg: Update mask frame if image path 
has been changed (authored by zzag).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13496?vs=36057&id=36759

REVISION DETAIL
  https://phabricator.kde.org/D13496

AFFECTED FILES
  src/plasma/framesvg.cpp

To: zzag, #plasma, #frameworks, mart
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14009: [Plasma PluginLoader] Cache plugins during startup

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> pluginloader.cpp:194-196
> +const QString pluginName = name.section(QLatin1Char('/'), -1);
>  
> +QVector plugins;

Nitpick: IMHO, it would be better to move these two down (right before `if 
(useRuntimeCache) {`). So, cache invalidation and cache population stuff goes 
first, then querying stuff.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D14009

To: broulik, #plasma, davidedmundson, mart
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns

D14520: Open `skipPager` functionality

[Vlad Zagorodniy]

zzag added a comment.


  Please checkout .arclint from master (git checkout master -- .arclint).

INLINE COMMENTS

> plasmashell_interface.h:192-194
> +
> +
> +

Unrelated change.

REPOSITORY
  R127 KWayland

REVISION DETAIL
  https://phabricator.kde.org/D14520

To: sharvey, #plasma_on_wayland, hein, #plasma, romangg, davidedmundson
Cc: zzag, kde-frameworks-devel, ngraham, michaelh, bruns

D14946: [DialogShadows] Use 0 offset for disabled borders on Wayland

[Vlad Zagorodniy]

zzag created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  On Wayland, DialogShadows doesn't attach empty tiles for disabled
  borders. Yet, it sets offset of 1 for them. This results in visual
  artifacts like this
  
  F6206088: before.png 
  
  //KWin thinks that there is 1px tall shadow in the bottom.//
  
  In general we don't need to pass empty shadow tiles for disabled
  borders. Thus, that's fine to set 0 offset for them.

TEST PLAN
  - Started kwin_wayland:
  
dbus-launch kwin_wayland --windowed --xwayland
  
  - Launched latte-dock and kate:
  
unset DISPLAY QT_QPA_PLATFORM=wayland latte-dock & QT_QPA_PLATFORM=wayland 
kate
  
  - Opened calendar popup:
  
  F6206090: after.png 

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  wayland-shadows

REVISION DETAIL
  https://phabricator.kde.org/D14946

AFFECTED FILES
  src/plasmaquick/dialogshadows.cpp

To: zzag
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14946: [DialogShadows] Use 0 offset for disabled borders on Wayland

[Vlad Zagorodniy]

zzag updated this revision to Diff 40150.
zzag added a comment.


  "QMarginsF margins = QMarginsF();" => "QMarginsF margins;"

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14946?vs=40044&id=40150

BRANCH
  wayland-shadows

REVISION DETAIL
  https://phabricator.kde.org/D14946

AFFECTED FILES
  src/plasmaquick/dialogshadows.cpp

To: zzag, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14946: [DialogShadows] Use 0 offset for disabled borders on Wayland

[Vlad Zagorodniy]

zzag added a comment.


  This change depends on D14783 .
  
  I also think that we don't need to attach empty tiles on X11. But it works, 
the Sliding Popups effect was fixed... So, maybe, that's fine.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  wayland-shadows

REVISION DETAIL
  https://phabricator.kde.org/D14946

To: zzag, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14946: [DialogShadows] Use 0 offset for disabled borders on Wayland

[Vlad Zagorodniy]

zzag added a comment.


  I suppose it has to go in 5.51 (tagging will be tomorrow).

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  wayland-shadows

REVISION DETAIL
  https://phabricator.kde.org/D14946

To: zzag, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14946: [DialogShadows] Use 0 offset for disabled borders on Wayland

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:8497832a8bf0: [DialogShadows] Use 0 offset for disabled 
borders on Wayland (authored by zzag).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14946?vs=40150&id=40929

REVISION DETAIL
  https://phabricator.kde.org/D14946

AFFECTED FILES
  src/plasmaquick/dialogshadows.cpp

To: zzag, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15330: [autotests] Compare booleans to booleans

[Vlad Zagorodniy]

zzag created this revision.
zzag added reviewers: KWin, romangg.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  Compiler errors:
  
  17:06:23 
/usr/lib64/gcc/x86_64-suse-linux/8/../../../../x86_64-suse-linux/bin/ld: 
CMakeFiles/testDataDevice.dir/test_datadevice.cpp.o: in function 
`TestDataDevice::testDrag()':
  17:06:23 /home/jenkins/workspace/Frameworks kwayland kf5-qt5 
SUSEQt5.9/autotests/client/test_datadevice.cpp:290: undefined reference to 
`bool QTest::qCompare(int const&, bool const&, char const*, char 
const*, char const*, int)'
  17:06:23 
/usr/lib64/gcc/x86_64-suse-linux/8/../../../../x86_64-suse-linux/bin/ld: 
CMakeFiles/testDataDevice.dir/test_datadevice.cpp.o: in function 
`TestDataDevice::testDragInternally()':
  17:06:23 /home/jenkins/workspace/Frameworks kwayland kf5-qt5 
SUSEQt5.9/autotests/client/test_datadevice.cpp:369: undefined reference to 
`bool QTest::qCompare(int const&, bool const&, char const*, char 
const*, char const*, int)'

TEST PLAN
  Ran testDataDevice, still passes.

REPOSITORY
  R127 KWayland

BRANCH
  fix-build

REVISION DETAIL
  https://phabricator.kde.org/D15330

AFFECTED FILES
  autotests/client/test_datadevice.cpp

To: zzag, #kwin, romangg
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15330: [autotests] Compare booleans to booleans

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:3c128a612c1f: [autotests] Compare booleans to booleans 
(authored by zzag).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15330?vs=41149&id=41152

REVISION DETAIL
  https://phabricator.kde.org/D15330

AFFECTED FILES
  autotests/client/test_datadevice.cpp

To: zzag, #kwin, romangg, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns