Add appmenu support to kwin

Review Request #104344 - Created March 19, 2012 and submitted

Information
Cedric Bellegarde
kde-workspace
Reviewers
kwin
Here a patch to add appmenu support to kwin via a menu button in titlebar

Works by Lionel Chauvin and me, original review request: https://git.reviewboard.kde.org/r/101162/

It depends on:
http://quickgit.kde.org/index.php?p=kded-appmenu.git

It looks like this:
https://www.youtube.com/watch?v=x1bm7Q6_SH4&hd=1

New version, all drawing code moved to kded-appmenu as asked.

  

Issues

  • 21
  • 54
  • 0
  • 75
Description From Last Updated
Do you think this is the perfect place for it? I'm not sure, but would say it belongs more into ... Martin Gräßlin Martin Gräßlin
menubar.cpp is probably also only needed if KWIN_BUILD_KAPPMENU is true. So best wrap it in an iff block like done ... Martin Gräßlin Martin Gräßlin
I would suggest to have the API always including the two methods even if the define is not set. Martin Gräßlin Martin Gräßlin
*cough* :-) Martin Gräßlin Martin Gräßlin
same as in bridge.h Martin Gräßlin Martin Gräßlin
In the library I think we should not have the ifdefs. That might cause ABI issues. Actually the library does ... Martin Gräßlin Martin Gräßlin
whitespaces and coding style Martin Gräßlin Martin Gräßlin
that does not sound very Qt: getters do not start with get :-) If it is really a window ID ... Martin Gräßlin Martin Gräßlin
? Martin Gräßlin Martin Gräßlin
I think here in the options we don't need all the ifdefs. Look above what has been done for the ... Martin Gräßlin Martin Gräßlin
you don't need to include config-kwin.h here. It gets include later on by the include of utils.h Martin Gräßlin Martin Gräßlin
is the wid really a window id? In that case you should use a WId type or at least qulonglong. ... Martin Gräßlin Martin Gräßlin
can we have m_variableName for new things? Yes I know it's inconsistant but we should try to be consistant for ... Martin Gräßlin Martin Gräßlin
please use NULL instead of 0. When we are using C++11 I plan to do a global replace of NULL ... Martin Gräßlin Martin Gräßlin
if (!applicationMenuImporter) Martin Gräßlin Martin Gräßlin
if (!menubar) Martin Gräßlin Martin Gräßlin
the second part of this else if (null check) is not needed. It is save to delete a null pointer. ... Martin Gräßlin Martin Gräßlin
why is this needed at all? options is available to everything of KWin, so no need to wrap it in ... Martin Gräßlin Martin Gräßlin
somehow I don't like this. Why not emitting a signal in workspace and have in client a connect from signal ... Martin Gräßlin Martin Gräßlin
In addition: this looks like it performs a synchronous dbus call, is that correct? Thomas Lübking Thomas Lübking
why this? just emit in the code - also the function name does not match conventions (sounds like a getter) Thomas Lübking Thomas Lübking
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Martin Gräßlin
Cedric Bellegarde
Martin Gräßlin
Hugo Pereira Da Costa
Hugo Pereira Da Costa
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Giorgos Tsiapaliokas
Giorgos Tsiapaliokas
Martin Gräßlin
Martin Gräßlin
Thomas Lübking
Hugo Pereira Da Costa
Lamarque Souza
Martin Gräßlin
Cedric Bellegarde
Martin Gräßlin
Thomas Lübking
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Thomas Lübking
Cedric Bellegarde
Lamarque Souza
Cedric Bellegarde
Cedric Bellegarde
Martin Gräßlin
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Martin Gräßlin
Cedric Bellegarde
Aaron J. Seigo
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Martin Gräßlin
Martin Gräßlin
Cedric Bellegarde
Review request changed

Status: Closed (submitted)

Loading...