Bug 423643 - plasma-framework commit 7c4f5c5f1 on qt-5.15.0 broke the systray (and more)
Summary: plasma-framework commit 7c4f5c5f1 on qt-5.15.0 broke the systray (and more)
Status: RESOLVED FIXED
Alias: None
Product: libplasma
Classification: Frameworks and Libraries
Component: components (show other bugs)
Version: unspecified
Platform: Other Other
: VHI normal
Target Milestone: ---
Assignee: Marco Martin
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2020-06-29 07:57 UTC by Duncan
Modified: 2020-07-01 17:53 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Duncan 2020-06-29 07:57:17 UTC
I'm on live-git frameworks and plasma via gentoo/kde's live-git ebuilds in the kde overlay.  qt-5.15.0

Plasma-framework's commit 7c4f5c5f1 broke the systray on qt-5.15.0.  The icons still appear and tooltips show up, but normal systray apps won't respond to clicks.  ("Normal" systray apps, as opposed to plasmoids in the tray, which continue to work.)  Backing off a commit to 94d7efaec gets them working again.

With the 7c4 commit, killing plasmashell and restarting it from a konsole window to get logging gives me this when I click on any of the unresponsive systray icons:

file:///share/plasma/plasmoids/org.kde.plasma.private.systemtray/contents/ui/items/StatusNotifierItem.qml:64: Error: Unknown method return type: ServiceJob*

(The path is /share because I have a reverse usrmerge, /usr is a symlink: /usr -> . so share ends up directly under /.)

The error is the same but with a different line number depending on the button clicked.  All such lines are calls to service.startOperationCall(operation).  Doing a search for startOperationCall in libs turned up libKF5Plasma.so*, and a query on owner said plasma.  A few plasma-frameworks rebuilds later and I had the culprit commit.

Note that I'm on qt-5.15.0, the #if conditional used repeatedly in that commit.  So either something's wrong with the code protected by that condition, or the condition itself is wrong (maybe it's 5.15.1+ or something).  

I'm not a dev so don't ask me to explain what it means, but given a specific commit to look at along with the error above, I can see in that commit:

+#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)
+    qmlRegisterInterface<Plasma::ServiceJob>(uri, 1);
+#else
     qmlRegisterInterface<Plasma::ServiceJob>("ServiceJob");
+#endif

And knowing I'm on qt 5.15...

FWIW there's a similar problem with the clipboard applet, which being a plasmoid still works in general, but if I right-click and select configure, no configure dialog pops up, but I see this in the konsole tab from which I restarted plasmashell:

file:///share/plasma/plasmoids/org.kde.plasma.clipboard/contents/ui/clipboard.qml:70: Error: Unknown method return type: ServiceJob*

grep -r startOperationCall /share/plasma/plasmoids/
lists a bunch of similar calls in other plasmoids, mediacontroller, battery, device-notifier, etc, most of which I don't run so I don't know if they're similarly affected, but I'd guess yes.

(It's obviously plasma-framework but I've no idea what component, so I left it "components".  Unfortunately a quick look at previous bugs to see what other people used for similar bugs didn't yield anything similar enough to provide a hint, either.  Additionally, the version is left at "unspecified" because the "master" version that I would chose if it was there and that I've used on other products when filing kde bugs, is missing.)
Comment 1 Nate Graham 2020-06-30 18:56:44 UTC
Can confirm. Thanks for the report.

Friedrich, can you investigate and preferably fix before Frameworks 5.72 tagging, which is four days? Thanks!
Comment 2 Friedrich W. H. Kossebau 2020-06-30 19:09:16 UTC
Had not seen that in my testing, so screwed up somewhere, meh.

Looking at the code with this bug report I mind I suspect the new method registers this with the namespace instead, that had slipped my eyes/attention.
Will look closer at this tonight and possibly have to revert then, no idea yet how this could be worked around.

Thanks for reporting.
Comment 3 Fabian Vogt 2020-06-30 20:18:53 UTC
(In reply to Friedrich W. H. Kossebau from comment #2)
> Looking at the code with this bug report I mind I suspect the new method
> registers this with the namespace instead, that had slipped my
> eyes/attention.

Yep, this sounds like a case of https://bugreports.qt.io/browse/QTBUG-50225
Comment 4 Aleix Pol 2020-07-01 01:48:45 UTC
Git commit ac6bff575f934bceab03abdabc6a5c841c5b1f5e by Aleix Pol.
Committed on 01/07/2020 at 01:47.
Pushed by apol into branch 'master'.

Use fully qualified identifiers for metaobject types

Fixes QML error:
share/plasma/plasmoids/org.kde.plasma.private.systemtray/contents/ui/items/StatusNotifierItem.qml:64: Error: Unknown method return type: ServiceJob*

M  +1    -1    src/plasma/service.h

https://invent.kde.org/frameworks/plasma-framework/commit/ac6bff575f934bceab03abdabc6a5c841c5b1f5e
Comment 5 Friedrich W. H. Kossebau 2020-07-01 01:55:45 UTC
(In reply to Fabian Vogt from comment #3)
> (In reply to Friedrich W. H. Kossebau from comment #2)
> > Looking at the code with this bug report I mind I suspect the new method
> > registers this with the namespace instead, that had slipped my
> > eyes/attention.
> 
> Yep, this sounds like a case of https://bugreports.qt.io/browse/QTBUG-50225

That issue you linked finally gave me the clue what could be broken here (though still unsure what was messed up with my test setup that I saw no issues or if I was unconcentrated those days without knowing):

The code before my change was this:
```
    qmlRegisterInterface<Plasma::Service>("Service");
    qRegisterMetaType<Plasma::Service*>("Service");
    qmlRegisterInterface<Plasma::ServiceJob>("ServiceJob");
    qRegisterMetaType<Plasma::ServiceJob*>("ServiceJob");
    // [...]
    qmlRegisterInterface<Plasma::DataSource>("DataSource");
    qRegisterMetaType<Plasma::DataSource*>("DataSource");
```
Next to that in the headers of the respective classes there is:
```
Q_DECLARE_METATYPE(Plasma::Service *)
Q_DECLARE_METATYPE(Plasma::ServiceJob *)
// nothing for Plasma::DataSource*
```
There are two issues here:
a)
all three classes are subclasses of QObject, and for that the docs of Q_DECLARE_METATYPE tell us:
"Some types are registered automatically and do not need this macro:
* Pointers to classes derived from QObject
[...]
"
This seems a change in Qt5 over Qt4, but the code, added at Plasma4 times, was not updated to remove the now unneeded Q_DECLARE_METATYPE declarations.
So, by itself Qt has already registered metatypes for "Plasma::Service", "Plasma::Service*", "Plasma::ServiceJob", "Plasma::ServiceJob*", "Plasma::DataSource" & "Plasma::DataSource*", under these full namespaced names.

Just, some API which is e.g. tagged with Q_INVOKABLE does not use the full namespace name, but omits the Plasma namespace, like "ServiceJob *Service::startOperationCall(const QVariantMap &description, QObject *parent)". And as said in the referenced bug report, that name has to be explicitly additionally registered, to allow Qt the mapping to the actual type metadata.
But see the current qRegisterMetaType<T>("name") as given above: they miss the "*" from the name. So they are registering the wrong incomplete name. And so e.g.
```
qRegisterMetaType<Plasma::ServiceJob*>("ServiceJob");
```
will not help Qt when it tries to match the type name "ServiceJob*" from the invokable above to a registered type in the metadata system.

Now why was there no runtime issue with the old code then you ask? Because 
```
qmlRegisterInterface<Plasma::ServiceJob>("ServiceJob");
```
has saved the day, as its implementation also registers the pointer to the template class automatically, using the passed name string and appending a "*":
```
template<typename T>
int qmlRegisterInterface(const char *typeName)
{
    QByteArray name(typeName);

    QByteArray pointerName(name + '*');
    QByteArray listName("QQmlListProperty<" + name + '>');

    QQmlPrivate::RegisterInterface qmlInterface = {
        1,

        qRegisterNormalizedMetaType<T *>(pointerName.constData()),
        qRegisterNormalizedMetaType<QQmlListProperty<T> >(listName.constData()),

        qobject_interface_iid<T *>(),
        "",
        0
    };

    return QQmlPrivate::qmlregister(QQmlPrivate::InterfaceRegistration, &qmlInterface);
}
```

Now the code  with Qt 5.15 instead calling the new qmlRegisterInterface<>(const char *uri, int versionMajor) instead does things using the full name, as fetched from the metaobject instance:
```
template<typename T>
int qmlRegisterInterface(const char *uri, int versionMajor)
{
    QML_GETTYPENAMES

    QQmlPrivate::RegisterInterface qmlInterface = {
        1,
        qRegisterNormalizedMetaType<T *>(pointerName.constData()),
        qRegisterNormalizedMetaType<QQmlListProperty<T>>(listName.constData()),
        qobject_interface_iid<T *>(),

        uri,
        versionMajor
    };

    return QQmlPrivate::qmlregister(QQmlPrivate::InterfaceRegistration, &qmlInterface);
}
```

Thus the day is no longer saved.

This would be my sleepy analyis of the cause. Not sure if this can be easily fixed. Possibly better to revert my commit and do a fully experts-checked patch for 5.73 then?
Comment 6 Friedrich W. H. Kossebau 2020-07-01 01:58:02 UTC
(In reply to Aleix Pol from comment #4)
> Git commit ac6bff575f934bceab03abdabc6a5c841c5b1f5e by Aleix Pol.
> Committed on 01/07/2020 at 01:47.
> Pushed by apol into branch 'master'.
> 
> Use fully qualified identifiers for metaobject types
> 
> Fixes QML error:
> share/plasma/plasmoids/org.kde.plasma.private.systemtray/contents/ui/items/
> StatusNotifierItem.qml:64: Error: Unknown method return type: ServiceJob*
> 
> M  +1    -1    src/plasma/service.h
> 
> https://invent.kde.org/frameworks/plasma-framework/commit/
> ac6bff575f934bceab03abdabc6a5c841c5b1f5e

I fear that would be only one incarnation where the no longer registered type is fixed, instead/additionally we might also want to make sure the unnamespaced pointer and non-pointer types are registered as they had been so far?
Comment 7 Friedrich W. H. Kossebau 2020-07-01 02:21:42 UTC
Dumped some patch sketches here:
https://invent.kde.org/frameworks/plasma-framework/-/merge_requests/24

Will continue tomorrow CEST afternoon.
Comment 8 Fabian Vogt 2020-07-01 06:27:19 UTC
(In reply to Friedrich W. H. Kossebau from comment #5)
> ...
> Just, some API which is e.g. tagged with Q_INVOKABLE does not use the full
> namespace name, but omits the Plasma namespace, like "ServiceJob
> *Service::startOperationCall(const QVariantMap &description, QObject
> *parent)". And as said in the referenced bug report, that name has to be
> explicitly additionally registered, to allow Qt the mapping to the actual
> type metadata.
> But see the current qRegisterMetaType<T>("name") as given above: they miss
> the "*" from the name. So they are registering the wrong incomplete name.
> And so e.g.
> ```
> qRegisterMetaType<Plasma::ServiceJob*>("ServiceJob");
> ```
> will not help Qt when it tries to match the type name "ServiceJob*" from the
> invokable above to a registered type in the metadata system.
I guess it should be either
qRegisterMetaType<Plasma::ServiceJob*>("ServiceJob*");
or
qRegisterMetaType<Plasma::ServiceJob>("ServiceJob");
?

The doc says:
> Warning: This function is useful only for registering an alias (typedef) for every other use case Q_DECLARE_METATYPE and qMetaTypeId() should be used instead.
So that seems like it could be used for this.

> ...
> This would be my sleepy analyis of the cause.
I got to the same conclusion at least.

(In reply to Friedrich W. H. Kossebau from comment #6)
> (In reply to Aleix Pol from comment #4)
> > Git commit ac6bff575f934bceab03abdabc6a5c841c5b1f5e by Aleix Pol.
> > Committed on 01/07/2020 at 01:47.
> > Pushed by apol into branch 'master'.
> > 
> > Use fully qualified identifiers for metaobject types
> > ...
> 
> I fear that would be only one incarnation where the no longer registered
> type is fixed, instead/additionally we might also want to make sure the
> unnamespaced pointer and non-pointer types are registered as they had been
> so far?
Yes, there might be users of this outside of plasma-framework as well.
Though I wasn't able to find any of those on lxr.kde.org or GitHub.

Technically this is an API break though.
Comment 9 Friedrich W. H. Kossebau 2020-07-01 11:13:30 UTC
I think best for now is to revert the respective commits, proposed as
https://invent.kde.org/frameworks/plasma-framework/-/merge_requests/26

And do a new approach with all needed care, as found now.

Pardon and thanks again for catching and reporting in time before the release tagging.
Comment 10 Friedrich W. H. Kossebau 2020-07-01 17:22:28 UTC
(In reply to Fabian Vogt from comment #8)
> The doc says:
> > Warning: This function is useful only for registering an alias (typedef) for every other use case Q_DECLARE_METATYPE and qMetaTypeId() should be used instead.

Interesting hint, not seen before, thanks. Though after a while I think this also should hint that for non-aliases one should still want to use "template <typename T> int qRegisterMetaType()" to achieve registration as needed beyond usage with QVariant, like for queued signal and slot connections etc.

Having now played & dug some more, actually with the current usages the qmlRegisterInterface seems not needed for those 3 classes, as they are not used with any QML properties, but only as objects in JavaScript code, if at all.
At least by what
    grep Q_PROPERTY . -R | grep "Service\|DataSource" 
done over all the Plasma repos told me.
And by my experiments the JS engine seems happy enough with the "normal" registrations to the meta-object system.

So by example of the bug as reported here, clicking the vlc systray icon to toggle window display:
```
    Q_INVOKABLE Plasma::ServiceJob *startOperationCall(const QVariantMap &description, QObject *parent = nullptr);
    // no qmlRegisterInterface
    qRegisterMetaType<Plasma::ServiceJob *>();
```
-> works
```
    Q_INVOKABLE ServiceJob *startOperationCall(const QVariantMap &description, QObject *parent = nullptr);
    // no qmlRegisterInterface
    qRegisterMetaType<Plasma::ServiceJob *>("ServiceJob*");
```
-> works

Now, the org.kde.plasma.core module is surely loaded everywhere in Plasma. And there is a small chance something 3rd-party relies on the current registrations as interface with QML without knowing. And having broken things just before I feel I want to propose now to just mark all this as KF6 TODO only in another MR :)
Comment 11 Friedrich W. H. Kossebau 2020-07-01 17:53:00 UTC
I suspect the root is this innocent 9 year old commit trying to get things working, and since everyone just copied & pasted, and things worked due to the implicit correct pointer type registration for the namespaceless name :)

https://invent.kde.org/unmaintained/plasma-mobile/-/commit/725c590261a4a0b0d749c78aa45d3d273eb4bdff

Think mysteries have been resolved mainly now for me.