Differential D7813

Suppress yakuake "Unregister input type" warning when using qdbus
ClosedPublic
Actions

Authored by vtasoulas on Sep 14 2017, 11:12 AM.

Details

Reviewers
hein
alexeymin
Group Reviewers
KDE Applications
Commits
R369:ca460bf5331b: Suppress yakuake "Unregister input type" warning when using qdbus
Summary

When using the qdbus interface of yakuake, the following warning is constantly printed in the terminal:

Skipped method "addSession" : Unregistered input type in parameter list: Session::SessionType

That's because the enum Session::SessionType is the type of parameter that is accepted by the addSession function that is exposed through the dbus interface, and this enum hasn't been registered (with a Q_DECLARE_METATYPE?).

As I see, registering a new metatype can be a bit tricky, and I guess this might be one reason why the additional functions addSessionTwoHorizontal(), addSessionTwoVertical() and addSessionQuad() exist: simplified code. After all, these functions are just one-line-code wrappers of the existing addSession() function (they call the addSession() with different session type parameter).

This patch renames the existing int SessionStack::addSession(Session::SessionType type) function to SessionStack::addSessionImpl(Session::SessionType type), and creates a yet-another-wrapper function int SessionStack::addSession() without any parameters to make it available through the QDBus interface in order to suppress these warnings without breaking the DBus API. The newly added addSession() behaves exactly as the old addSession() behaved; just creates a new Session::Single type session.

Test Plan

Start yakuake from a shell, and:

  1. (optional) If your system supports auto completion, type the following in a second shell:

qdbus org.kde.yakuake / <- start pressing the tab key here to see the available functions.

  1. Execute the command qdbus org.kde.yakuake /yakuake/sessions org.kde.yakuake.addSession

Both for 1 and 2 check the shell that you started yakuake from, and you should see Skipped method "addSession" : Unregistered input type in parameter list: Session::SessionType messages appearing each time you press the tab key or calling the function org.kde.yakuake.addSession.

Diff Detail

Repository
R369 Yakuake
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vtasoulas created this revision.Sep 14 2017, 11:12 AM
alexeymin accepted this revision.Sep 14 2017, 6:56 PM
alexeymin added a subscriber: alexeymin.

Tested, works as described, but changes DBus interface. Not sure if someone/something is using it (old addSession()) with DBus

This revision is now accepted and ready to land.Sep 14 2017, 6:56 PM
alexeymin requested changes to this revision.Sep 14 2017, 7:53 PM

I tend to think that this may be too intrusive change just to fix a warning (which can be seen only if yakuake is started from console?) . This diff can be changed to avoid possible breakage of DBus interface, for example:

  1. add non-exported method called like addSessionImpl(), that will accept parameter Session::SessionType (rename addSession() -> addSessionImpl() and remove Q_SCRIPTABLE)
  2. rename all existing code references to addSession() to call new addSessionImpl() (there are not so many)
  3. then add new exported addSession() with Q_SCRIPTABLE, but without parameter, which calls addSessionImpl(Session::Single)

So, DBus interface is kept, and no warning...

Or, discard this (it works as is, the new tab is opened anyway, and the warning is only seen if yakuake is started from console)
Or, wait what @hein says.

This revision now requires changes to proceed.Sep 14 2017, 7:53 PM
hein requested changes to this revision.Sep 14 2017, 8:39 PM

This is not acceptable as it breaks public D-Bus API.

vtasoulas updated this revision to Diff 19548.EditedSep 14 2017, 11:06 PM
vtasoulas edited edge metadata.
vtasoulas edited the summary of this revision. (Show Details)

Changed the commit to prevent breaking the existing DBus API based on the suggestions of @alexeymin.

alexeymin accepted this revision as: alexeymin.Sep 16 2017, 11:57 AM

Works with the same D-Bus interface as before; and no more warnings about unregistered types. Looks good...

hein added a comment.Sep 16 2017, 2:09 PM

I.e. this keeps the interface unchanged because the parameter-passing to the old method didn't actually work, or what is the idea here?

alexeymin added a comment.EditedSep 17 2017, 9:59 AM
In D7813#146280, @hein wrote:

I.e. this keeps the interface unchanged because the parameter-passing to the old method didn't actually work, or what is the idea here?

Yes, I tested with current master: the addSession method is not recognized as taking any parameters:

lexx@gentoo ~/dev/yakuake/build $ qdbus org.kde.yakuake /yakuake/sessions org.kde.yakuake.addSession 1
Invalid number of parameters

This patch keeps that the same, and does not print: Skipped method "addSession" : Unregistered input type in parameter list: Session::SessionType when this method is called.

For example the other method, removeSession() when called from qdbusviewer explicitly requests arguments; it doesn't do that for addSession() :

Logically, DBus does not have a built-in type Session::SessionType; other options to do this could be:

  1. export an overload of addSession() taking int parameter, with smth like reinterpret_cast<Session::SessionType>(..) ...
  2. deal with qRegisterMetaType or whatever needed to register Session::SessionType.
vtasoulas added a comment.Sep 17 2017, 8:54 PM

As long as the parameters of a function that is exposed via the QDBus interface are of a non-standard type such as int, QString etc, QDbus doesn't know how to handle them (kind of expected).
After some searching I believe that the correct way to handle such parameters is to register a new metatype for your special parameter and create marhalling/demarshalling functions as explained here: http://doc.qt.io/qt-5/qdbusargument.html#details
But this will need many more changes in the code.

The addSession by default was spawning a new Single session, and the function was only called with parameters other than the default (in order to spawn a different type of session) from the wrapper functions.

So with this patch, I just made the addSession yet-another-wrapper-function that doesn't accept any parameters (so QDbus doesn't complain), and calls the addSessionImpl with the Session::Single parameter to start a single session.

vtasoulas edited the summary of this revision. (Show Details)Oct 7 2017, 11:55 PM

@hein is this patch acceptable now to be committed?

hein added a comment.Oct 10 2017, 1:15 PM

Yes, push it. Or do you need help comitting?

vtasoulas added a comment.Oct 10 2017, 2:07 PM

I don't have commit access so someone else should do it for me :)

Thank you!

Closed by commit R369:ca460bf5331b: Suppress yakuake "Unregister input type" warning when using qdbus (authored by hein). · Explain WhyOct 24 2017, 6:53 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mapp/sessionstack.h (3 lines)
Mapp/sessionstack.cpp (13 lines)

Diff 21213

View Options

app/sessionstack.h

Loading...
View Options

app/sessionstack.cpp

Loading...