Fix multiple StatusNotifierItems.

[rkapl]

StatusNotifierItems were cloned, because only one object was registered for all StatusNotifierItem services.

Adding support for multiple notifiers fixes https://github.com/lxde/lxqt/issues/1105 for me.

[tsujan]

The logic looked sound to me too; I just wasn't sure about probable consequences for LXQt. So, if Qt uses this too, GTM.

[luis-pereira]

@rkapl Arguing is part of good decisions.

Let's take mSessionBus.unregisterObject("/StatusNotifierItem"); and it's prototype: void unregisterObject(const QString &path, UnregisterMode mode = UnregisterNode).

QDBusConnection::unregisterObject() takes a const QString &. There is an implicit conversion from const char* to QString, via its constructor. A new QStringData is allocated with enough room to hold "/StatusNotifierItem", and then the string is copied and converted from UTF-8 to UTF-16. Please note: Not from Latin1, from UTF-8.

Qt5 changed the default codec of QString’s 8-bit functions from Latin1 to UTF-8. A needed change, but many algorithms are much slower with UTF-8 than with Latin1 (or plain ASCII). QLatin1String, is just a thin wrapper around char * that specify the encoding. There are overloads taking QLatin1String for functions that can operate or the raw Latin1 data directly without conversion, e.g. QString::append(), QString::compare(), QString::contains() and almost all operators (==, !=, +=, =, etc).

By using mSessionBus.unregisterObject(QLatin1String("/StatusNotifierItem")), we avoided the conversion from UTF-8, but we still have an (implicit) conversion from QLatin1String to QString which has to allocate the QStringData on the heap. Can the QStringData allocation be avoided ? Yes, by using QStringLiteral. QStringLiteral will try to generate the QStringData at compile time with all the fields initialized. It will even allocate it in the .rodata section. The use of QStringLiteral with plugins (QPluginLoader) can cause QString static finalization SEGFAULT's. Take a look at QTBUG-49061. The discussion led to this fix. It was considered a new featured and it will only make it to 5.8.

The use of QLatin1String here has no drawbacks. One can argue that it's over-engineering or that this tiny case doesn't matter in the big picture. Well, IMO all new code should be written using QStringLiteral/QLatin1String. It sets the tone. Newcomers will probably follow it. It's not that easy to spot which algorithms perform badly with UTF-8, so we should use it always.

KDE has almost completely updated it's code base. Almost all of the 8 bit strings are handled by QLatin1String/QStringLiteral. The main reason for that change is the change in QString default encoding, AFAIK. They enforce it with KDEFrameworkCompilerSettings.cmake.

@tsujan Using QT_NO_CAST_FROM_ASCII with mSessionBus.unregisterObject("/StatusNotifierItem") will produce a compile error.

The documentation states:

Note: If the function you're calling with a QLatin1String argument isn't actually overloaded to take QLatin1String, the implicit conversion to QString will trigger a memory allocation, which is usually what you want to avoid by using QLatin1String in the first place. In those cases, using QStringLiteral may be the better option.

That comparison is made against the use of QStringLiteral not char*.

What the documentation doesn't clearly states:

• Qt5 changed the default encoding from Latin1 to UTF-8. Qt4 rules of thumb don't apply anymore.
• Many algorithms are much slower with UTF-8 than with Latin1.
• mSessionBus.unregisterObject("/StatusNotifierItem") will convert it from UTF-8... without that being needed.

[rkapl]

@luis-pereira Thanks for the explanation, that makes sense. Changes done.

[tsujan]

That comparison is made against the use of QStringLiteral not char*.

My mistake.

mSessionBus.unregisterObject("/StatusNotifierItem") will convert it from UTF-8... without that being needed.

Interesting! I didn't know that. Thanks!

[luis-pereira]

@rkapl Thanks.