D23839: Port away from Qt's foreach

[Vlad Zagorodniy]

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


  I tend to leave only one comment about troubling issue/problem and expect 
that the author of a patch will address all other occurrences of the 
issue/problem.
  
  However, let's get this change in. Perhaps I'm too picky about const refs.

REPOSITORY
  R278 KWindowSystem

BRANCH
  portfromforeach

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

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23839: Port away from Qt's foreach

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> zzag wrote in kwindowsystem_threadtest.cpp:61
> I asked that because `for (const Type  : collection) {` is more common. 
> I know that const ref doesn't have any advantages here.

In either case, this change is good to go. However, it would be great to 
capture values by const ref.

REPOSITORY
  R278 KWindowSystem

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

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23839: Port away from Qt's foreach

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> kossebau wrote in kwindowsystem_threadtest.cpp:61
> Would a const ref make sense here, given the type nature of WId which boils 
> down to a integer matching the byte size of a pointer?

I asked that because `for (const Type  : collection) {` is more common. I 
know that const ref doesn't have any advantages here.

REPOSITORY
  R278 KWindowSystem

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

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23839: Port away from Qt's foreach

[Vlad Zagorodniy]

zzag added a comment.


  Did you use a script to create this patch?

INLINE COMMENTS

> kwindowsystem_threadtest.cpp:61
>  const QList windows = KWindowSystem::stackingOrder();
> -foreach (auto wid, windows) {
> +for (auto wid : windows) {
>  KWindowInfo info(wid, NET::WMVisibleName);

Capture the value by const ref please.

REPOSITORY
  R278 KWindowSystem

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

To: kossebau, #kwin, zzag
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23835: Add TabKCM

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> TabKCM.qml:153-160
> +var theTabs = []
> +var i = 0
> +
> +for( i=0; i < tabs.length; i++) {
> +tabs[i].delegate.createObject(sl, {})
> +theTabs.push(tabs[i].title)
> +}

FYI, QML has some features from ECMAScript 6, so you could make this code a bit 
cleaner, e.g.

  for (const tab of tabs)
  tab.delegate.createObject(sl, {});
  
  tabsRepeater.model = tabs.map(tab => tab.title);

(I didn't test whether this code actually works)

REPOSITORY
  R296 KDeclarative

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

To: nicolasfella, mart
Cc: zzag, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23745: [server] Make double-buffered properties in xdg-shell double-buffered

[Vlad Zagorodniy]

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


  Handle value of zero in set_max_size callback.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23745?vs=65472=65603

BRANCH
  surface-role

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

AFFECTED FILES
  autotests/client/test_xdg_shell_stable.cpp
  autotests/client/test_xdg_shell_v6.cpp
  src/server/CMakeLists.txt
  src/server/generic_shell_surface_p.h
  src/server/shell_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h
  src/server/surface_interface_p.h
  src/server/surfacerole.cpp
  src/server/surfacerole_p.h
  src/server/xdgshell_interface.cpp
  src/server/xdgshell_interface.h
  src/server/xdgshell_interface_p.h
  src/server/xdgshell_stable_interface.cpp
  src/server/xdgshell_v5_interface.cpp
  src/server/xdgshell_v6_interface.cpp

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

D23745: [server] Make double-buffered properties in xdg-shell double-buffered

[Vlad Zagorodniy]

zzag added a comment.


  In D23745#527309 , @davidedmundson 
wrote:
  
  > Only question is whether it'd more useful to have an api which helps track 
that we only have one role, or have an api where /all/ the objects attached to 
a surface (blur, plasma shell, etc.) can easily double buffer.
  
  
  Not sure about the latter, but we definitely need to track attached surface 
role.

REPOSITORY
  R127 KWayland

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

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

D23772: Make less use of deprecated enum alias NET::StaysOnTop

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> netwininfotestwm.cpp:449
>  QTest::newRow("keepAbove")<< NET::States(NET::KeepAbove)
> << (QVector() << keepAbove << staysOnTop);
> -QTest::newRow("staysOnTop")   << NET::States(NET::StaysOnTop)   
> << (QVector() << keepAbove << staysOnTop);
> +QTest::newRow("staysOnTop")   << NET::States(NET::KeepAbove)
> << (QVector() << keepAbove << staysOnTop);
>  QTest::newRow("skipPager")<< NET::States(NET::SkipPager)
> << (QVector() << skipPager);

We can remove this row now.

REPOSITORY
  R278 KWindowSystem

BRANCH
  lessuseofstaysOnTop

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

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

D23772: Make less use of deprecated enum alias NET::StaysOnTop

[Vlad Zagorodniy]

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

REPOSITORY
  R278 KWindowSystem

BRANCH
  lessuseofstaysOnTop

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

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

D23418: KStartupInfo: drop dead code for removing quotes from data values

[Vlad Zagorodniy]

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

REPOSITORY
  R278 KWindowSystem

BRANCH
  dropdeadcodeforremovingquotesfromstartupdatavalue

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

To: kossebau, #plasma, hein, broulik, #kwin, zzag
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23772: Make less use of deprecated enum alias NET::StaysOnTop

[Vlad Zagorodniy]

zzag edited reviewers, added: KWin; removed: Frameworks.

REPOSITORY
  R278 KWindowSystem

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

To: kossebau, #kwin, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23745: [server] Make double-buffered properties in xdg-shell double-buffered

[Vlad Zagorodniy]

zzag added a task: T11480: Make xdg-shell properties double-buffered.

REPOSITORY
  R127 KWayland

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

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

D23745: [server] Make double-buffered properties in xdg-shell double-buffered

[Vlad Zagorodniy]

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

REVISION SUMMARY
  So far all double-buffered properties in our implementation of xdg-shell
  weren't actually double-buffered. When a property setter is invoked, we
  pray to the God hoping that the client committed associated surface.
  
  This change introduces private SurfaceRole class. The new class provides
  a way for SurfaceInterface to commit pending state of associated shell
  surface.
  
  The chosen architecture allows us to do more in the future. For example,
  we could use SurfaceRole to prevent associating several roles to a single
  wl_surface object, e.g. xdg-toplevel to a pointer surface, etc.

TEST PLAN
  This change breaks support for client-side decorated clients in KWin.

REPOSITORY
  R127 KWayland

BRANCH
  surface-role

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

AFFECTED FILES
  autotests/client/test_xdg_shell_stable.cpp
  autotests/client/test_xdg_shell_v6.cpp
  src/server/CMakeLists.txt
  src/server/generic_shell_surface_p.h
  src/server/shell_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h
  src/server/surface_interface_p.h
  src/server/surfacerole.cpp
  src/server/surfacerole_p.h
  src/server/xdgshell_interface.cpp
  src/server/xdgshell_interface.h
  src/server/xdgshell_interface_p.h
  src/server/xdgshell_stable_interface.cpp
  src/server/xdgshell_v5_interface.cpp
  src/server/xdgshell_v6_interface.cpp

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

Re: Coding style: white space around asterisks

[Vlad Zagorodniy]

On 8/6/19 5:44 PM, Roman Gilg wrote:

Hi,


Hi,

I am not sure whether you started this discussion in Qt/Development 
mailing list, so I'll just leave my 2 cents here.


Let's start with very obvious thing first: If you align pointers and 
references to right, then you're doing something fishy. [1]



Frameworks C++ coding style for pointer declarations follows the
asterisk to the right approach like Qt coding style recommends.

This is most often a sensible choice but is difficult to read in my
opinion when there is no identifier to the right the asterisk is
associated with, for example for function return types and casts.

Because of that I prefer a style like explained here:
https://www.quora.com/Where-should-I-put-the-asterisk-for-pointers-in-C++-Is-it-int*-ptr-int-*-ptr-or-int-*ptr/answer/David-Vandevoorde

Any arguments for or against doing it this way?


While I kind of see what the point of the proposed style is, I think 
it's just complex. If you write code with this style for a long period 
of time, maybe it'll be natural for you. However, we also value 
contributions from community. The proposed style is very rare as most 
people align pointers and references either to left or right, so it'll 
be more difficult for them to contribute.


Aestetically, code with this style looks clunky (imho), e.g.

Foo* bar(const QVector *foo1, Bar *foo2);

Another problem is that tools like clang-format can't be used to 
re-format existing code with such style. They can attach * and & either 
to type, or name, or between name and type. It's very crucial because 
you should not expect people to fix coding style by hand.


Cheers,
Vlad

[1] Some people can disagree with that. ;-)

D23619: [server] Introduce cloning and replication of output devices

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> zzag wrote in outputdevice.cpp:391
> That's not safe if the raw string is dynamically allocated, use qstrlen 
> instead.

Correction: that's not reliable way to check whether a C string is empty. You 
assume that all empty strings have the same address, which in some cases cannot 
be true, e.g. if the string is dynamically allocated.

REPOSITORY
  R127 KWayland

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

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

D23619: [server] Introduce cloning and replication of output devices

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> outputdevice.cpp:391
> +Q_UNUSED(output);
> +if (!source_uuid || source_uuid == "") {
> +o->replicationSource.clear();

That's not safe if the raw string is dynamically allocated, use qstrlen instead.

REPOSITORY
  R127 KWayland

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

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

D23347: kdelibs4support: port away from deprecated KWindowSystem API

[Vlad Zagorodniy]

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

REPOSITORY
  R239 KDELibs4Support

BRANCH
  master

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

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

D23108: Add KWindowSystem::setMainWindow(QWindow *) overload

[Vlad Zagorodniy]

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


  Looks good to me.

REPOSITORY
  R278 KWindowSystem

BRANCH
  setmainwindow

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

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

D23108: Add KWindowSystem::setMainWindow(QWindow *) overload

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> kwindowsystem.cpp:458
>  
> +static QWindow* setMainWindowHelper(QWindow *subWindow, WId mainWindowId)
> +{

Coding style: For pointers or references, use a single space before '*' or '&', 
but not after.

https://techbase.kde.org/Policies/Frameworks_Coding_Style#Whitespace

> kwindowsystem.cpp:490
> +{
> +(void) setMainWindowHelper(subWindow, mainWindowId);
>  }

Each time this method is called a new foreign QWindow object will be created. 
Can you please explain why leaking those objects is okay?

REPOSITORY
  R278 KWindowSystem

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

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

D23108: Add KWindowSystem::setMainWindow(QWindow *) overload

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> kwindowsystem.h:233
>   * when the parent is deleted are unaffected and normally use
>   * the QWidget parent.
>   *

> and normally use the QWidget parent.

I think this statement is inaccurate. Documentation needs more love. :-)

REPOSITORY
  R278 KWindowSystem

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

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

D23089: KWindowSystem: remove unused QWidget includes

[Vlad Zagorodniy]

zzag added a comment.


  In D23089#510308 , @dfaure wrote:
  
  > That's for the benefit of the kde-frameworks-devel mailing list where it's 
all a bit mixed up otherwise...
  
  
  Oh, I didn't know that. Thank you for the clarification. :-)

REPOSITORY
  R278 KWindowSystem

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

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

D23088: KWindowSystem: add setNewStartupId(QWindow *...) overload

[Vlad Zagorodniy]

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

REPOSITORY
  R278 KWindowSystem

BRANCH
  2019_setNewStartupId

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

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

D23089: KWindowSystem: remove unused QWidget includes

[Vlad Zagorodniy]

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


  "KWindowSystem:" can be omitted in the subject line.

INLINE COMMENTS

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

coding stykle nitpick: Each section is sorted alphabetically. :-)

REPOSITORY
  R278 KWindowSystem

BRANCH
  2019_remove_widget_includes_my_master

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

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

D22862: [server] Wrap proxyRemoveSurface in smart pointer

[Vlad Zagorodniy]

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

REPOSITORY
  R127 KWayland

BRANCH
  master

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

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

D22755: [platforms/xcb] Use XRES extension to get real window PID

[Vlad Zagorodniy]

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

REPOSITORY
  R278 KWindowSystem

BRANCH
  master

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

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

D22755: [platforms/xcb] Use XRES extension to get real window PID

[Vlad Zagorodniy]

zzag added a comment.


  > Querying the NET_WM_ATOM with the NETWM classes remains unchanged
  
  _NET_WM_PID?

INLINE COMMENTS

> kwindowinfo.cpp:85
> +specs.mask = XCB_RES_CLIENT_ID_MASK_LOCAL_CLIENT_PID;
> +auto cookie = xcb_res_query_client_ids(QX11Info::connection(), 1, 
> );
> +

I don't like `auto` here, but on the other hand we don't have yet any policy on 
whether auto is allowed to be used in this case.

> kwindowinfo.cpp:449
>  {
> +if (m_pid > 0) {
> +return m_pid;

Leave a comment that explains why m_pid is preferred.

> kwindowinfo.cpp:462
> +
> +bool KWindowInfoPrivateX11::haveXres()
> +{

Make it a static function instead.

REPOSITORY
  R278 KWindowSystem

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

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

D22685: Fix failing autotest

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R278:14998613603c: Fix failing autotest (authored by zzag).

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22685?vs=62406=62407

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kwindowinfox11test.cpp

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

D22685: Fix failing autotest

[Vlad Zagorodniy]

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


  Make hostnames in testClientMachine unique.

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22685?vs=62404=62406

BRANCH
  fix-autotest

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kwindowinfox11test.cpp

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

D22685: Fix failing autotest

[Vlad Zagorodniy]

zzag retitled this revision from "Fix failing autotests" to "Fix failing 
autotest".

REPOSITORY
  R278 KWindowSystem

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

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

D22685: Fix failing autotests

[Vlad Zagorodniy]

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


  Check initial value of group leader.

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22685?vs=62399=62404

BRANCH
  fix-autotest

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kwindowinfox11test.cpp

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

D22685: Fix failing autotests

[Vlad Zagorodniy]

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


  Simplify a bit

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22685?vs=62397=62399

BRANCH
  fix-autotest

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kwindowinfox11test.cpp

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

D22685: Fix failing autotests

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R278 KWindowSystem

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

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

D22685: Fix failing autotests

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R278 KWindowSystem

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

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

D22685: Fix failing autotests

[Vlad Zagorodniy]

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


  Require XCB::ICCCM in autotests/CMakeLists.txt

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22685?vs=62396=62397

BRANCH
  fix-autotest

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kwindowinfox11test.cpp

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

D22685: Fix failing autotests

[Vlad Zagorodniy]

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

REVISION SUMMARY
  KWindowInfoX11Testsome fails because some parts of QXcbWindow were changed
  in Qt 5.12.X:
  
  - Since Qt 5.12.1, QXcbWindow sets WM_CLIENT_LEADER property for all clients;
  
  - Since Qt 5.12.0, QXcbWindow sets WM_CLIENT_MACHINE property.

REPOSITORY
  R278 KWindowSystem

BRANCH
  fix-autotest

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

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  autotests/kwindowinfox11test.cpp

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

D21659: add .vscode to .gitignore

[Vlad Zagorodniy]

zzag added a comment.


  Heh, I usually put .vscode and compile_commands.json (for clangd) in 
.git/info/exclude.

REPOSITORY
  R289 KNotifications

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

To: brute4s99, broulik, nicolasfella
Cc: zzag, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22216: Use wayland-protocols

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:40b86f7792eb: Use wayland-protocols (authored by zzag).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D22216?vs=61018=61310#toc

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22216?vs=61018=61310

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

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindWaylandProtocols.cmake
  src/client/CMakeLists.txt
  src/client/protocols/idle-inhibit-unstable-v1.xml
  src/client/protocols/pointer-constraints-unstable-v1.xml
  src/client/protocols/pointer-gestures-unstable-v1.xml
  src/client/protocols/relative-pointer-unstable-v1.xml
  src/client/protocols/xdg-decoration-unstable-v1.xml
  src/client/protocols/xdg-foreign-unstable-v2.xml
  src/client/protocols/xdg-output-unstable-v1.xml
  src/client/protocols/xdg-shell-unstable-v5.xml
  src/client/protocols/xdg-shell-unstable-v6.xml
  src/client/protocols/xdg-shell.xml
  src/server/CMakeLists.txt

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

D22216: Use wayland-protocols

[Vlad Zagorodniy]

zzag retitled this revision from "RFC: Use wayland-protocols" to "Use 
wayland-protocols".

REPOSITORY
  R127 KWayland

BRANCH
  use-wayland-protocols

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

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

D22216: RFC: Use wayland-protocols

[Vlad Zagorodniy]

zzag added a comment.


  Also, it's worth to point out that neither weston, nor Mutter, nor wlroots 
copy protocol specs, they just have wayland-protocols as a dependency.

REPOSITORY
  R127 KWayland

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

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

D22216: RFC: Use wayland-protocols

[Vlad Zagorodniy]

zzag added a comment.


  Generally, I see nothing wrong with waiting until protocol foo becomes part 
of the current wayland-protocols release. If the protocol is really importatnt, 
we can make temporary copy and after several kwayland releases drop it.

REPOSITORY
  R127 KWayland

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

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

D22216: RFC: Use wayland-protocols

[Vlad Zagorodniy]

zzag added a comment.


  > Why was it done differently until now?
  
  I can't give you an answer, but my guess is for historical reasons. 
Initially, kwayland repo had only proprietary protocols, like 
server_decoration, etc. Protocols from wayland-protocols were copied to follow 
the pattern. Also perhaps because some protocols weren't in the current 
wayland-protocols release yet.

REPOSITORY
  R127 KWayland

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

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

D22216: RFC: Use wayland-protocols

[Vlad Zagorodniy]

zzag added a reviewer: Frameworks.

REPOSITORY
  R127 KWayland

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

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

D22216: RFC: Use wayland-protocols

[Vlad Zagorodniy]

zzag added a comment.


  > find_package(WaylandProtocols 1.15)
  
  because xdg-decoration was introduced in 1.15.

REPOSITORY
  R127 KWayland

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

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

D22216: RFC: Use wayland-protocols

[Vlad Zagorodniy]

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

REVISION SUMMARY
  So far we've been copying protocol specifications from wayland-protocols
  repo to kwayland repo. The main issue with copying is that we have to
  keep xml files in sync manually. A better approach would be to use xml
  specs straight from wayland-protocols package.

TEST PLAN
  Compiles.

REPOSITORY
  R127 KWayland

BRANCH
  use-wayland-protocols

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

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindWaylandProtocols.cmake
  src/client/CMakeLists.txt
  src/client/protocols/idle-inhibit-unstable-v1.xml
  src/client/protocols/pointer-constraints-unstable-v1.xml
  src/client/protocols/pointer-gestures-unstable-v1.xml
  src/client/protocols/relative-pointer-unstable-v1.xml
  src/client/protocols/xdg-decoration-unstable-v1.xml
  src/client/protocols/xdg-foreign-unstable-v2.xml
  src/client/protocols/xdg-output-unstable-v1.xml
  src/client/protocols/xdg-shell-unstable-v5.xml
  src/client/protocols/xdg-shell-unstable-v6.xml
  src/client/protocols/xdg-shell.xml
  src/server/CMakeLists.txt

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

D22156: RFC: Add wayland-protocols find module

[Vlad Zagorodniy]

zzag abandoned this revision.
zzag added a comment.


  In D22156#488219 , @cgiboudeaux 
wrote:
  
  > how many users are there?
  
  
  Hmm, at this moment none. KWayland would be the primary user (and perhaps the 
only one). Perhaps ECM is not good place for this find module. Sorry for the 
noise.

REPOSITORY
  R240 Extra CMake Modules

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

To: zzag, #kwin, #frameworks
Cc: cgiboudeaux, apol, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
bencreasy, michaelh, ngraham, bruns

D22156: RFC: Add wayland-protocols find module

[Vlad Zagorodniy]

zzag retitled this revision from "Add wayland-protocols find module" to "RFC: 
Add wayland-protocols find module".

REPOSITORY
  R240 Extra CMake Modules

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

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

D22156: Add wayland-protocols find module

[Vlad Zagorodniy]

zzag created this revision.
zzag added reviewers: KWin, Frameworks.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
zzag requested review of this revision.

REVISION SUMMARY
  This module can be used to retrieve path of a directory with all 
wayland-protocols'
  protocols, like xdg-shell, and so on.
  
  Example usage
  
find_package(WaylandProtocols REQUIRED)

ecm_add_wayland_client_protocol(CLIENT_LIB_SRCS
PROTOCOL ${WaylandProtocols_DATADIR}/stable/xdg-shell/xdg-shell.xml
BASENAME xdg-shell
)

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  find-wayland-protocols

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

AFFECTED FILES
  docs/find-module/FindWaylandProtocols.rst
  find-modules/FindWaylandProtocols.cmake

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

D10747: Implement zwp_linux_dmabuf_v1

[Vlad Zagorodniy]

zzag added a comment.


  > Update protocol xml to wayland-protocols master
  
  OT: there's a package called wayland-protocols. We could use it in order to 
avoid copying protocol specs.
  
  name=FindWaylandProtocols.cmake
#.rst:
# FindWaylandProtocols
# ---
#
# Try to find wayland-protocols on a Unix system.
#
# This will define the following variables:
#
# ``WaylandProtocols_FOUND``
# True if (the requested version of) wayland-protocols is available
# ``WaylandProtocols_VERSION``
# The version of wayland-protocols
# ``WaylandProtocols_DATADIR``
# The wayland protocols data directory

find_package(PkgConfig)
pkg_check_modules(PKG_wayland_protocols QUIET wayland-protocols)

set(WaylandProtocols_VERSION ${PKG_wayland_protocols_VERSION})
pkg_get_variable(WaylandProtocols_DATADIR wayland-protocols pkgdatadir)

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(WaylandProtocols
FOUND_VAR WaylandProtocols_FOUND
REQUIRED_VARS WaylandProtocols_DATADIR
VERSION_VAR WaylandProtocols_VERSION
)

include(FeatureSummary)
set_package_properties(WaylandProtocols PROPERTIES
URL "https://wayland.freedesktop.org/;
DESCRIPTION "Specifications of extended Wayland protocols"
)
  
  and then use it as follows
  
find_package(WaylandProtocols 1.8 REQUIRED)


${WaylandProtocols_DATADIR}/unstable/linux-dmabuf/linux-dmabuf-unstable-v1.xml

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #plasma, graesslin, davidedmundson, mart, fredrik
Cc: zzag, kde-frameworks-devel, romangg, plasma-devel, LeGast00n, hannahk, 
jraleigh, sgureev, fbampaloukas, GB_2, ragreen, Pitel, schernikov, michaelh, 
ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D20193: Expose locked keystates on KModifierKeyInfo when on wayland

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> kmodifierkeyinfo.cpp:34-44
> +#ifdef WITH_XCB
> +if (qGuiApp->platformName() == QLatin1String("xcb"))
> +return new KModifierKeyInfoProviderXcb;
> +else
> +#endif
> +#ifdef WITH_WAYLAND
> +if (qGuiApp->platformName() == QLatin1String("wayland"))

You don't need `else`s, e.g.

  static KModifierKeyInfoProvider *createProvider()
  {
  #ifdef WITH_XCB
  if (qGuiApp->platformName() == QLatin1String("xcb")) {
  return new KModifierKeyInfoProviderXcb;
  }
  #endif
  #ifdef WITH_WAYLAND
  if (qGuiApp->platformName() == QLatin1String("wayland")) {
  return new KModifierKeyInfoProviderWayland;
  }
  #endif
  return new KModifierKeyInfoProvider;
  }

> kmodifierkeyinfoprovider.cpp:90
> +{
> +auto  = m_modifierStates[key];
> +if (newState != state) {

Abuse of `auto`.

> kmodifierkeyinfoprovider.cpp:92
> +if (newState != state) {
> +const auto difference = (newState ^ state);
> +state = newState;

Same here.

> dfaure wrote in kmodifierkeyinfoprovider_wayland.cpp:36
> static

Needs to be a static function instead.

> kmodifierkeyinfoprovider_wayland.cpp:38-45
> +switch(state) {
> +case state_unlocked:
> +return KModifierKeyInfoProvider::Nothing;
> +case state_latched:
> +return KModifierKeyInfoProvider::Latched;
> +case state_locked:
> +return KModifierKeyInfoProvider::Locked;

https://techbase.kde.org/Policies/Frameworks_Coding_Style#Switch_Statements

> kmodifierkeyinfoprovider_wayland.h:29
>  {
> -}
> +Q_OBJECT
> +public:

Indent it.

REPOSITORY
  R273 KGuiAddons

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

To: apol, romangg, dfaure
Cc: zzag, romangg, dfaure, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D18933: Allow compositors to send discrete axis values

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:ddff2c06d17a: Allow compositors to send discrete axis 
values (authored by zzag).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18933?vs=57883=58186

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

AFFECTED FILES
  autotests/client/test_wayland_seat.cpp
  src/client/pointer.cpp
  src/client/pointer.h
  src/server/pointer_interface.cpp
  src/server/pointer_interface.h
  src/server/seat_interface.cpp
  src/server/seat_interface.h

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

Re: Deprecating KDEWebKit?

[Vlad Zagorodniy]

On 5/1/19 1:39 PM, Volker Krause wrote:

- Should we remove KDEWebKit from the default frameworks module set in kdesrc-
build, so that especially newcomers aren't confused by the common but harmless
build error caused by it? Applications actually needing it could still pull it
in via an explicit dependency.


+1

Cheers,
Vlad

D20595: Implement set_window_geometry

[Vlad Zagorodniy]

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

REPOSITORY
  R127 KWayland

BRANCH
  master

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

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

D18933: Allow compositors to send discrete axis values

[Vlad Zagorodniy]

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


  Small cleanup.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18933?vs=57882=57883

BRANCH
  pointer-axis-value-discrete

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

AFFECTED FILES
  autotests/client/test_wayland_seat.cpp
  src/client/pointer.cpp
  src/client/pointer.h
  src/server/pointer_interface.cpp
  src/server/pointer_interface.h
  src/server/seat_interface.cpp
  src/server/seat_interface.h

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

D18933: Allow compositors to send discrete axis values

[Vlad Zagorodniy]

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


  @since 5.XX becomes @since 5.59

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18933?vs=51659=57882

BRANCH
  pointer-axis-value-discrete

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

AFFECTED FILES
  autotests/client/test_wayland_seat.cpp
  src/client/pointer.cpp
  src/client/pointer.h
  src/server/pointer_interface.cpp
  src/server/pointer_interface.h
  src/server/seat_interface.cpp
  src/server/seat_interface.h

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

D14910: Pass a dedicated fd to each keyboard for the xkb keymap

[Vlad Zagorodniy]

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

REPOSITORY
  R127 KWayland

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

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

D20627: Introduce CriticalNotificationType

[Vlad Zagorodniy]

zzag added subscribers: graesslin, zzag.
zzag accepted this revision.
zzag added a comment.
This revision is now accepted and ready to land.


  Hmm, in general this is quite delicate case because KWin/X11 is feature 
frozen...
  
  Given that there is no other way around (except using above state for 
critical notifications or letting plasmashell keep track of fullscreen state), 
the only option is to break the rule. :/
  
  But you may still want to wait for input from other #KWin 
 folks as well, more specifically from 
@graesslin.

REPOSITORY
  R278 KWindowSystem

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

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

D20628: Add CriticalNotification window type to PlasmaShellSurface protocol

[Vlad Zagorodniy]

zzag added a comment.


  Please land together with other revisions.

REPOSITORY
  R127 KWayland

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

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

D20628: Add CriticalNotification window type to PlasmaShellSurface protocol

[Vlad Zagorodniy]

zzag accepted this revision.
zzag added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> davidedmundson wrote in plasma-shell.xml:194
> since="6"
> 
> See
> https://github.com/wayland-project/wayland/blob/master/protocol/wayland.dtd

Also, folks usually put

  

:-)

REPOSITORY
  R127 KWayland

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

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

D20805: [Window Thumbnail] Also monitor scene visibility and clean up

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> windowthumbnail.cpp:213
> +case ItemEnabledHasChanged:
> +Q_FALLTHROUGH();
> +case ItemVisibleHasChanged:

Do you actually need Q_FALLTHROUGH() here?

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D20595: Implement set_window_geometry

[Vlad Zagorodniy]

zzag added a comment.


  > Maybe we can revisit once I've done the kwin side?
  
  Sure.

REPOSITORY
  R127 KWayland

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

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

D20595: Implement set_window_geometry

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> xdgshell_interface.h:283
> + * @brief windowGeometry
> + * The position of the window within the buffer
> + *

s/position/geometry/

> xdgshell_stable_interface.cpp:524
> +if (s->m_topLevel) {
> +s->m_topLevel->d_func()->windowGeometry = QRect(windowRect);
> +emit s->m_topLevel->windowGeometryChanged(windowRect);

The copy constructor is redundant (most likely a leftover I guess).

REPOSITORY
  R127 KWayland

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

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

D20595: Implement set_window_geometry

[Vlad Zagorodniy]

zzag added a comment.


  What about xdg_shell_unstable_v6?

REPOSITORY
  R127 KWayland

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

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

D18824: Implement wl_eglstream_controller Server Interface

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> eglstream_controller_interface.cpp:103
> +{
> +dynamic_cast(*d).create();
> +}

Can we avoid dynamic casting a reference? Is it possible to use static_cast 
instead?

> eglstream_controller_interface_p.h:33
> +
> +class EglStreamControllerInterface::Private : public Global::Private
> +{

Should be marked with Q_DECL_HIDDEN.

REPOSITORY
  R127 KWayland

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

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

D18824: Implement wl_eglstream_controller Server Interface

[Vlad Zagorodniy]

zzag resigned from this revision.
zzag added a comment.


  I don't have any NVIDIA hardware to test these patches so I'm kinda useless.

REPOSITORY
  R127 KWayland

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

To: ekurzinger, romangg, davidedmundson, #kwin
Cc: ngraham, mati865, schernikov, guilhermesi, kde-frameworks-devel, michaelh, 
bruns

D18824: Implement wl_eglstream_controller Server Interface

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> eglstream_controller_interface.cpp:58
> +Q_UNUSED(client);
> +Private *p = (Private *)wl_resource_get_user_data(resource);
> +emit p->q->streamConsumerAttached(SurfaceInterface::get(surface), (void 
> *)eglStream, attribs);

Please don't use c style casts.

REPOSITORY
  R127 KWayland

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

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

D18933: Allow compositors to send discrete axis values

[Vlad Zagorodniy]

zzag retitled this revision from "Allow compositors to send discrete axis 
deltas" to "Allow compositors to send discrete axis values".

REPOSITORY
  R127 KWayland

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

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

D18933: Allow compositors to send discrete axis deltas

[Vlad Zagorodniy]

zzag retitled this revision from "Send optional events from wl_pointer v5" to 
"Allow compositors to send discrete axis deltas".

REPOSITORY
  R127 KWayland

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

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

D20105: ecm_add_wayland_client_protocol: Improve error messages

[Vlad Zagorodniy]

zzag added a comment.


  Hmm it looks like we're using ancient `code` option.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

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

D20105: ecm_add_wayland_client_protocol: Improve error messages

[Vlad Zagorodniy]

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

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

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

D19617: Support NET_WM_STATE_FOCUSED

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> netwm_def.h:506
> +  Only the window manager is allowed to change it.
> +  @since 5.56
> + **/

5.56 or 5.57?

REPOSITORY
  R278 KWindowSystem

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

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

D19544: Send blur/background contrast in device pixels

[Vlad Zagorodniy]

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

REPOSITORY
  R278 KWindowSystem

BRANCH
  master

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

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

D19544: Send blur/background contrast in device pixels

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> kwindoweffects.cpp:266
> +auto dpr = qApp->devicePixelRatio();
> +data << r.x() * dpr << r.y() * dpr << r.width() * dpr << 
> r.height() * dpr;
>  }

should we floor or round?

REPOSITORY
  R278 KWindowSystem

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

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

D11924: Add "SkipSwitcher" to API

[Vlad Zagorodniy]

zzag added a comment.


  @dfaure cc765bfb3f90a52410cbc0d2324de121cdffda61 


REPOSITORY
  R278 KWindowSystem

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

To: sharvey, hein, graesslin
Cc: zzag, dfaure, kde-frameworks-devel, #plasma, ngraham, graesslin, apol, 
michaelh, bruns

D19536: [autotests] Fix KWindowInfoX11Test::testState

[Vlad Zagorodniy]

This revision was automatically updated to reflect the committed changes.
Closed by commit R278:cc765bfb3f90: [autotests] Fix 
KWindowInfoX11Test::testState (authored by zzag).

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19536?vs=53192=53193

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

AFFECTED FILES
  autotests/kwindowinfox11test.cpp

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

D19536: [autotests] Fix KWindowInfoX11Test::testState

[Vlad Zagorodniy]

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


  sneaky update

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19536?vs=53190=53192

BRANCH
  foo

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

AFFECTED FILES
  autotests/kwindowinfox11test.cpp

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

D19536: [autotests] Fix KWindowInfoX11Test::testState

[Vlad Zagorodniy]

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


  check supported states

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19536?vs=53187=53190

BRANCH
  foo

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

AFFECTED FILES
  autotests/kwindowinfox11test.cpp

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

D19536: [autotests] Fix KWindowInfoX11Test::testState

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R278 KWindowSystem

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

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

D19536: [autotests] Fix KWindowInfoX11Test::testState

[Vlad Zagorodniy]

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

REVISION SUMMARY
  KWindowInfoX11Test::testState(skipSwitcher) fails because Openbox
  doesn't implement _KDE_NET_WM_STATE_SKIP_SWITCHER.

REPOSITORY
  R278 KWindowSystem

BRANCH
  foo

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

AFFECTED FILES
  autotests/kwindowinfox11test.cpp

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

D11924: Add "SkipSwitcher" to API

[Vlad Zagorodniy]

zzag added a comment.


  @dfaure I'll take a look into this issue.

REPOSITORY
  R278 KWindowSystem

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

To: sharvey, hein, graesslin
Cc: zzag, dfaure, kde-frameworks-devel, #plasma, ngraham, graesslin, apol, 
michaelh, bruns

D19476: Hopefully fix flaky remote access test

[Vlad Zagorodniy]

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

REPOSITORY
  R127 KWayland

BRANCH
  master

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

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

D18933: Send optional events from wl_pointer v5

[Vlad Zagorodniy]

zzag added a comment.


  In D18933#411966 , @davidedmundson 
wrote:
  
  > IMHO we should make frame() explicitly sent from the caller.
  
  
  Yeah, I'd prefer this way as well to be honest...
  
  > Hard to add retroactively in KF5 though :/
  
  So, it would be KF6 stuff, right?

REPOSITORY
  R127 KWayland

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

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

D18933: Send optional events from wl_pointer v5

[Vlad Zagorodniy]

zzag added a subscriber: davidedmundson.
zzag added a comment.


  @davidedmundson Would it make sense to have something like
  
void pointerAxisV5(Qt::Orientations orientations, const QPointF ,
   const QPoint , PointerAxisSource source);
  
  so we could send a diagonal scroll motion in one frame?

REPOSITORY
  R127 KWayland

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

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

D18933: Send optional events from wl_pointer v5

[Vlad Zagorodniy]

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


  Add Unknown axis source.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18933?vs=51438=51659

BRANCH
  pointer-axis-value-discrete

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

AFFECTED FILES
  autotests/client/test_wayland_seat.cpp
  src/client/pointer.cpp
  src/client/pointer.h
  src/server/pointer_interface.cpp
  src/server/pointer_interface.h
  src/server/seat_interface.cpp
  src/server/seat_interface.h

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

D18953: Update copy paste error in XdgShellPopupStable member vars

[Vlad Zagorodniy]

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

REPOSITORY
  R127 KWayland

BRANCH
  master

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

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

D18954: Add missing XdgShellPopup::ackConfigure

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> xdgshell.h:595
> + * request, passing along the @p serial of the configure event.
> + * @see configureRequested
> + **/

Also, maybe @since?

REPOSITORY
  R127 KWayland

BRANCH
  master

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

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

D18954: Add missing XdgShellPopup::ackConfigure

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> xdgshell_v6.cpp:588
> +}
> +
>  

here as well

REPOSITORY
  R127 KWayland

BRANCH
  master

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

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

D18954: Add missing XdgShellPopup::ackConfigure

[Vlad Zagorodniy]

zzag accepted this revision.
zzag added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> xdgshell.cpp:367
> +}
> +
> +

Please remove one extra new line.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

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

D18933: Send optional events from wl_pointer v5

[Vlad Zagorodniy]

zzag edited the summary of this revision.

REPOSITORY
  R127 KWayland

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

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

D18933: Send optional events from wl_pointer v5

[Vlad Zagorodniy]

zzag retitled this revision from "Implement optional events from wl_pointer v5" 
to "Send optional events from wl_pointer v5".

REPOSITORY
  R127 KWayland

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

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

D18933: Implement optional events from wl_pointer v5

[Vlad Zagorodniy]

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

REVISION SUMMARY
  So far not all v5 features were implemented because most of them are
  optional. But given that XWayland needs axis_discrete event maybe it's
  time to implement them.

REPOSITORY
  R127 KWayland

BRANCH
  pointer-axis-value-discrete

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

AFFECTED FILES
  autotests/client/test_wayland_seat.cpp
  src/client/pointer.cpp
  src/client/pointer.h
  src/server/pointer_interface.cpp
  src/server/pointer_interface.h
  src/server/seat_interface.cpp
  src/server/seat_interface.h

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

D16735: Implement text_input_unstable_v3

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> test_text_input.cpp:545-548
> +QFETCH(bool, clientCommits);
> +if (clientCommits) {
> +textInput->commit();
> +}

Personal opinion: tests have to be easy to follow, it's okay if we copy some 
code. I'd prefer to have a specialized test, e.g. testSurroundingTextV3.

> test_text_input.cpp:1001
> +
> +QTest::newRow("UnstableV3") << TextInputInterfaceVersion::UnstableV3;
> +}

If there is only one row, we don't need _data().

> textinput.h:122
> + * of the surrounding text or cursor position.
> + */
> +enum class ChangeCause : uint32_t {

**/

> textinput_p.h:83
> +/**
> + * Creates a new TextInputManagerUnstableV0.
> + * Note: after constructing the TextInputManagerUnstableV0 it is not yet 
> valid and one needs

V3?

> textinput_interface_v3.cpp:137-140
> +/*
> + * Pending data can only be amended when the interface
> + * is (about to be) enabled.
> + */

Please use single line comments instead.

> textinput_interface_v3.cpp:428
> +
> +namespace {
> +static TextInputInterface::ChangeCause 
> waylandCauseToKWayland(zwp_text_input_v3_change_cause wlCause)

We don't need this namespace because waylandCauseToKWayland is static.

REPOSITORY
  R127 KWayland

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

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

D18739: Sync QML module padding to reflect system setting pages

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> kcmultidialog.cpp:40-41
>  #include 
> +#include 
> +#include 
>  

It looks like includes are sorted. Maybe keep it that way?

REPOSITORY
  R295 KCMUtils

BRANCH
  master

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

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

D18834: Change glowbar.svgz to smoother style

[Vlad Zagorodniy]

zzag added a comment.


  Also, it's worth to mention glowbar from Oxygen theme 
(plasma-framework/src/desktoptheme/oxygen/widgets/glowbar.svgz).

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ndavis, #vdg, rooty
Cc: zzag, rooty, rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D18834: Change glowbar.svgz to smoother style

[Vlad Zagorodniy]

zzag added a comment.


  In D18834#407533 , @ndavis wrote:
  
  > I had a look through the Screen Edge effect code and while I'm no expert, I 
didn't see anything that would obviously make the corners large.
  
  
  The screen edge effect scales corner tiles and I don't think this will be 
changed.
  
  If slightly bigger corner glows is really such a big issue, then as a 
potential solution, the gradients could be "scaled" down a little bit.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ndavis, #vdg, rooty
Cc: zzag, rooty, rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D18763: [client] Fix typos

[Vlad Zagorodniy]

zzag abandoned this revision.
zzag added a comment.


  Well ok.

REPOSITORY
  R127 KWayland

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

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

D18763: [client] Fix typos

[Vlad Zagorodniy]

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

REPOSITORY
  R127 KWayland

BRANCH
  fix-typos

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

AFFECTED FILES
  src/client/datadevicemanager.h

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

D15910: Implement wl_surface::damage_buffer

[Vlad Zagorodniy]

zzag added a comment.


  Is it good now?

REPOSITORY
  R127 KWayland

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

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

D18545: [breeze desktop theme/dialogs] Add rounded corners to dialogs

[Vlad Zagorodniy]

zzag added a comment.


  The blur effect is able to handle rounded corners, a client just has to 
upload proper blur region. Yes, I had the latest version of the diff. I don't 
use the zoom effect.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: rooty, #vdg, ngraham
Cc: zzag, davidedmundson, Codezela, filipf, kde-frameworks-devel, michaelh, 
ngraham, bruns

D18545: [breeze desktop theme/dialogs] Add rounded corners to dialogs

[Vlad Zagorodniy]

zzag added a comment.


  ... though I'm not sure whether that's a bug.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: rooty, #vdg, ngraham
Cc: zzag, davidedmundson, Codezela, filipf, kde-frameworks-devel, michaelh, 
ngraham, bruns

D18545: [breeze desktop theme/dialogs] Add rounded corners to dialogs

[Vlad Zagorodniy]

zzag added a comment.


  Well, there are still issues with corners
  
  F6580225: Screenshot_20190131_151157.png 

  
  (both the blur and the background contrast effect are disabled)

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: rooty, #vdg, ngraham
Cc: zzag, davidedmundson, Codezela, filipf, kde-frameworks-devel, michaelh, 
ngraham, bruns