D6047: WIP: Support XDG v6

[David Edmundson]

davidedmundson added a comment.


  Update from IRC chat:
  It was BIC from when I renamed that signal.  All resolved.

REPOSITORY
  R127 KWayland

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

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

D6047: WIP: Support XDG v6

[Marco Martin]

mart added a comment.


  right now, the xdgv6 branch (xdgv6popup actually) for me causes a crash in 
kwin at startup, with the following BT:
  #0  _int_malloc (av=av@entry=0x7556ab20 , 
bytes=bytes@entry=72) at malloc.c:3414
  #1  0x7522a5d4 in __GI___libc_malloc (bytes=72) at malloc.c:2911
  #2  0x755fde78 in operator new(unsigned long) ()
  
from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
  
  #3  0x75dcc32e in QObjectPrivate::connectImpl 
(sender=sender@entry=0x69d950,
  
signal_index=5, receiver=receiver@entry=0x69c600, slot=slot@entry=0x0, 
slotObj=slotObj@entry=0x69c860, type=Qt::AutoConnection, types=0x0, 
senderMetaObject=0x7720c220 
)
at kernel/qobject.cpp:4829
  
  #4  0x75dcc69b in QObject::connectImpl (sender=0x69d950, 
signal=,
  
receiver=0x69c600, slot=0x0, slotObj=0x69c860, type=Qt::AutoConnection, 
types=0x0, 
senderMetaObject=0x7720c220 
)
at kernel/qobject.cpp:4784
  
  #5  0x76f1f249 in QObject::connect >(const 
QtPrivate::FunctionPointer::Object *, 
void (KWayland::Server::Display::*)(KWayland::Server::Display * const), const 
QObject *, KWayland::Server::Display::, Qt::ConnectionType) 
(sender=0x69d950,
  
signal=(void (KWayland::Server::Display::*)(KWayland::Server::Display * 
const)) 0x76fa67de , 
context=0x69c600, slot=..., 
type=Qt::AutoConnection) at /opt/kde5/include/QtCore/qobject.h:338

REPOSITORY
  R127 KWayland

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

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

D6047: WIP: Support XDG v6

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D6047#119765, @graesslin wrote:
  
  > In https://phabricator.kde.org/D6047#119698, @mart wrote:
  >
  > > trying to implement the kwin part of ping btw, will do in a branch of the 
branch kwin/xdgv6
  >
  >
  > no need to hurry with it. IMHO the way ping works is completely broken and 
won't work. At least Qt holds the connection in a thread, so it will react to 
the ping even if the application is frozen in the main gui thread.
  
  
  any way to change this/make the gui thread actually answer?
  
  > Also please note that ping needs to be different in Wayland compared to 
X11. In X11 we only ping when trying to close the window. In Wayland we should 
ping whenever we interact with the window in some way. E.g. when we pass 
pointer focus, when we pass keyboard focus, maybe even when sending in input 
events.
  
  if it's done in all input events, couldn't this affect performance?

REPOSITORY
  R127 KWayland

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

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

D6047: WIP: Support XDG v6

[Martin Flöser]

graesslin added a comment.


  In https://phabricator.kde.org/D6047#119698, @mart wrote:
  
  > trying to implement the kwin part of ping btw, will do in a branch of the 
branch kwin/xdgv6
  
  
  no need to hurry with it. IMHO the way ping works is completely broken and 
won't work. At least Qt holds the connection in a thread, so it will react to 
the ping even if the application is frozen in the main gui thread.
  
  Also please note that ping needs to be different in Wayland compared to X11. 
In X11 we only ping when trying to close the window. In Wayland we should ping 
whenever we interact with the window in some way. E.g. when we pass pointer 
focus, when we pass keyboard focus, maybe even when sending in input events.

REPOSITORY
  R127 KWayland

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

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

D6047: WIP: Support XDG v6

[David Edmundson]

davidedmundson marked 2 inline comments as done.
davidedmundson added a comment.


  Done all comments except ping/pong.
  Marco, ping me if you ever want this updating.

REPOSITORY
  R127 KWayland

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

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

D6047: WIP: Support XDG v6

[David Edmundson]

davidedmundson updated this revision to Diff 15889.
davidedmundson marked 2 inline comments as done.
davidedmundson added a comment.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.


  - cleanups

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6047?vs=15797=15889

BRANCH
  davidedmundson/xdgv6popup

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

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_xdg_shell.cpp
  autotests/client/test_xdg_shell.h
  autotests/client/test_xdg_shell_v5.cpp
  autotests/client/test_xdg_shell_v6.cpp
  src/client/CMakeLists.txt
  src/client/protocols/xdg-shell-unstable-v6.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/xdgshell.cpp
  src/client/xdgshell.h
  src/client/xdgshell_p.h
  src/client/xdgshell_v5.cpp
  src/client/xdgshell_v6.cpp
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/xdgshell_interface.cpp
  src/server/xdgshell_interface.h
  src/server/xdgshell_interface_p.h
  src/server/xdgshell_v5_interface.cpp
  src/server/xdgshell_v5_interface_p.h
  src/server/xdgshell_v6_interface.cpp
  src/server/xdgshell_v6_interface_p.h
  src/tools/mapping.txt
  tests/CMakeLists.txt
  tests/xdgtest.cpp

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

D6047: WIP: Support XDG v6

[Marco Martin]

mart added a comment.


  trying to implement the kwin part of ping btw, will do in a branch of the 
branch kwin/xdgv6

REPOSITORY
  R127 KWayland

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

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

D6047: WIP: Support XDG v6

[David Edmundson]

davidedmundson updated this revision to Diff 15797.
davidedmundson edited the summary of this revision.
davidedmundson added a comment.
Restricted Application edited projects, added Plasma on Wayland; removed Plasma.


  Try to upload via arc...

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6047?vs=15634=15797

BRANCH
  xdg_merge

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

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_xdg_shell.cpp
  autotests/client/test_xdg_shell.h
  autotests/client/test_xdg_shell_v5.cpp
  autotests/client/test_xdg_shell_v6.cpp
  src/client/CMakeLists.txt
  src/client/protocols/xdg-shell-unstable-v6.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/xdgshell.cpp
  src/client/xdgshell.h
  src/client/xdgshell_p.h
  src/client/xdgshell_v5.cpp
  src/client/xdgshell_v6.cpp
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/xdgshell_interface.cpp
  src/server/xdgshell_interface.h
  src/server/xdgshell_interface_p.h
  src/server/xdgshell_v5_interface.cpp
  src/server/xdgshell_v5_interface_p.h
  src/server/xdgshell_v6_interface.cpp
  src/server/xdgshell_v6_interface_p.h
  src/tools/mapping.txt
  tests/CMakeLists.txt
  tests/xdgtest.cpp

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

D6047: WIP: Support XDG v6

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> graesslin wrote in xdgshell_interface.h:80
> @davidedmundson  for the ping you could check the old wl_shell_surface 
> implementation in KWayland.

the implementation here in xdgshell_interface is basically copied over the 
kwayland  wl_shell_surface.

the code pasted above, is the only kwin part i found that was managing pings in 
any way, which is the x11 implementation (Client)
i guess ShellClient will have to have something along the lines using the 
kwayland implementations, tough casing between the possible interfaces exposed 
(wl_shell, xdgshellv5 and xdgshellv6)

> graesslin wrote in xdgshell_interface.h:75
> what is "gravity"?

as far i understood, it's an hint that tells the window the direction it 
prefers to popup, so like "please dear compositor make it show below the point 
i asked to", or at the left, or whatever
hint that the compositor can choose to ignore

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

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

D6047: WIP: Support XDG v6

[Martin Flöser]

graesslin added a comment.


  could you please upload a version with context?

INLINE COMMENTS

> mart wrote in xdgshell_interface.h:80
> from kwin, client.cpp:
> i guess that's the kwin part that will have to use this.
> it uses a single timer, and tries for a couple of timeout, as soon as the 
> client answers it's considered good, no serial for pings is considered.
> 
> void Client::pingWindow()
> {
> 
>   if (!info->supportsProtocol(NET::PingProtocol))
>   return; // Can't ping :(
>   if (options->killPingTimeout() == 0)
>   return; // Turned off
>   if (ping_timer != NULL)
>   return; // Pinging already
>   ping_timer = new QTimer(this);
>   connect(ping_timer, ::timeout, this,
>   [this]() {
>   if (unresponsive()) {
>   qCDebug(KWIN_CORE) << "Final ping timeout, asking to kill:" << 
> caption();
>   ping_timer->deleteLater();
>   ping_timer = nullptr;
>   killProcess(true, m_pingTimestamp);
>   return;
>   }
>   
>   qCDebug(KWIN_CORE) << "First ping timeout:" << caption();
>   
>   setUnresponsive(true);
>   ping_timer->start();
>   }
>   );

please note that this is X11 code which has a different ping concept. On 
Wayland we don't ping yet.

> davidedmundson wrote in xdgshell_interface.h:80
> We need the serial Ids here, otherwise it's not very usable; especially as 
> the pong doesn't have an elapsed time.
> 
> A kjob like API wrapping this might be perfect for here?

@davidedmundson  for the ping you could check the old wl_shell_surface 
implementation in KWayland.

> xdgshell_interface.h:75
> +/*
> + * Invert the anchor and gravity on the X axis
> + */

what is "gravity"?

> xdgshell_interface.h:124
>   *
> + * @deprecated
>   * @param surface The popup xdg shell surface which got created

please add what the replacement is

> xdgshell_interface.h:142
> + */
> +void popupCreated2(KWayland::Server::XdgShellPopupInterface *surface);
> +

can we have a better name than popupCreated2?

> xdgshell_interface.h:145
> +
> +void pongReceived();
> +

documentation missing

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

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

D6047: WIP: Support XDG v6

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> mart wrote in xdgshell_interface.h:80
> so storing all the ids of pings in progress somewhere to make a more recent 
> ping not cancel an older one still pending?

from kwin, client.cpp:
i guess that's the kwin part that will have to use this.
it uses a single timer, and tries for a couple of timeout, as soon as the 
client answers it's considered good, no serial for pings is considered.

void Client::pingWindow()
{

  if (!info->supportsProtocol(NET::PingProtocol))
  return; // Can't ping :(
  if (options->killPingTimeout() == 0)
  return; // Turned off
  if (ping_timer != NULL)
  return; // Pinging already
  ping_timer = new QTimer(this);
  connect(ping_timer, ::timeout, this,
  [this]() {
  if (unresponsive()) {
  qCDebug(KWIN_CORE) << "Final ping timeout, asking to kill:" << 
caption();
  ping_timer->deleteLater();
  ping_timer = nullptr;
  killProcess(true, m_pingTimestamp);
  return;
  }
  
  qCDebug(KWIN_CORE) << "First ping timeout:" << caption();
  
  setUnresponsive(true);
  ping_timer->start();
  }
  );

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

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

D6047: WIP: Support XDG v6

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> davidedmundson wrote in xdgshell_interface.h:80
> We need the serial Ids here, otherwise it's not very usable; especially as 
> the pong doesn't have an elapsed time.
> 
> A kjob like API wrapping this might be perfect for here?

so storing all the ids of pings in progress somewhere to make a more recent 
ping not cancel an older one still pending?

> davidedmundson wrote in xdgshell_interface_p.h:41
> I don't understand this timer, all it's used for is for making us not emit a 
> pong if it comes in after a timeout?

send ping, if a pong doesn't arrive after a timeout, consider it dead.. I 
copied it as-is from wl_shell iirc, so assumed this would be the expected 
behavior.

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

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

D6047: WIP: Support XDG v6

[David Edmundson]

davidedmundson added a comment.


  Comments for Marco:

INLINE COMMENTS

> xdgshell_interface.h:80
>  
> +void ping();
> +

We need the serial Ids here, otherwise it's not very usable; especially as the 
pong doesn't have an elapsed time.

A kjob like API wrapping this might be perfect for here?

> xdgshell_interface_p.h:41
> +quint32 pingSerial = 0;
> +QScopedPointer pingTimer;
> +

I don't understand this timer, all it's used for is for making us not emit a 
pong if it comes in after a timeout?

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

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

D6047: WIP: Support XDG v6

[David Edmundson]

davidedmundson updated this revision to Diff 15020.
davidedmundson added a comment.


  Arc didn't work, I think it was too big. 
  Manually uploading the patch did.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6047?vs=15013=15020

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

AFFECTED FILES
  autotests/client/CMakeLists.txt
  autotests/client/test_xdg_shell.cpp
  autotests/client/test_xdg_shell_v5.cpp
  autotests/client/test_xdg_shell_v6.cpp
  src/client/CMakeLists.txt
  src/client/protocols/xdg-shell-unstable-v6.xml
  src/client/registry.cpp
  src/client/registry.h
  src/client/xdgshell.cpp
  src/client/xdgshell.h
  src/client/xdgshell_p.h
  src/client/xdgshell_v5.cpp
  src/client/xdgshell_v6.cpp
  src/server/CMakeLists.txt
  src/server/display.cpp
  src/server/xdgshell_interface.cpp
  src/server/xdgshell_interface.h
  src/server/xdgshell_interface_p.h
  src/server/xdgshell_v5_interface.cpp
  src/server/xdgshell_v5_interface_p.h
  src/server/xdgshell_v6_interface.cpp
  src/server/xdgshell_v6_interface_p.h
  src/tools/mapping.txt

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

D6047: WIP: Support XDG v6

[David Edmundson]

davidedmundson abandoned this revision.
davidedmundson added a comment.


  Flipping phabricator...

REPOSITORY
  R127 KWayland

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

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

D6047: WIP: Support XDG v6

[David Edmundson]

davidedmundson created this revision.
davidedmundson added projects: Plasma on Wayland, Frameworks.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland.

REVISION SUMMARY
  To move forward faster I thought we could start reviewing the 
davidedmundson/xdgv6 branch for the top level
  whilst I continue the Popup stuff in a new branch.
  
  Commits are made by both me and Marco. 
  He did pings + min/max sizes I did most most the rest.
  
  The main clever part that's not just boring boiler plate is how we handle the 
structure change
  A surface now has an XDGSurface which then has a an Xdg TopLevel or a Xdg 
Popup
  
  We need to fit this into the public API which assumes a surface has a Surface 
or a Popup.
  The old Surface is similar to the new TopLevel.
  
  The shoehorning works by relying on the fact that a surface without a role is 
pretty useless.
  
  Clients create the surface implicitly with the toplevel or implicitly with 
the popup.
  The server only announced it has a new "XdgSurface" when it gets a new 
zxdg_surface_get_toplevel.
  
  Don't write comments about popup code not being implemented...

TEST PLAN
  Current test still passes.

REPOSITORY
  R127 KWayland

BRANCH
  davidedmundson/xdgv6

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

AFFECTED FILES
  autotests/client/test_xdg_shell_v6.cpp

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