[Differential] [Commented On] D3706: WIP: Global Menu Applet

[davidedmundson (David Edmundson)]

davidedmundson added inline comments.

INLINE COMMENTS

> davidedmundson wrote in appmenumodel.cpp:85
> If you return here you have an unmatching begin/remove

Turns out this is what was causing that kate->konsole crash. 
I should have trusted my reviewing instead of spending some time in valgrind...

The UI is left without updating, but we've cleared m_activeMenu.
The model has the actions stored in this casted pointer, but because QQmlEngine doesn't know it's a QObject doesn't reset it. ::trigger uses the action object with now a dangling pointer.

Just remove this if statement, the for loop with no actions will do nothing anyway, so we're not saving any processing.

> appmenumodel.cpp:89
> +        for (QAction *action : actions) {
> +            qint64 data = reinterpret_cast<qint64>(action);
> +            m_activeActions.append(data);

We can't do this!
For one thing, every 32 bit system will pad the first half of this copy with garbage.

We can either register QAction as an uncreatable QML type; or just box in QVariant<QObject*> and pass QVariant as arguments to ::trigger

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D3706

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: chinmoyr, #plasma, broulik
Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161219/2707f9e6/attachment.html>