[Differential] [Commented On] D3085: RFC: Use DBusMenu if available

[dmitryshachnev (Dmitry Shachnev)]

dmitryshachnev added a comment.


  > in KXMLGui applications you still get a traditional menu bar in addition to 
the global menu, it seems it explicitly sets visible true on the menu bar - in 
VLC it hides its own menu bar.
  
  This will be fixed in Qt 5.7.1, see this commit 
.

REPOSITORY
  rPLASMAINTEGRATION Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: dmitryshachnev, graesslin, plasma-devel, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas

[Differential] [Commented On] D3085: RFC: Use DBusMenu if available

[Martin Gräßlin]

graesslin added inline comments.

INLINE COMMENTS

> CMakeLists.txt:10-13
> +  find_package(XCB COMPONENTS XCB)
> +  set_package_properties(XCB PROPERTIES DESCRIPTION "Required for exposing 
> the global menu on X11"
> + TYPE REQUIRED
> +)

HAVE_X11 is just trying to find XLib. There is no need to bind find XCB to find 
XLib. You can just move it out of the if.

Also the HAVE_X11 is I think wrong, but that's unrelated ;-)

> CMakeLists.txt:62
>  if(HAVE_X11)
> -  target_link_libraries(KDEPlasmaPlatformTheme PRIVATE Qt5::X11Extras 
> ${X11_Xcursor_LIB})
> +  target_link_libraries(KDEPlasmaPlatformTheme PRIVATE Qt5::X11Extras 
> ${X11_Xcursor_LIB} ${XCB_XCB_LIBRARY})
>  endif()

XCB::XCB

> kdeplatformtheme.cpp:115
> +case QEvent::ApplicationStateChange:
> +// FIXME isn't actually triggered when engaging Alt+left-click 
> window moving
> +m_timer->stop();

Maybe FocusOut delivers it?

> x11integration.cpp:59-60
> +
> +const xcb_intern_atom_cookie_t cookie = xcb_intern_atom(c, false, 
> name.length(), name.constData());
> +QScopedPointer 
> reply(xcb_intern_atom_reply(c, cookie, Q_NULLPTR));
> +if (!reply.isNull()) {

can we cache the atom? That's causing a roundtrip every time it's invoked.

REPOSITORY
  rPLASMAINTEGRATION Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Commented On] D3085: RFC: Use DBusMenu if available

[Martin Gräßlin]

graesslin added a comment.


  Trying to apply the patch results in conflicts in autotests/CMakeLists.txt
  
  Trying to compile nevertheless without autotests gives me a compile error:
  

/home/martin/src/kf5/kde/workspace/plasma-integration/src/platformtheme/kdeplatformtheme.cpp:47:36:
 fatal error: private/qdbusmenubar_p.h: No such file or directory
 #include 
^
compilation terminated.
src/platformtheme/CMakeFiles/KDEPlasmaPlatformTheme.dir/build.make:62: 
recipe for target 
'src/platformtheme/CMakeFiles/KDEPlasmaPlatformTheme.dir/kdeplatformtheme.cpp.o'
 failed
make[2]: *** 
[src/platformtheme/CMakeFiles/KDEPlasmaPlatformTheme.dir/kdeplatformtheme.cpp.o]
 Error 1
CMakeFiles/Makefile2:136: recipe for target 
'src/platformtheme/CMakeFiles/KDEPlasmaPlatformTheme.dir/all' failed

REPOSITORY
  rPLASMAINTEGRATION Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Commented On] D3085: RFC: Use DBusMenu if available

[Martin Gräßlin]

graesslin added a comment.


  In https://phabricator.kde.org/D3085#57496, @broulik wrote:
  
  > To expose the menu id and object path as X/Wayland properties on the window 
I would need to implement QDBusMenuBar myself as I need to implement 
handleReparent as well as access m_objectPath which is private. I'll see how it 
goes.
  
  
  I think you can hack it without needing to implement. We know that it's 
exported on the DBusConnection and we know what the object is looking like. I'm 
sure we can get to it without needing to reimplement.

REPOSITORY
  rPLASMAINTEGRATION Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Commented On] D3085: RFC: Use DBusMenu if available

[broulik (Kai Uwe Broulik)]

broulik added a comment.


  To expose the menu id and object path as X/Wayland properties on the window I 
would need to implement QDBusMenuBar myself as I need to implement 
handleReparent as well as access m_objectPath which is private. I'll see how it 
goes.

REPOSITORY
  rPLASMAINTEGRATION Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Commented On] D3085: RFC: Use DBusMenu if available

[Martin Gräßlin]

graesslin added inline comments.

INLINE COMMENTS

> broulik wrote in kdeplatformtheme.cpp:59
> That's what Qt does and I don't think Qt is equipped to re-create a platform 
> menu at runtime; at least that's what I was told when I raised the very same 
> concern in the upstream Qt codereview that added this.
> 
> We could perhaps do it for newly created menus but when the service becomes 
> unavailable at runtime there's no way for us to re-create existing menus.

hmm then we need to make sure that changing the option cannot be done at 
runtime - it has to require a session restart.

REPOSITORY
  rPLASMAINTEGRATION Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Commented On] D3085: RFC: Use DBusMenu if available

[broulik (Kai Uwe Broulik)]

broulik added a comment.


  It's not much code on our side because Qt has the code nowadays. :)

INLINE COMMENTS

> graesslin wrote in FindQt5PlatformSupport.cmake:1
> given that it has my copyright I assume you copied from kwin - that would be 
> a reason to move it to ecm. We have two users.

+1 to that.

I had one issue regarding the include path, though, but I can't reproduce it 
anymore and I forgot what I actually did but the path I need is

  /usr/include/x86_64-linux-gnu/qt5/QtPlatformSupport/5.7.0/QtPlatformSupport/

but I sometimes got

  /usr/include/x86_64-linux-gnu/qt5/QtPlatformSupport/5.7.0/

but I'm sure we can figur that out.

> graesslin wrote in kdeplatformtheme.cpp:59
> can we really make that static? What about runtime changes?

That's what Qt does and I don't think Qt is equipped to re-create a platform 
menu at runtime; at least that's what I was told when I raised the very same 
concern in the upstream Qt codereview that added this.

We could perhaps do it for newly created menus but when the service becomes 
unavailable at runtime there's no way for us to re-create existing menus.

REPOSITORY
  rPLASMAINTEGRATION Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Commented On] D3085: RFC: Use DBusMenu if available

[Martin Gräßlin]

graesslin added a comment.


  Overall significant less code than I expected - nice :-)

INLINE COMMENTS

> CMakeLists.txt:46
>  
> +find_package(Qt5PlatformSupport REQUIRED)
> +

version 5.7?

> FindQt5PlatformSupport.cmake:1
> +#.rst:
> +# FindQt5PlatformSupport

given that it has my copyright I assume you copied from kwin - that would be a 
reason to move it to ecm. We have two users.

> kdeplatformtheme.cpp:59
> +{
> +static bool dbusGlobalMenuAvailable = checkDBusGlobalMenuAvailable();
> +return dbusGlobalMenuAvailable;

can we really make that static? What about runtime changes?

> kdeplatformtheme.cpp:310
> +{
> +if (isDBusGlobalMenuAvailable()) {
> +return new QDBusMenuBar();

what happens if it becomes unavailable at runtime?

REPOSITORY
  rPLASMAINTEGRATION Integration for Qt applications in Plasma

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: graesslin, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas