Should we hide the implementation details in a private class like KSNI does it?

[sebholt]

statusnotifieritem.h as of now contains methods, slots, members etc. that are implementations details that could be hidden in a private class.

Should we implement a private class so that StatusNotifierItem contains only single private member std::unique_ptr<StatusNotifierItemPrivate> d;?

[redtide]

Yes, I was thinking the same thing.

A request: could you add those 3 qDBusRegisterMetaType to fix tool tips with a proper commit message, from what you have seen in the source code?

I would like to focus on the build with the CMakeFiles I did in the sni-example, so to test the latter by make it link to a prototype of the lib; I will make a library' AUR PKGBUILD for this purpose.

For the sendMessage I'm still not sure, I wouldn't like to write code apart just for the notification if it can work with little effort here. But I haven't got if the uid changes I did doesn't work for you in KDE or need testing? Or can we add some code to track the replies?

I can then start with the private d pointer as last step if you want.

[sebholt]

can we discuss this on irc on weekend?

Yes, I'm free tomorrow.

Meanwhile about the icon thing: you linked to the XyzName sections, maybe you meant the IconPixmap ones?

Afaiu one can set either the icon name or the pixmap. So I thought of a SNIIcon class with the following interface

SNIIcon (const QString & name);
SNIIcon (const QIcon & icon);

// Is either name or icon
bool isName() const;
bool isIcon() const;

const QString & name() const;
const QIcon & icon() const;

and the SNI then has only three setters

setIcon (const SNIIcon & icon);
setOverlayIcon (const SNIIcon & icon);
setAttentionIcon (const SNIIcon & icon);

and maybe three getters

SNIIcon icon() const;
SNIIcon overlayIcon() const;
SNIIcon attentionIcon() const;

But that may be bit over engineered.

[redtide]

need to think on it with a more fresh mind, meanwhile I see we can't remove custom classes from the public header which is used to build the adaptor.

What KSNI does:

• Use a custom, private dbus class to build the adaptor and keep the DBus stuff apart to be able to work also with "legacy" QSystemIconTray without using DBus at all
• have no custom classes in public API, not even the menu path, which is also a QString

so that file that declares the KStatusNotifierItemDBus is the one where our dbustypes are declared.
I think we should simply do the same, by renaming dbustypes to "snidbus_p" and add the rest there and use it with the cmake qt_add_dbus_adaptor command.

[sebholt]

I like it.

[redtide]

I did it, so I guess if it's OK we could make it public and make some packages for this: I've created an OBS project at https://build.opensuse.org/project/show/home:qtiliti3s (I messed up with the qtilities account :P ).
If you will have time for this, I'll appreciate some help to create the build files, mainly for Debian, which IIRC is the package you miss.

So when you will have some time to give it a look, the changes are in the snidbus branch, I'll make a PR if useful, since it's a messy commit.

[redtide]

Just published a project, a fork of KDE CDEmu Manager iso image mounter, that make use of our lib instead ksni.