D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Martin Flöser]

graesslin added a comment.


  The human error exists as long as clang-tidy is not used. What I fear is that 
someone does a hand porting - we have seen several attempts to do that in KWin 
from various developers. If devs don't know and now fix the warnings, they can 
bring in human error.
  
  Thus I suggest that those who think this should be the default for all 
projects by KDE do the work to run clang-tidy over the complete KDE code base 
and afterwards enable this warning.
  
  I'm just not happy with the approach of breaking workflow without any 
discussion at all with the larger community. We have points in time where we 
can break things. E.g. the upcoming Qt 6. What I do not like is breaking in the 
middle of a release cycle without any coordination. Also I don't want to spend 
my very little spare time hunting behind what broke KWin build. I'm really not 
pleased about this from above attitude to break the compile of projects. It's 
one of the "dann macht euren Scheiss doch selbst" moments.
  
  Btw. of course KWin is fully maintained - also the old xcb code. It's just 
not possible to review 500+ line changes with someone adding override. 
Furthermore we have here virtuals which nobody touched for 15 years, but git 
blame on them is super important. And if you wonder: we have 1555 override in 
KWin. We use the new possibilities. We just don't adjust the old code base 
which nobody touches. I'm sure you all will be the first one to yell if KWin 
breaks and renders your desktop unusable. Yes we have very strict requirements 
on stability. A change for the sake of change is not done in KWin.
  
  So please revert this change and do a proper approach, talk to the community, 
ensure that this doesn't cause any new warnings and thus make it a useful new 
warning. In the current state it only causes work without any benefits for KWin.

REPOSITORY
  R240 Extra CMake Modules

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

To: aacid
Cc: kossebau, graesslin, apol, vkrause, kde-frameworks-devel, kde-buildsystem, 
michaelh, ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Martin Flöser]

graesslin added a comment.


  For done code this warning is pointless and negative. I invite you to work 
with a code base like KWin where it is more important to have a working git 
blame than protection for theoretical problems. Nobody will be able to 
guarantee that a 500+ change to add override won't break. Human errors happen, 
nobody will be able to review something like that. Addressing this warning by 
adding override all over our legacy code base has a serious risk.
  
  I'm seriously pissed that this is forced on us and we have to change code.
  
  I'm totally fine with this warning for new code and new projects. But let 
projects opt in for it instead of forcing it on legacy code bases.

REPOSITORY
  R240 Extra CMake Modules

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

To: aacid
Cc: graesslin, apol, vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

D18167: Move -Wsuggest-override -Wlogical-op to regular compiler settings

[Martin Flöser]

graesslin added a comment.


  This causes in KWin 500+ new warnings. Do you really think it's a good idea 
to spam all of KDE with new compiler warnings. KDE has an old code base. We 
cannot enable warnings for the way you developed C++ for 20 years.

REPOSITORY
  R240 Extra CMake Modules

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

To: aacid
Cc: graesslin, apol, vkrause, kde-frameworks-devel, kde-buildsystem, michaelh, 
ngraham, bruns

D18114: FakeInput: add support for pointer move with absolute coordinates and keyboard key press and release

[Martin Flöser]

graesslin added a comment.


  I'm the author of this interface and I consider it nowadays as a mistake to 
have added it. If I would restart I would do the whole thing differently: input 
events need to be injected through a kernel module in a dedicated input device. 
It would need a user space component which processes can talk to and that would 
need to be secured though e.g. polkit. Thus we could properly authenticate. 
This protocol does not support it and KWin does not even try to authenticate. 
Overall from security perspective this is a mess.

REPOSITORY
  R127 KWayland

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

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

D18114: FakeInput: add support for pointer move with absolute coordinates and keyboard key press and release

[Martin Flöser]

graesslin added a comment.


  I don't want to have key press/release in it. It has a reason that I didn't 
add key press/release from the start. The problem is the mismatch of key events 
and keysyms. Especially the kdeconnect use case for which this interface was 
developed has this problem. Android composes text, not key codes. Thus it would 
not be possible to inject key events.
  
  Also from a security perspective I'm strictly against adding fake key events. 
That's one of the largest security problems on X11. I rather have no remote 
support than a gaping security hole.

REPOSITORY
  R127 KWayland

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

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

D18114: FakeInput: add support for pointer move with absolute coordinates

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> fake-input.xml:44
>
> +  
> +  

New requests can only be added at end. Since is missing.

> fakeinput_interface.cpp:85
>  pointerMotionCallback,
> +pointerMotionAbsoluteCallback,
>  buttonCallback,

Must be last, new requests only at end.

REPOSITORY
  R127 KWayland

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

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

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

[Martin Flöser]

graesslin added a subscriber: plasma-devel.

REPOSITORY
  R127 KWayland

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

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

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

[Martin Flöser]

graesslin added a comment.


  Sorry for strange upload, arc decided to no longer like me.
  
  I created the patch on Jun 30, 2017. Unfortunately the issue was embargo on 
freedesktop.org with the idea of a coordinated release. Today the patch for 
weston landed so I consider the embargo as lifted. I had send the patch already 
to secur...@kde.org last year. Relevant freedesktop org bug is: 
https://bugs.freedesktop.org/show_bug.cgi?id=101595 - it might be that it's 
still private, if that's the case, please leave a note and I'll try to add to 
cc.
  
  Personal note: we are expecting our child any day now. So I cannot guarantee 
that I will have time to follow up or to push it. If you see inactivity: feel 
free to take over the patch and push it.

REPOSITORY
  R127 KWayland

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

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

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

[Martin Flöser]

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

REVISION SUMMARY
  To better isolate the clients from each other eachh KeyboardInterface
  creates it's own dedicated temporary file and sends the fd for this
  temporary file to the client. This means the memory for the keymap is no
  longer shared between all clients, every client has an own copy.
  
  To support this the existing API to set the keymap is deprecated and
  replaced by a new method setKeymapData which takes the content of the
  keymap as a byte array. The now deprecated method which takes a file
  descriptor is changed to use the new setKeymapData method. For that it
  reads the content of the file.
  
  The implementation in KeyboardInterface to create the file descriptor is
  based on the implementation of KWin. As I implemented the change in KWin
  (see 3b4c508ee36ac74c37e77fcaa14d106397ad2994 
) it 
is not a problem from
  GPL vs LGPL perspective.
  
  The change includes test cases to verify that the content of the keymap
  is properly passed to the client and that the memory is no longer shared.
  
  BUG: 381674

REPOSITORY
  R127 KWayland

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

AFFECTED FILES
  autotests/client/test_wayland_seat.cpp
  src/server/keyboard_interface.cpp
  src/server/keyboard_interface.h
  src/server/keyboard_interface_p.h
  src/server/seat_interface.cpp
  src/server/seat_interface.h
  src/server/seat_interface_p.h

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

D13541: Port solid from Qt5::Widgets to Qt5::Gui

[Martin Flöser]

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

REVISION SUMMARY
  The only usage of widgets was to get the active window. Incorrectly this
  was done through QWidget which would fail for any application using just
  QtGui (e.g. QtQuick).
  
  As QGuiApplication also provides access to the focus window we can use
  QtGui API instead. As a note: the whole thing is X11 specific as a
  window ID is passed to another process, which cannot work on Wayland.
  It's possible to get it work on Wayland, but that would require:
  
  - changes to the dbus protocol
  - add a dependency to KWayland
  - or extend QtWayland to generate the required code

TEST PLAN
  Compiles

REPOSITORY
  R245 Solid

BRANCH
  gui-instead-of-widgets

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

AFFECTED FILES
  CMakeLists.txt
  src/solid/CMakeLists.txt
  src/solid/devices/backends/udisks2/udisksstorageaccess.cpp

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

D13535: Do not cancel old clipboard selection if it is same as the new one.

[Martin Flöser]

graesslin added a comment.


  I would appreciate a unit test for the issue.

REPOSITORY
  R127 KWayland

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

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

D13037: [Dialog] Set PlasmaShellSurface on PlasmaShell being announced

[Martin Flöser]

graesslin added a comment.


  I wouldn't change plasmaquick based on KWin's behavior. KWin is weird after 
all. We should rather work around inside KWin.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D11925: Add "SkipSwitcher" to API

[Martin Flöser]

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


  wonderful! Thanks to you two :-) And yes as soon as you push this change you 
can push the kwin change.

REPOSITORY
  R127 KWayland

BRANCH
  arcpatch-D11925

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

To: sharvey, hein, graesslin
Cc: kde-frameworks-devel, lbeltrame, rikmills, davidedmundson, #plasma, 
graesslin, michaelh, ngraham, bruns

D12820: Add KWayland virtual desktop protocol

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> mart wrote in org_kde_plasma_virtual_desktop.xml:45
> the virtualdesktop would then have an output property used for filtering?
> or, the layout event could be kept in the manager, but be not just rows and 
> column, but pass the pair output and actual vector containing the 
> virtualdesktop instances?
> 
> also if it is per output then two desktops can be on the same row/column, as 
> long as they are on different outputs..
> then also all the api for requesting/notifying activate/deactivate would need 
> an output parameter.. /o\
> 
> My primary use for this protocol was to make activities==virtual desktops 
> (the quick way would be to just use the activity ids as desktop ids and use 
> kactivitymanager to add/remove, so no need for api to ask kwin to add/remove 
> desktops)
> this clashes a bit with different desktops per output, tough could be solved 
> if i then make the activity id a separate property in the Virtualdesktop 
> interface...

Please note that your quick way would not work. Activities are disabled in KWin 
Wayland and I don't see them getting enabled again. Activities resulted in hard 
freezes several times and I don't trust them due to that. As maintainer I would 
need very convincing arguments on why activities won't break KWin again. Due to 
the fact that kamd speaks both dbus and windowing protocol I don't see a chance 
that KWin can rely on it.

Due to the fact that activities used to block KWin it's an area which is 
completely not ported to Wayland yet. A Wayland window cannot be put on an 
activity. So this would require a significant amount of work. This also doesn't 
match anything we have in KWin to move forward. Activities have been in an 
unmaintained state since the start and the code is in a really bad state. None 
of the core KWin developers have touched it in the last year's. It's just a bad 
copy and paste of virtual desktops without any understanding of the domain. I 
do not want to see any future code being based on that. In KWin we started to 
evolve virtual desktops, so that in future it would be possible to implement 
the feature set of activities in virtual desktops. But for the time being it 
would mean X11 identifiers, that is numerical desktops.

I put know out an interesting question: do we need layouts?

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

D11925: Add "SkipSwitcher" to API

[Martin Flöser]

graesslin added a comment.
Restricted Application edited subscribers, added: kde-frameworks-devel; 
removed: Frameworks.


  any update on this? It's just the version increase in registry.cpp which is 
missing...

REPOSITORY
  R127 KWayland

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

To: sharvey, hein, graesslin
Cc: kde-frameworks-devel, lbeltrame, rikmills, davidedmundson, #plasma, 
graesslin, michaelh, ngraham, bruns, #frameworks

D7038: [server] Respect input region of sub-surfaces on pointer surface focus

[Martin Flöser]

graesslin added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  any update on this?

REPOSITORY
  R127 KWayland

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

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

D12820: Add KWayland virtual desktop protocol

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> mart wrote in org_kde_plasma_virtual_desktop.xml:45
> How you would envision different virtual desktops per output?
> to me is a bit of a weird concept, as an output is an actual desktop, not 
> virtual?

Have a look at i3. Btw it's a really often requested feature.

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

D12820: Add KWayland virtual desktop protocol

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> mart wrote in org_kde_plasma_virtual_desktop.xml:45
> Ok, so no layout information at all neither in the manager nor in the single 
> virtual desktops?
> I kindof like that as makes things a lot simpler. Tough in this case if the 
> concept of layout becomes completely internal in kwin, how to communicate it 
> to the client? a pager would still need to loosely represent the same layout.

A possibility could be a sibling concept. Every desktop could have a reference 
to the desktop to the left/top/right/bottom.

REPOSITORY
  R127 KWayland

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

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

D12820: Add KWayland virtual desktop protocol

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> org_kde_plasma_virtual_desktop.xml:4
> +  

D12388: Output device color curves correction

[Martin Flöser]

graesslin added inline comments.
Restricted Application added a subscriber: kde-frameworks-devel.

INLINE COMMENTS

> outputchangeset.cpp:91
>  }
> +
>  bool OutputChangeSet::positionChanged() const

nitpick: added unrelated new line

> outputdevice_interface.cpp:427
> +
> +org_kde_kwin_outputdevice_send_colorcurves(data.resource, , 
> , );
> +

you need to do a version check and only send to clients which have the required 
version. A FOO_SINCE macro should be defined by the generated wayland header.

REPOSITORY
  R127 KWayland

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

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

D12388: Output device color curves correction

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.


  Please increase the versions.

INLINE COMMENTS

> output-management.xml:82
>  
>  
>  

please increment version

> output-management.xml:142
>  
> +
> +

since is missing

> output-management.xml:142
>  
> +
> +

you cannot add requests in between, a new request must be the last.

> outputdevice.xml:32
>  
>  
>  

version needs incrementation

> outputdevice.xml:179
>  
> +
> +

you cannot add events in between, new one needs to be last.

REPOSITORY
  R127 KWayland

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

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

D11925: Add "SkipSwitcher" to API

[Martin Flöser]

graesslin added a comment.


  The adjustments in registry.cpp are still missing.

INLINE COMMENTS

> graesslin wrote in plasmawindowmanagement_interface.cpp:122
> you need to increase the version number.

this needs to be 9

REPOSITORY
  R127 KWayland

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

To: sharvey, hein, graesslin
Cc: davidedmundson, #plasma, graesslin, #frameworks, michaelh, bruns

D11925: Add "SkipSwitcher" to API

[Martin Flöser]

graesslin added a comment.


  There is one more file which need changes. In src/client/registry.cpp you 
need to increase the version numbers of the two changed protocols. Adding a new 
request can be really cumbersome.

INLINE COMMENTS

> plasmashell_interface.cpp:53
>  
>  const quint32 PlasmaShellInterface::Private::s_version = 4;
>  

you need to increase the version number.

> plasmashell_interface.cpp:168
>  setSkipTaskbarCallback,
> +setSkipSwitcherCallback,
>  panelAutoHideHideCallback,

This is at the wrong position. It needs to be the last entry in the list. That 
might be your compile problem you mentioned in the KWin request.

> plasmawindowmanagement_interface.cpp:122
>  
>  const quint32 PlasmaWindowManagementInterface::Private::s_version = 7;
>  

you need to increase the version number.

REPOSITORY
  R127 KWayland

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

To: sharvey, hein, graesslin
Cc: davidedmundson, #plasma, graesslin, #frameworks, michaelh, bruns

D11583: Add platform detection and adjustment to kded

[Martin Flöser]

graesslin added a comment.


  Btw could someone please push for me. I'm not around the next two weeks

REPOSITORY
  R297 KDED

BRANCH
  platform-detection

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

To: graesslin, #frameworks, #plasma, apol, romangg
Cc: michaelh, ngraham, bruns

D11583: Add platform detection and adjustment to kded

[Martin Flöser]

graesslin added a comment.


  In D11583#241314 , @apol wrote:
  
  > I wonder if all this code should be ifdef'd for Qt 5.10 though. The premise 
is that this won't be a problem with Qt 5.11, right?
  
  
  Let's wait for it to happen, then we can still ifdef

REPOSITORY
  R297 KDED

BRANCH
  platform-detection

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

To: graesslin, #frameworks, #plasma, apol, romangg
Cc: michaelh, ngraham, bruns

D11924: Add "SkipSwitcher" to API

[Martin Flöser]

graesslin added a comment.


  In D11924#240631 , @ngraham wrote:
  
  > Martin, @sharvey does not yet have commit access. Would you like to land 
this patch for him, or should I?
  
  
  Please push, I probably won't get to it the next two weeks.

REPOSITORY
  R278 KWindowSystem

BRANCH
  skip-switcher (branched from master)

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

To: sharvey, hein, graesslin
Cc: ngraham, graesslin, apol, #frameworks, michaelh

D11924: [WIP] Add "SkipSwitcher" to API

[Martin Flöser]

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

REPOSITORY
  R278 KWindowSystem

BRANCH
  skip-switcher (branched from master)

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

To: sharvey, hein, graesslin
Cc: ngraham, graesslin, apol, #frameworks, michaelh

D11924: [WIP] Add "SkipSwitcher" to API

[Martin Flöser]

graesslin added a comment.


  In D11924#240045 , @graesslin 
wrote:
  
  > I'm not sure whether we can add elements to the states.
  
  
  Just checked:
  
  An implementation MAY add new atoms to this list. Implementations without 
extensions MUST ignore any unknown atoms, effectively removing them from the 
list. These extension atoms MUST NOT start with the prefix _NET.
  
  So all is fine, but the prefix needs to change

REPOSITORY
  R278 KWindowSystem

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

To: sharvey, hein, graesslin
Cc: graesslin, apol, #frameworks, michaelh, ngraham

D11924: [WIP] Add "SkipSwitcher" to API

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.


  I'm not sure whether we can add elements to the states.

INLINE COMMENTS

> netwininfotestwm.cpp:432
>  const QByteArray skipTaskbar  = 
> QByteArrayLiteral("_NET_WM_STATE_SKIP_TASKBAR");
> +const QByteArray skipSwitcher = 
> QByteArrayLiteral("_NET_WM_STATE_SKIP_SWITCHER");
>  const QByteArray keepAbove= 
> QByteArrayLiteral("_NET_WM_STATE_ABOVE");

This needs a KDE prefix as it's a KDE specific extension

> netwm_def.h:502
>  **/
> -DemandsAttention = 1u << 11
> +DemandsAttention = 1u << 12
> +

You cannot change enum values. Please add the new one afterwards

> atoms_p.h:136
>  ENUM(_NET_WM_STATE_DEMANDS_ATTENTION),
> +ENUM(_NET_WM_STATE_SKIP_SWITCHER),
>  

Also a KDE prefix is needed

> netwm.cpp:26
>  
> -//#define NETWMDEBUG
> +// #define NETWMDEBUG
>  #include "netwm.h"

Unrelated change

REPOSITORY
  R278 KWindowSystem

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

To: sharvey, hein, graesslin
Cc: graesslin, apol, #frameworks, michaelh, ngraham

D11925: [WIP] Add "SkipSwitcher" to API

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> sharvey wrote in plasmawindowmodel.h:110
> Correct version number? 5.45?

I normally update those just before pushing to the next frameworks release 
number.

> plasma-shell.xml:339
> +
> +
> +

The since is wrong. It must be higher than the current version. You also need 
to increase the version number and I think this should be added as last request 
and not in between.

> plasma-window-management.xml:49
> since="3"/>
> +  
>  

Here also the since is wrong

REPOSITORY
  R127 KWayland

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

To: sharvey, hein, graesslin
Cc: graesslin, #frameworks, michaelh, ngraham

Re: KDE CI: Frameworks kwindowsystem kf5-qt5 FreeBSDQt5.9 - Build # 15 - Failure!

[Martin Flöser]

Hi Ben,

I pushed a possible fix. No guarnatees as this code obviously compiled 
on my system.


Could the compiler settings please be adjusted so that we have warnings 
be warnings on all ci systems and not fail on randomly? This was rather 
unexpected for me and makes it quite difficult to properly test before 
hand. The firefighting afterwards is something I don't like as mails 
with "fix this as soon as possible" are difficult if you don't have much 
time.


Cheers
Martin

Am 2018-03-24 11:25, schrieb Ben Cooksley:

Hi Martin,

Please see below. Could you please fix this as soon as possible?

This is blocking all FreeBSD Dependency builds meaning any change to
the binary compatibility state of the FreeBSD platform will break the
whole CI system on FreeBSD.

Cheers,
Ben

On Wed, Mar 21, 2018 at 6:51 PM, CI System  wrote:


BUILD FAILURE

Build URL


https://build.kde.org/job/Frameworks%20kwindowsystem%20kf5-qt5%20FreeBSDQt5.9/15/

[1]

Project:
Frameworks kwindowsystem kf5-qt5 FreeBSDQt5.9

Date of build:
Wed, 21 Mar 2018 05:50:19 +

Build duration:
1 min 31 sec and counting

CONSOLE OUTPUT

[...truncated 92.62 KB...]

[ 10%] Building CXX object


src/CMakeFiles/KF5WindowSystem.dir/ECMQmLoader-kwindowsystem5_qt.cpp.o


[ 11%] Building CXX object


src/CMakeFiles/KF5WindowSystem.dir/platforms/xcb/kselectionowner.cpp.o


[ 12%] Building CXX object


src/CMakeFiles/KF5WindowSystem.dir/platforms/xcb/kselectionwatcher.cpp.o


[ 12%] Building CXX object


src/CMakeFiles/KF5WindowSystem.dir/platforms/xcb/kxerrorhandler.cpp.o


[ 13%] Building CXX object
src/CMakeFiles/KF5WindowSystem.dir/platforms/xcb/kxutils.cpp.o

/usr/home/jenkins/workspace/Frameworks kwindowsystem kf5-qt5
FreeBSDQt5.9/src/platforms/xcb/kxerrorhandler.cpp:145:10: warning:
unused variable 'num' [-Wunused-variable]

char num[ 256 ];

^

1 warning generated.

[ 14%] Building CXX object
src/CMakeFiles/KF5WindowSystem.dir/platforms/xcb/kkeyserver.cpp.o

[ 15%] Building CXX object
src/CMakeFiles/KF5WindowSystem.dir/platforms/xcb/kxmessages.cpp.o

[ 16%] Building CXX object
src/CMakeFiles/KF5WindowSystem.dir/platforms/xcb/netwm.cpp.o

/usr/home/jenkins/workspace/Frameworks kwindowsystem kf5-qt5
FreeBSDQt5.9/src/platforms/xcb/kkeyserver.cpp:524:27: warning:
'XKeycodeToKeysym' is deprecated [-Wdeprecated-declarations]

keySymX = XKeycodeToKeysym(QX11Info::display(),
xmk->modifiermap[xmk->max_keypermod * i + j], k);

^

/usr/local/include/X11/Xlib.h:1687:15: note: 'XKeycodeToKeysym'
has been explicitly marked deprecated here

extern KeySym XKeycodeToKeysym(

^

/usr/home/jenkins/workspace/Frameworks kwindowsystem kf5-qt5
FreeBSDQt5.9/src/platforms/xcb/kkeyserver.cpp:719:20: warning:
'XKeycodeToKeysym' is deprecated [-Wdeprecated-declarations]

if (sym == XKeycodeToKeysym(QX11Info::display(), code, 0))

^

/usr/local/include/X11/Xlib.h:1687:15: note: 'XKeycodeToKeysym'
has been explicitly marked deprecated here

extern KeySym XKeycodeToKeysym(

^

/usr/home/jenkins/workspace/Frameworks kwindowsystem kf5-qt5
FreeBSDQt5.9/src/platforms/xcb/kkeyserver.cpp:721:25: warning:
'XKeycodeToKeysym' is deprecated [-Wdeprecated-declarations]

else if (sym == XKeycodeToKeysym(QX11Info::display(), code, 1)) {

^

/usr/local/include/X11/Xlib.h:1687:15: note: 'XKeycodeToKeysym'
has been explicitly marked deprecated here

extern KeySym XKeycodeToKeysym(

^

/usr/home/jenkins/workspace/Frameworks kwindowsystem kf5-qt5
FreeBSDQt5.9/src/platforms/xcb/kkeyserver.cpp:723:27: warning:
'XKeycodeToKeysym' is deprecated [-Wdeprecated-declarations]

} else if (sym == XKeycodeToKeysym(QX11Info::display(), code, 2))
{

^

/usr/local/include/X11/Xlib.h:1687:15: note: 'XKeycodeToKeysym'
has been explicitly marked deprecated here

extern KeySym XKeycodeToKeysym(

^

/usr/home/jenkins/workspace/Frameworks kwindowsystem kf5-qt5
FreeBSDQt5.9/src/platforms/xcb/kkeyserver.cpp:725:27: warning:
'XKeycodeToKeysym' is deprecated [-Wdeprecated-declarations]

} else if (sym == XKeycodeToKeysym(QX11Info::display(), code, 3))
{

^

/usr/local/include/X11/Xlib.h:1687:15: note: 'XKeycodeToKeysym'
has been explicitly marked deprecated here

extern KeySym XKeycodeToKeysym(

^

/usr/home/jenkins/workspace/Frameworks kwindowsystem kf5-qt5
FreeBSDQt5.9/src/platforms/xcb/kkeyserver.cpp:917:20: warning:
'XKeycodeToKeysym' is deprecated [-Wdeprecated-declarations]

uint sym = XKeycodeToKeysym(QX11Info::display(), keyCodeX, 0);

^

/usr/local/include/X11/Xlib.h:1687:15: note: 'XKeycodeToKeysym'
has been explicitly marked deprecated here

extern KeySym XKeycodeToKeysym(

^

[ 16%] Building CXX object


src/CMakeFiles/KF5WindowSystem.dir/KF5WindowSystem_autogen/mocs_compilation.cpp.o


6 warnings generated.

/usr/home/jenkins/workspace/Frameworks kwindowsystem kf5-qt5
FreeBSDQt5.9/src/platforms/xcb/netwm.cpp:2762:25: warning: cast from
'unsigned char *' to 'uint32_t *' (aka 'unsigned int *') increases
required alignment from 1 to 4 [-Wcast-align]

uint32_t *d32 = (uint32_t *) 

D11414: [xcb] Fix implementation of _NET_WM_FULLSCREEN_MONITORS

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> kossebau wrote in netwm.cpp:2845-2847
> Seems clang (at least as of FreeBSD with -Wc++11-narrowing) does not like 
> this narrowing from the int topology properties to the uint32_t array one:
> 
>   00:37:23 /usr/home/jenkins/workspace/Dependency Build Frameworks kf5-qt5 
> FreeBSDQt5.9/kwindowsystem/src/platforms/xcb/netwm.cpp:2846:13: error: 
> non-constant-expression cannot be narrowed from type 'int' to 'uint32_t' (aka 
> 'unsigned int') in initializer list [-Wc++11-narrowing]
>   00:37:23 topology.top, topology.bottom, topology.left, 
> topology.right, 1
>   00:37:23 ^~~~
> 
> https://build.kde.org/view/CI%20Management/job/Dependency%20Build%20Frameworks%20kf5-qt5%20FreeBSDQt5.9/25/console
> 
> Should this be fixed with some casting?

I guess we need to fix this

REPOSITORY
  R278 KWindowSystem

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

To: graesslin, #frameworks, #kwin, #plasma, davidedmundson
Cc: kossebau, michaelh, ngraham

D11583: Add platform detection and adjustment to kded

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: Frameworks, Plasma, apol.
Restricted Application added a project: Frameworks.
graesslin requested review of this revision.

REVISION SUMMARY
  Current Plasma/master branch does no longer set the QT_QPA_PLATFORM env
  variable on Wayland. As kded is a process tightly connected to the
  workspace it also needs to pick wayland QPA in a Wayland session. This
  change brings in the adjustment from plasma-workspace and ensures that
  kded works correctly on any Wayland desktop environment, being it
  Plasma, GNOME or Weston.

TEST PLAN
  Restarted session, kscreen got layout correctly

REPOSITORY
  R297 KDED

BRANCH
  platform-detection

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

AFFECTED FILES
  src/kded.cpp

To: graesslin, #frameworks, #plasma, apol
Cc: michaelh, ngraham

D11414: [xcb] Fix implementation of _NET_WM_FULLSCREEN_MONITORS

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R278:cc5d6fde1aba: [xcb] Fix implementation of 
_NET_WM_FULLSCREEN_MONITORS (authored by graesslin).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D11414?vs=29760=30083#toc

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11414?vs=29760=30083

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

AFFECTED FILES
  autotests/netwininfotestclient.cpp
  autotests/netwininfotestwm.cpp
  src/platforms/xcb/netwm.cpp

To: graesslin, #frameworks, #kwin, #plasma, davidedmundson
Cc: michaelh, ngraham

D11414: [xcb] Fix implementation of _NET_WM_FULLSCREEN_MONITORS

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: Frameworks, KWin, Plasma.
Restricted Application added a project: Frameworks.
graesslin requested review of this revision.

REVISION SUMMARY
  According to NETWM spec the client "wishing to change this list MUST send
  a _NET_WM_FULLSCREEN_MONITORS client message to the root window". This
  was not implemented at all, instead the property was updated. The
  property must be set by the window manager, or as the spec says:
  "The Window Manager MUST keep this list updated to reflect the current
  state of the window."
  
  In KWin (as the user of NETWinInfo) this was implemented correctly (see
  kwin, file netinfo.cpp method WinInfo::changeFullscreenMonitors).
  
  BUG: 391960

TEST PLAN
  New test case added which verifies the client/wm interplay,
  old and incorrect test case removed

REPOSITORY
  R278 KWindowSystem

BRANCH
  fix-fullscreen-monitors

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

AFFECTED FILES
  autotests/netwininfotestclient.cpp
  autotests/netwininfotestwm.cpp
  src/platforms/xcb/netwm.cpp

To: graesslin, #frameworks, #kwin, #plasma
Cc: michaelh, ngraham

D10235: [server] Add support for the frame semantics of Pointer version 5

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:b6bd28ab0492: [server] Add support for the frame 
semantics of Pointer version 5 (authored by graesslin).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D10235?vs=26336=28563#toc

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10235?vs=26336=28563

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

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

To: graesslin, #kwin, #plasma, #frameworks, romangg
Cc: romangg, plasma-devel, schernikov, michaelh, ZrenBot, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10081: Don't assert if used incorrectly from dbus

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R268:7d272782a55e: Dont assert if used incorrectly from 
dbus (authored by graesslin).

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10081?vs=26098=28562

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

AFFECTED FILES
  src/runtime/component.cpp

To: graesslin, #frameworks, apol, romangg
Cc: romangg, michaelh

D10142: [server] Properly handle the situation when the DataSource for a drag gets destroyed

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:2dfe16d774c7: [server] Properly handle the situation when 
the DataSource for a drag gets… (authored by graesslin).
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10142?vs=26054=28025

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

AFFECTED FILES
  autotests/client/test_drag_drop.cpp
  src/server/datadevice_interface.cpp
  src/server/seat_interface.cpp
  src/server/seat_interface_p.h

To: graesslin, #frameworks, #kwin, #plasma, romangg
Cc: plasma-devel, michaelh, kmorwinski, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart, schernikov, alexeymin, eliasp, hein

D10235: [server] Add support for the frame semantics of Pointer version 5

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.


  In D10235#213009 , @romangg wrote:
  
  > Shouldn't the sendFrame event sent by the compositor explicitly? The way 
it's currently send after each event compositors are not able to group these.
  
  
  Yes and no. For all the events we currently support, there is no grouping. 
For new event types, which we do not yet support, we need to add API to allow 
the compositer to send the frame.

REPOSITORY
  R127 KWayland

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

To: graesslin, #kwin, #plasma, #frameworks
Cc: romangg, plasma-devel, schernikov, michaelh, kmorwinski, ZrenBot, 
alexeymin, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D10081: Don't assert if used incorrectly from dbus

[Martin Flöser]

graesslin added a comment.


  ping

REPOSITORY
  R268 KGlobalAccel

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

To: graesslin, #frameworks
Cc: romangg, michaelh

D10142: [server] Properly handle the situation when the DataSource for a drag gets destroyed

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.


  ping

REPOSITORY
  R127 KWayland

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

To: graesslin, #frameworks, #kwin, #plasma
Cc: plasma-devel, michaelh, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart, schernikov, alexeymin, eliasp, hein

D10235: [server] Add support for the frame semantics of Pointer version 5

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.


  ping

REPOSITORY
  R127 KWayland

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

To: graesslin, #kwin, #plasma, #frameworks
Cc: plasma-devel, schernikov, michaelh, ZrenBot, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10495: Workaround to restore KF5 programs from system tray

[Martin Flöser]

graesslin added a comment.


  In D10495#205811 , @stikonas wrote:
  
  > This is more to start some discussion on system tray under Wayland. I'm not 
sure myself if this should be committed. I just use this workaround locally 
until winId() works on Wayland.
  
  
  winId will never work. I cannot say much on the patch as the context is 
missing.

REPOSITORY
  R289 KNotifications

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

To: stikonas, wbauer, #plasma, davidedmundson, volkov
Cc: graesslin, #kwin, plasma-devel, kde-frameworks-devel, #frameworks, 
michaelh, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D10142: [server] Properly handle the situation when the DataSource for a drag gets destroyed

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.


  ping

REPOSITORY
  R127 KWayland

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

To: graesslin, #frameworks, #kwin, #plasma
Cc: plasma-devel, schernikov, michaelh, ZrenBot, ngraham, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10300: [server] Don't crash when a subsurface gets commited whose parent surface got destroyed

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:d38825d934e4: [server] Dont crash when a subsurface 
gets commited whose parent surface got… (authored by graesslin).
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10300?vs=26513=26710

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

AFFECTED FILES
  autotests/client/test_wayland_subsurface.cpp
  src/server/subcompositor_interface.cpp
  src/server/surface_interface.cpp

To: graesslin, #frameworks, #kwin, #plasma, davidedmundson
Cc: lbeltrame, plasma-devel, michaelh, ZrenBot, ngraham, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10300: [server] Don't crash when a subsurface gets commited whose parent surface got destroyed

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.


  In https://phabricator.kde.org/D10300#202468, @lbeltrame wrote:
  
  > Ping? Why this has not been committed even if in "accepted" state?
  
  
  Because I wasn't at my system yet.

REPOSITORY
  R127 KWayland

BRANCH
  subsurface-parent-destroyed

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

To: graesslin, #frameworks, #kwin, #plasma, davidedmundson
Cc: lbeltrame, plasma-devel, schernikov, michaelh, ZrenBot, ngraham, alexeymin, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D9207: Don't load KDE platform module in kglobalaccel5

[Martin Flöser]

graesslin added a comment.


  In https://phabricator.kde.org/D9207#201848, @hein wrote:
  
  > Well, the maintainer spoke out against it, so not much I can do.
  
  
  Please be aware that I am no longer the maintainer of kglobalaccel. 
Nevertheless I recommend against it as the risk of breakage is high especially 
as nobody notices breakage during the frameworks dev cycle.

REPOSITORY
  R268 KGlobalAccel

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

To: hein, #plasma, davidedmundson, mart, graesslin
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, ngraham, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10314: Use F11 as the shortcut to toggle the aside preview

[Martin Flöser]

graesslin added a comment.


  In https://phabricator.kde.org/D10314#201553, @ngraham wrote:
  
  > Yeah, and Dolphin already overloads that by using F11 to show and hide the 
Information panel. I wouldn't object to using a different keyboard shortcut, 
but then we'd want to but we'd want to do it for all of for Dolphin's panels, 
and change the shortcuts here, too, which currently match Dolphin's.
  
  
  my suggestion would be changing Dolphin's shortcut. If you use F11 it's 
unexpected if the window does not go to fullscreen. Even for dolphin a 
fullscreen shortcut might make sense. And in the case of fullscreen: Firefox 
defined that as the default. We have to accept that ;-)

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, markg
Cc: graesslin, ltoscano, markg, michaelh, ngraham

D10314: Use F11 as the shortcut to toggle the aside preview

[Martin Flöser]

graesslin added a comment.


  I'm a little bit late to the party, but F11is kind of the standard key for 
fullscreen.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, markg
Cc: graesslin, ltoscano, markg, michaelh, ngraham

D10300: [server] Don't crash when a subsurface gets commited whose parent surface got destroyed

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: Frameworks, KWin, Plasma.
Restricted Application added projects: Plasma on Wayland, Frameworks.
Restricted Application added a subscriber: plasma-devel.
graesslin requested review of this revision.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REVISION SUMMARY
  Qt seems to damage and commit child subsurfaces although their parent
  got destroyed. This actually doesn't make any sense as without a parent
  surface they cannot be shown. But nevertheless we should not crash in
  such a situation.
  
  This change guards the places in the commit handling code where the
  parent gets accessed.
  
  BUG: 389231

TEST PLAN
  New test case which exposes the problem

REPOSITORY
  R127 KWayland

BRANCH
  subsurface-parent-destroyed

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

AFFECTED FILES
  autotests/client/test_wayland_subsurface.cpp
  src/server/subcompositor_interface.cpp
  src/server/surface_interface.cpp

To: graesslin, #frameworks, #kwin, #plasma
Cc: plasma-devel, michaelh, ZrenBot, ngraham, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10235: [server] Add support for the frame semantics of Pointer version 5

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: KWin, Plasma, Frameworks.
Restricted Application added projects: Plasma on Wayland, Frameworks.
Restricted Application added a subscriber: plasma-devel.
graesslin requested review of this revision.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REVISION SUMMARY
  This change implements the required changes for wl_seat version 5.
  There seem to be applications which require version 5 and refuse to
  start if the server doesn't provide it. Thus we need to provide it.
  
  The main difference with version 5 is that pointer need to send a
  frame event after each logical group of events. As we don't support the
  new events from version 5 yet, we just emit the frame after each event
  and implement the suggested semantics for the enter/leave behavior.
  
  To really make use of this, we will have to implement additions in the
  API and then in KWin to expose the new API elements. But to just support
  the semantics we don't need it.
  
  BUG: 389189

TEST PLAN
  Extended autotest

REPOSITORY
  R127 KWayland

BRANCH
  server-pointer-frame-semantics

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

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

To: graesslin, #kwin, #plasma, #frameworks
Cc: plasma-devel, michaelh, ZrenBot, ngraham, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10081: Don't assert if used incorrectly from dbus

[Martin Flöser]

graesslin updated this revision to Diff 26098.
graesslin added a comment.


  Remove the usless assert Roman found

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10081?vs=25895=26098

BRANCH
  dont-assert-on-dbus-misusage

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

AFFECTED FILES
  src/runtime/component.cpp

To: graesslin, #frameworks
Cc: romangg, michaelh

D10142: [server] Properly handle the situation when the DataSource for a drag gets destroyed

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: Frameworks, KWin, Plasma.
Restricted Application added projects: Plasma on Wayland, Frameworks.
Restricted Application added a subscriber: plasma-devel.
graesslin requested review of this revision.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REVISION SUMMARY
  This addresses the following situation:
  
  1. Start drag on a QtWayland based window
  2. Press escape
  3. Release mouse
  
  -> this results in a crash. The main reason for this is that QtWayland
  destroys the DataSource in step 2 and KWayland did not expect this at
  all. The drag and drop operation continued and results in step 3 in the
  drag target to request data from the no longer existing DataSource.
  
  This change addresses the root of the problem by cancelling the drag
  operation when the DataSource gets destroyed.
  
  BUG: 389221
  FIXED-IN: 5.43

TEST PLAN
  New test case exposing the problem and manual testing with
  kwin_wayland and dolphin (based on bug report)

REPOSITORY
  R127 KWayland

BRANCH
  cancel-drag-on-datasource-destroy

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

AFFECTED FILES
  autotests/client/test_drag_drop.cpp
  src/server/datadevice_interface.cpp
  src/server/seat_interface.cpp
  src/server/seat_interface_p.h

To: graesslin, #frameworks, #kwin, #plasma
Cc: plasma-devel, michaelh, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D10081: Don't assert if used incorrectly from dbus

[Martin Flöser]

graesslin created this revision.
graesslin added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
graesslin requested review of this revision.

REVISION SUMMARY
  The component dbus interface allowed to remote kill the wayland session
  by sending an incorrect request to the kglobalaccel dbus interface.
  
  There is no point in asserting, the code must be able to handle error
  cases and also is able to handle them.
  
  BUG: 389375
  FIXED-IN: 5.43

REPOSITORY
  R268 KGlobalAccel

BRANCH
  dont-assert-on-dbus-misusage

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

AFFECTED FILES
  src/runtime/component.cpp

To: graesslin, #frameworks

D10040: Add serial number and EISA ID to OutputDevice interface

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> outputdevice.xml:104-107
> + + summary="textual representation of serial number"/>
> + + summary="textual representation of EISA identifier"/>

I'm not sure whether it's allowed to add arguments to an existing event. This 
would result in incompatibilities. You can make the server only emit to clients 
having the version, but then you actually broke any client which only has 
version 1.

To be really compatible you need to add a new event.

REPOSITORY
  R127 KWayland

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

To: dvratil, graesslin, sebas
Cc: davidedmundson, plasma-devel, #frameworks, schernikov, ZrenBot, ngraham, 
alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, 
sebas, apol, mart, hein

[kglobalaccel] /: Stepping down as maintainer of KGlobalAccel

[Martin Flöser]

Git commit 147acd294dd08ab6c7468d34457d4d9d7995d988 by Martin Flöser.
Committed on 13/01/2018 at 15:24.
Pushed by graesslin into branch 'master'.

Stepping down as maintainer of KGlobalAccel

CCMAIL: kde-frameworks-devel@kde.org

M  +1-1metainfo.yaml

https://commits.kde.org/kglobalaccel/147acd294dd08ab6c7468d34457d4d9d7995d988

diff --git a/metainfo.yaml b/metainfo.yaml
index 8ac5136..dd5d9eb 100644
--- a/metainfo.yaml
+++ b/metainfo.yaml
@@ -1,4 +1,4 @@
-maintainer: graesslin
+maintainer:
 description: Add support for global workspace shortcuts
 tier: 3
 type: integration

[kwindowsystem] /: Stepping down as maintainer of KWindowSystem

[Martin Flöser]

Git commit 5b28310ef15360b05c1899a4cb229b4d71d28743 by Martin Flöser.
Committed on 13/01/2018 at 15:22.
Pushed by graesslin into branch 'master'.

Stepping down as maintainer of KWindowSystem

CCMAIL: kde-frameworks-devel@kde.org

M  +1-1metainfo.yaml

https://commits.kde.org/kwindowsystem/5b28310ef15360b05c1899a4cb229b4d71d28743

diff --git a/metainfo.yaml b/metainfo.yaml
index aa33c70..6fbdfe5 100644
--- a/metainfo.yaml
+++ b/metainfo.yaml
@@ -1,4 +1,4 @@
-maintainer: graesslin
+maintainer: 
 description: Access to the windowing system
 tier: 1
 type: integration

Stepping down as KGlobalAccel and KWindowSystem maintainer after 5.42

[Martin Flöser]

Hi all,

I had sent a mail some time ago asking for someone to step up as 
KGlobalAccel maintainer. I kept maintaining it as I didn't want to leave 
it in empty hands.


But I feel the time is come for me that I am no longer able to maintain 
KGlobalAccel and as well KWindowSystem. Both are too X11 specific and 
too much about window systems I don't care about. Sometimes I feel like 
standing in the way of evolving the frameworks as I am too conservative 
due to the fact that I don't use X11 any more.


I hope we find someone to step up. After 5.42 is released I'll push 
commits to those repositories removing me as maintainer.


Thank you
Martin

D9656: Fix naming inconsistency

[Martin Flöser]

graesslin accepted this revision.
This revision is now accepted and ready to land.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, #plasma, graesslin
Cc: plasma-devel, #frameworks, leezu, ZrenBot, ngraham, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D9599: Create interface for passing server decoration palettes

[Martin Flöser]

graesslin accepted this revision.
This revision is now accepted and ready to land.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, #plasma, graesslin
Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

Re: Required Compiler for KDE Frameworks

[Martin Flöser]

Am 2017-12-30 23:41, schrieb Dominik Haumann:

Hi,

on https://community.kde.org/Frameworks/Policies, it says:

  The following minimal compiler versions are supported
  by KDE Frameworks:

  - GCC 4.6
  - Clang 3.1
  - VS2012 (MSVC11)

Is this list really up-do-date?
Afaik, we even require VS2015 these days. What about the others?

Also: Can we use 'override' or are we still bound to Q_DECL_OVERRIDE?


KWayland uses override and nobody ever complained. Given that I would 
say that at least in reality we require more than GCC 4.6.


Cheers
Martin

D9550: Don't set the LD_LIBRARY_PATH in prefix.sh

[Martin Flöser]

graesslin accepted this revision.
graesslin added inline comments.

INLINE COMMENTS

> prefix.sh.cmake:3-4
> +
> +# LD_LIBRARY_PATH only needed if you are building without rpath
> +# export LD_LIBRARY_PATH=@KDE_INSTALL_FULL_LIBDIR@:$LD_LIBRARY_PATH
>  

Just a note: this would break for Plasma/Wayland anyway. kwin_wayland has 
capabilities and thus LD_LIBRARY_PATH gets unset and is not available to the 
session.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: apol, #frameworks, dfaure, ngraham, graesslin
Cc: graesslin, ngraham, #build_system

D9510: [server] Add a method IdleInterface::simulateUserActivity

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:4595a2c2709e: [server] Add a method 
IdleInterface::simulateUserActivity (authored by graesslin).
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D9510?vs=24380=24459#toc

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9510?vs=24380=24459

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

AFFECTED FILES
  autotests/client/test_idle.cpp
  src/server/idle_interface.cpp
  src/server/idle_interface.h

To: graesslin, #frameworks, #plasma, #kwin, davidedmundson
Cc: plasma-devel, leezu, ZrenBot, ngraham, alexeymin, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D9136: Add support for version 3 of data device manager interface

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:a715a6cd1fe3: Add support for version 3 of data device 
manager interface (authored by graesslin).
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D9136?vs=23347=24382#toc

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9136?vs=23347=24382

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

AFFECTED FILES
  autotests/client/test_drag_drop.cpp
  src/client/datadevice.cpp
  src/client/datadevicemanager.h
  src/client/dataoffer.cpp
  src/client/dataoffer.h
  src/client/datasource.cpp
  src/client/datasource.h
  src/client/registry.cpp
  src/server/datadevice_interface.cpp
  src/server/datadevicemanager_interface.cpp
  src/server/datadevicemanager_interface.h
  src/server/dataoffer_interface.cpp
  src/server/dataoffer_interface.h
  src/server/dataoffer_interface_p.h
  src/server/datasource_interface.cpp
  src/server/datasource_interface.h
  src/server/seat_interface.cpp

To: graesslin, #frameworks, #plasma, #kwin, davidedmundson
Cc: ngraham, broulik, plasma-devel, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D9510: [server] Add a method IdleInterface::simulateUserActivity

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: Frameworks, Plasma, KWin.
Restricted Application added projects: Plasma on Wayland, Frameworks.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  So far only the client was able to simulate user activity. This new
  method allows the server to also simulate user activity on all created
  idle timeouts. This is required by KWin to prevent idle timeouts when
  the user interacts through KDE Connect's virtual touchpad. In that
  situation the mouse pointer is used without updating the input time
  stamp as it doesn't come from "real" input devices and thus the idle
  timeout prevention is not activated.

REPOSITORY
  R127 KWayland

BRANCH
  idle-timeout-simulate-user-activity

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

AFFECTED FILES
  autotests/client/test_idle.cpp
  src/server/idle_interface.cpp
  src/server/idle_interface.h

To: graesslin, #frameworks, #plasma, #kwin
Cc: plasma-devel, leezu, ZrenBot, ngraham, alexeymin, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D9407: Scope exported/imported objects to the test

[Martin Flöser]

graesslin accepted this revision.
This revision is now accepted and ready to land.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, #plasma, graesslin
Cc: plasma-devel, #frameworks, leezu, ZrenBot, ngraham, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D9406: Fix ASAN warning on TestXdgForeign

[Martin Flöser]

graesslin accepted this revision.
This revision is now accepted and ready to land.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #plasma, graesslin
Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9402: Fix error in WaylandSurface::testDisconnect

[Martin Flöser]

graesslin accepted this revision.
This revision is now accepted and ready to land.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, #plasma, graesslin
Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9319: Fix ASAN warnings in XDGForeignV2Interface

[Martin Flöser]

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

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, graesslin
Cc: plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D9207: Don't load KDE platform module in kglobalaccel5

[Martin Flöser]

graesslin added a comment.


  In https://phabricator.kde.org/D9207#178257, @apol wrote:
  
  > In https://phabricator.kde.org/D9207#176322, @graesslin wrote:
  >
  > > I'm against a risky change here. Especially as this becomes irrelevant 
with Wayland.
  >
  >
  > What makes it risky?
  
  
  The description: "I'm not as sure on this..." and "but from code skimming I 
can't see anything that
  requires Plasma settings". So what I took from it is that it's unknown 
whether something might break. And that's what I consider risky. Especially in 
an application in frameworks which affects the stable Plasma releases. If 
kglobalaccel breaks all users are heavily affected. Due to that I'm extremely 
afraid of anything which could break.

REPOSITORY
  R268 KGlobalAccel

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

To: hein, #plasma, davidedmundson, mart
Cc: apol, graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D9136: Add support for version 3 of data device manager interface

[Martin Flöser]

graesslin added inline comments.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

INLINE COMMENTS

> broulik wrote in datadevicemanager.h:74
> Should `Ask` perhaps become a bigger number in  case stuff gets added in the 
> middle later like `Link` or is that unlikely/incompatible?

It would be incompatible. The values are the same as the flags from the wayland 
protocol

REPOSITORY
  R127 KWayland

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

To: graesslin, #frameworks, #plasma, #kwin
Cc: broulik, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9136: Add support for version 3 of data device manager interface

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.


  ping

REPOSITORY
  R127 KWayland

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

To: graesslin, #frameworks, #plasma, #kwin
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D8652: Add supported transformations to OutputDevice

[Martin Flöser]

graesslin added a comment.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.


  In https://phabricator.kde.org/D8652#176706, @davidedmundson wrote:
  
  > The comment about being static needs to be in the protocol xml too. Then 
this is good to go.
  >
  > ( though what's your reasoning for not just having the argument as a 
Wayland array ?)
  
  
  I didn't think of it. Just followed the existing way to announce modes.

REPOSITORY
  R127 KWayland

BRANCH
  supported-transformations

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

To: graesslin, #frameworks, #kwin, #plasma, sebas, davidedmundson
Cc: sebas, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, apol, mart

D9209: Don't set a window icon in Plasma::Dialog

[Martin Flöser]

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

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: hein, #plasma, #frameworks, davidedmundson, graesslin
Cc: broulik, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9207: Don't load KDE platform module in kglobalaccel5

[Martin Flöser]

graesslin added a comment.


  I'm against a risky change here. Especially as this becomes irrelevant with 
Wayland.

REPOSITORY
  R268 KGlobalAccel

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

To: hein, #plasma, davidedmundson, mart
Cc: graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D9136: Add support for version 3 of data device manager interface

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: Frameworks, Plasma, KWin.
Restricted Application added projects: Plasma on Wayland, Frameworks.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  The main difference compared to version 2 is additional drag and drop
  actions. The source and destination can negotiate whether the data
  should be copied or moved or the user should be asked for either or.
  This seems to be important for GTK, but is not yet implemented in Qt.
  
  The main motivation for adding support is that it is required by SDL to
  launch on Wayland.
  
  BUG: 386993

TEST PLAN
  Extended test case, sdl apps now start

REPOSITORY
  R127 KWayland

BRANCH
  data-device-version-3

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

AFFECTED FILES
  autotests/client/test_drag_drop.cpp
  src/client/datadevice.cpp
  src/client/datadevicemanager.h
  src/client/dataoffer.cpp
  src/client/dataoffer.h
  src/client/datasource.cpp
  src/client/datasource.h
  src/client/registry.cpp
  src/server/datadevice_interface.cpp
  src/server/datadevicemanager_interface.cpp
  src/server/datadevicemanager_interface.h
  src/server/dataoffer_interface.cpp
  src/server/dataoffer_interface.h
  src/server/dataoffer_interface_p.h
  src/server/datasource_interface.cpp
  src/server/datasource_interface.h
  src/server/seat_interface.cpp

To: graesslin, #frameworks, #plasma, #kwin
Cc: plasma-devel, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D9119: Initialize scale factor to the last scale factor set on any instance

[Martin Flöser]

graesslin added a comment.


  I agree with David that this is a hack. Also I think it will fail for multi 
screen with different dpi.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: hein, #plasma, davidedmundson, broulik, mart
Cc: graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D9104: Properly do strings in the kpackage framework

[Martin Flöser]

graesslin added a comment.


  There are a few unrelated changes, but otherwise looks good.

INLINE COMMENTS

> package.cpp:360
>  //Nested loop, but in the medium case resolves to just one iteration
> -//qDebug() << "prefixes:" << prefixes.count() << prefixes;
> +// qDebug() << "prefixes:" << d->contentsPrefixPaths.count() << 
> d->contentsPrefixPaths;
>  foreach (const QString , d->contentsPrefixPaths) {

?

> package.cpp:594
> +if (!it.value().endsWith(QLatin1Char('/'))) {
> +it.setValue(it.value() % QLatin1Char('/'));
>  }

Just curious: what does the modulo operator on a string?

> package.cpp:946
>  
> -if (isDir && QFile::exists(path + "/metadata.json")) {
> -metadata = new KPluginMetaData(path + "/metadata.json");
> -} else if (isDir && QFile::exists(path + "/metadata.desktop")) {
> -auto md = KPluginMetaData::fromDesktopFile(path + 
> "/metadata.desktop", 
> {QStringLiteral(":/kservicetypes5/kpackage-generic.desktop")});
> +delete metadata;
> +if (isDir && QFile::exists(path + QStringLiteral("/metadata.json"))) {

This looks like a unrelated move of the line.

> kpackagetool.cpp:437
>  // This can happen in the case an application wanting to support 
> kpackage based extensions includes in the same project both the 
> packagestructure plugin and the packages themselves. In that case at build 
> time the packagestructure plugin wouldn't be installed yet
> +
>  if (QFile::exists(pluginName + QStringLiteral("/metadata.desktop"))) {

unrelated?

REPOSITORY
  R290 KPackage

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

To: apol, #frameworks, #plasma, davidedmundson
Cc: graesslin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9092: Group some blocking dbus calls

[Martin Flöser]

graesslin accepted this revision.

REPOSITORY
  R268 KGlobalAccel

BRANCH
  master

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

To: apol, #frameworks, #plasma, davidedmundson, graesslin
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D9086: [KWindowEffectsPrivateX11] Add reserve() call

[Martin Flöser]

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

REPOSITORY
  R278 KWindowSystem

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

To: broulik, #plasma, graesslin
Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9048: [kglobalaccel] Drop QWidget dependency

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in CMakeLists.txt:36
> A depend has widgets on a public interface, i guess

If KGlogalAccel needs Widgets, Widgets must stay.

REPOSITORY
  R268 KGlobalAccel

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

To: anthonyfieroni, davidedmundson, apol
Cc: graesslin, broulik, #frameworks

D8919: Add explicit AppMenu protocol

[Martin Flöser]

graesslin accepted this revision.
This revision is now accepted and ready to land.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: davidedmundson, #plasma, graesslin
Cc: broulik, graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D8858: Fix testWaylandFullscreenShell.

[Martin Flöser]

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

REPOSITORY
  R127 KWayland

BRANCH
  master

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

To: cgiboudeaux, graesslin
Cc: plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8919: Add explicit AppMenu protocol

[Martin Flöser]

graesslin added inline comments.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.

INLINE COMMENTS

> appmenu.xml:37-38
> +
> +  
> +  
> +  

Could you please add "The string must be encoded in latin-1".

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #plasma
Cc: broulik, graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D8998: Add FindSeccomp to find-modules

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> davidk wrote in FindSeccomp.cmake:50-55
> I'm not sure about this. @graesslin  is this nessecarry? Most find-modules 
> don't check for the CMake version, and we don't do anything special here.

No idea, that's copy pasted from some other cmake modules.

REPOSITORY
  R240 Extra CMake Modules

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

To: davidk, graesslin
Cc: #frameworks, #build_system

D8919: Add explicit AppMenu protocol

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in appmenu.cpp:178
> Wayland strings can be UTF-8 but DBus service names and object paths can only 
> be ASCII.
> 
> I figured it was best to avoid any potential locale issues.

Fair enough. I wasn't aware of the DBus restriction. I suggest to add it to the 
protocol definition. E.g. in Wayland protocols we find: "The string must be 
encoded in UTF-8. " So I suggest to do something similar, just to be sure.

> appmenu.xml:35-38
> +  
> +  
> +  
> +  

I just noticed that this is missing documentation. The wayland protocol 
generator is not happy if the documentation is missing and emits warnings.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #plasma
Cc: broulik, graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, 
apol, mart, hein

D8919: Add explicit AppMenu protocol

[Martin Flöser]

graesslin added a comment.


  Looks good, just a few minor comments.

INLINE COMMENTS

> appmenu.cpp:178
> +Q_ASSERT(isValid());
> +org_kde_kwin_appmenu_set_address(d->appmenu, serviceName.toLatin1(), 
> objectPath.toLatin1());
> +}

why toLatin1? I would expect a toUtf8?

> appmenu.xml:6-17
> +This program is free software: you can redistribute it and/or modify
> +it under the terms of the GNU Lesser General Public License as published 
> by
> +the Free Software Foundation, either version 2.1 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of

random suggestion: use a wayland-protocols compliant license right from the 
start? Just in case it ever goes upstream...

> registry.h:585
> + * @see createAppMenuManager
> + * @since 5.5
> + **/

5.XX

> registry.h:1473
> + * @param name The name of the removed interface
> + * @since 5.41
> + **/

5.XX, though I think you don't need to change it. I'm quite certain we make 5.41

> appmenu_interface.h:53
> + */
> +AppMenuInterface* appMenuForSurface(SurfaceInterface *);
> +

a conceptional alternative could be to delegate this through the 
SurfaceInterface like how it was done for idleInhibit protocol. I'm fine with 
both approaches.

> appmenu_interface.h:78-81
> +struct InterfaceAddress {
> +QString serviceName;
> +QString objectPath;
> +};

Please add some documentation.

> appmenu_interface.h:95
> +Q_SIGNALS:
> +void 
> addressChanged(KWayland::Server::AppMenuInterface::InterfaceAddress);
> +

documentation missing.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #plasma
Cc: graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8858: Fix testWaylandFullscreenShell.

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.


  Could you please add a message that it is missing?

REPOSITORY
  R127 KWayland

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

To: cgiboudeaux, graesslin
Cc: plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8396: Add support for zwp_idle_inhibit_manager_v1

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:9520c2f292d0: Add support for zwp_idle_inhibit_manager_v1 
(authored by graesslin).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D8396?vs=21046=22482#toc

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8396?vs=21046=22482

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

AFFECTED FILES
  autotests/client/test_wayland_registry.cpp
  autotests/client/test_wayland_surface.cpp
  src/client/CMakeLists.txt
  src/client/idleinhibit.cpp
  src/client/idleinhibit.h
  src/client/protocols/idle-inhibit-unstable-v1.xml
  src/client/registry.cpp
  src/client/registry.h
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/display.h
  src/server/idleinhibit_interface.cpp
  src/server/idleinhibit_interface.h
  src/server/idleinhibit_interface_p.h
  src/server/idleinhibit_interface_v1.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h
  src/server/surface_interface_p.h
  src/tools/mapping.txt

To: graesslin, #frameworks, #kwin, #plasma_on_wayland, davidedmundson
Cc: davidedmundson, plasma-devel, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8396: Add support for zwp_idle_inhibit_manager_v1

[Martin Flöser]

graesslin added a comment.


  In https://phabricator.kde.org/D8396#168165, @davidedmundson wrote:
  
  > I think we need some big discussion about how powerdevil inhbitions, logind 
inhibitions and this are all going to fit together in a clear coherent way.
  
  
  yeah. I'm not happy with this protocol. I think it's a step backwards as we 
are again in a situation where an application can just block everything. And we 
don't have any context information available on why it's blocking.
  
  The only good thing is that KWin is allowed to restrict it to when the window 
is visible (and that's something I want to implement). And we could probably 
pass an inhibition to Powerdevil informing it that "Window Foo inhibits power 
management".
  
  > But given there are clients using this protocol already, I guess we need 
this regardless certainly at a protocol level.
  
  That was the reason why I implemented it. When the protocol was added to 
wayland-protocols I didn't consider it for implementation as I thought it's 
useless. But if apps use it and don't use the dbus protocol any more...

REPOSITORY
  R127 KWayland

BRANCH
  idle-inhibit-manager

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

To: graesslin, #frameworks, #kwin, #plasma_on_wayland, davidedmundson
Cc: davidedmundson, plasma-devel, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8383: [server] Support inhibiting the IdleInterface

[Martin Flöser]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:42b2daaec407: [server] Support inhibiting the 
IdleInterface (authored by graesslin).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D8383?vs=21016=22363#toc

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8383?vs=21016=22363

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

AFFECTED FILES
  autotests/client/test_idle.cpp
  src/server/idle_interface.cpp
  src/server/idle_interface.h

To: graesslin, #frameworks, #kwin, #plasma_on_wayland, davidedmundson
Cc: plasma-devel, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8396: Add support for zwp_idle_inhibit_manager_v1

[Martin Flöser]

graesslin added a comment.


  ping

REPOSITORY
  R127 KWayland

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

To: graesslin, #frameworks, #kwin, #plasma_on_wayland
Cc: plasma-devel, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8705: Remove Qt5::Widgets as a dependency

[Martin Flöser]

graesslin added a comment.


  In https://phabricator.kde.org/D8705#166045, @dfaure wrote:
  
  > I guess that the same argument could be made about Qt5::Widgets... if 
you're using a widget you're supposed to link to that, rather than indirectly 
via KWindowSystem...
  >
  > This is making me change my mind. Maybe it's OK after all. It breaks SC in 
a case where "proper" application code shouldn't be affected.
  
  
  I'm not totally happy with this as @apol showed with the list of reviews he 
already put up that this causes compile breakage for many products. I don't 
mind getting rid of QWidget in KWindowSystem - in fact I would even prefer if 
we could get rid of it completely and was very sad when I realized we missed 
the 5.0 slot for it. But I think we shouldn't just break existing software. 
Especially given the short release cycles we have and the encouragement to 
distros to ship such versions. It would not be good for us if distros cannot 
rely on our own code to compile with latest frameworks.
  
  Thus I suggest we do this in two steps: introduce a build option which 
enforces the old behavior and keep this the default. In half a year change the 
default and with KF 6 remove it completely. This just gives the whole stack 
more time to catch up and we hopefully don't run into the situations that 
things break. Oh and kdesrc-build should be adjusted to enable the build option 
from the start, so that our own code falls on our feet.

REPOSITORY
  R278 KWindowSystem

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

To: apol, #frameworks, dfaure
Cc: graesslin, dfaure, anthonyfieroni

D8705: Remove Qt5::Widgets as a dependency

[Martin Flöser]

graesslin added subscribers: dfaure, graesslin.
graesslin added a comment.


  From my side a -1. This is not source compatible. Thus I think we need to 
wait for Frameworks 6 for it.
  
  I want @dfaure to ack/nack this change.

REPOSITORY
  R278 KWindowSystem

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

To: apol, #frameworks
Cc: graesslin, dfaure, anthonyfieroni

D8652: Add supported transformations to OutputDevice

[Martin Flöser]

graesslin updated this revision to Diff 21872.
graesslin added a comment.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.


  enum and uint

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8652?vs=21869=21872

BRANCH
  supported-transformations

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

AFFECTED FILES
  autotests/client/test_wayland_outputdevice.cpp
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/client/registry.cpp
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: graesslin, #frameworks, #kwin, #plasma
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D8652: Add supported transformations to OutputDevice

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in outputdevice_interface.cpp:470
> we have the initial send which is all fine
> 
> Then you've got this dynamically add them.
> But at a protocol level we have no way to remove and replace.
> That's somewhat broken.
> 
> Personally, I'd just make it a static and document that it must be sent 
> before the first done()

I'm not 100 % sure whether we can in KWin determine the supported rotations in 
all cases before the OutputDeviceInterface is fully setup. Thus I wanted to 
have a little bit of flexibility here.

Also if we need it we can add a remove. If we go for a static approach, we 
cannot remove again.

REPOSITORY
  R127 KWayland

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

To: graesslin, #frameworks, #kwin, #plasma
Cc: davidedmundson, plasma-devel, leezu, ZrenBot, alexeymin, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8652: Add supported transformations to OutputDevice

[Martin Flöser]

graesslin created this revision.
graesslin added reviewers: Frameworks, KWin, Plasma.
Restricted Application added projects: Plasma on Wayland, Frameworks.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  The server can announce all supported transformations on an OutputDevice.
  The rational for this change is a requirement recognized in KWin in
  combination with KScreen: KScreen should not offer to transform a screen
  in a way which KWin cannot support. This API can fill the need.

REPOSITORY
  R127 KWayland

BRANCH
  supported-transformations

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

AFFECTED FILES
  autotests/client/test_wayland_outputdevice.cpp
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/client/registry.cpp
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: graesslin, #frameworks, #kwin, #plasma
Cc: plasma-devel, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8383: [server] Support inhibiting the IdleInterface

[Martin Flöser]

graesslin added a comment.


  ping

REPOSITORY
  R127 KWayland

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

To: graesslin, #frameworks, #kwin, #plasma_on_wayland
Cc: plasma-devel, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8396: Add support for zwp_idle_inhibit_manager_v1

[Martin Flöser]

graesslin added a comment.


  ping

REPOSITORY
  R127 KWayland

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

To: graesslin, #frameworks, #kwin, #plasma_on_wayland
Cc: plasma-devel, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D8519: do not make the context menu a Window; do not force raise a window

[Martin Flöser]

graesslin added a comment.


  Maybe we can drop the force activate for all hosts? Are there any 
implementations left which are on X11? At least Ubuntu is Wayland based.

REPOSITORY
  R289 KNotifications

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

To: mkoller, davidedmundson, graesslin
Cc: #frameworks