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 Flöser Martin Flöser
menubar.cpp is probably also only needed if KWIN_BUILD_KAPPMENU is true. So best wrap it in an iff block like done ... Martin Flöser Martin Flöser
I would suggest to have the API always including the two methods even if the define is not set. Martin Flöser Martin Flöser
*cough* :-) Martin Flöser Martin Flöser
same as in bridge.h Martin Flöser Martin Flöser
In the library I think we should not have the ifdefs. That might cause ABI issues. Actually the library does ... Martin Flöser Martin Flöser
whitespaces and coding style Martin Flöser Martin Flöser
that does not sound very Qt: getters do not start with get :-) If it is really a window ID ... Martin Flöser Martin Flöser
? Martin Flöser Martin Flöser
I think here in the options we don't need all the ifdefs. Look above what has been done for the ... Martin Flöser Martin Flöser
you don't need to include config-kwin.h here. It gets include later on by the include of utils.h Martin Flöser Martin Flöser
is the wid really a window id? In that case you should use a WId type or at least qulonglong. ... Martin Flöser Martin Flöser
can we have m_variableName for new things? Yes I know it's inconsistant but we should try to be consistant for ... Martin Flöser Martin Flöser
please use NULL instead of 0. When we are using C++11 I plan to do a global replace of NULL ... Martin Flöser Martin Flöser
if (!applicationMenuImporter) Martin Flöser Martin Flöser
if (!menubar) Martin Flöser Martin Flöser
the second part of this else if (null check) is not needed. It is save to delete a null pointer. ... Martin Flöser Martin Flöser
why is this needed at all? options is available to everything of KWin, so no need to wrap it in ... Martin Flöser Martin Flöser
somehow I don't like this. Why not emitting a signal in workspace and have in client a connect from signal ... Martin Flöser Martin Flöser
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 Flöser
Cedric Bellegarde
Martin Flöser
Hugo Pereira Da Costa
Hugo Pereira Da Costa
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Giorgos Tsiapaliokas
Giorgos Tsiapaliokas
Martin Flöser
Martin Flöser
Thomas Lübking
Hugo Pereira Da Costa
Lamarque Souza
Martin Flöser
Cedric Bellegarde
Martin Flöser
Thomas Lübking
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Thomas Lübking
Cedric Bellegarde
Lamarque Souza
Cedric Bellegarde
Cedric Bellegarde
Martin Flöser
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Martin Flöser
Cedric Bellegarde
Aaron J. Seigo
Cedric Bellegarde
Cedric Bellegarde
Cedric Bellegarde
Martin Flöser
Martin Flöser
Cedric Bellegarde
Review request changed

Status: Closed (submitted)

Loading...