D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a comment.


  @davidre can you extract your test improvement to a separate patch? it would 
help me in another issue and I don't want to "steal" your code :)

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, kmaterka, broulik, mart, #plasma, #vdg, #frameworks
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  Regression fixed in D29314 .

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: ngraham, mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, 
michaelh, bruns

D29314: [PlasmaCore.IconItem] Regression: fix crash on source change

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:4b0d4c4bdaed: [PlasmaCore.IconItem] Regression: fix crash 
on source change (authored by kmaterka).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29314?vs=81629=81631

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp

To: kmaterka, #plasma, #frameworks, ngraham, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29314: [PlasmaCore.IconItem] Regression: fix crash on source change

[Konrad Materka]

kmaterka edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, #frameworks, ngraham, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29314: [PlasmaCore.IconItem] Regression: fix crash on source change

[Konrad Materka]

kmaterka edited the summary of this revision.
kmaterka edited the test plan for this revision.
kmaterka added a reviewer: davidedmundson.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, #frameworks, ngraham, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29314: [PlasmaCore.IconItem] Regression: fix crash on source change

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma, Frameworks, ngraham.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  When SvgSource is changed, old one is deleted. Connections are not 
automatically disconected.
  This change disconnects connections.

TEST PLAN
  STEPS TO REPRODUCE
  
  1. click on mute button for a device
  2. click on the desktop to collapse the applet
  
  OBSERVED RESULT
  crash in step 1 and in the rare case it doesn't crash it crash in step 2
  EXPECTED RESULT
  don't crash
  
  BUG: 420801

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp

To: kmaterka, #plasma, #frameworks, ngraham
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  In D28470#660492 , @ngraham wrote:
  
  > `git bisect` says this caused https://bugs.kde.org/show_bug.cgi?id=420801.
  
  
  I will check that, sorry...

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: ngraham, mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, 
michaelh, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:5a3fb570feda: [PlasmaCore.IconItem] Refactor source 
handling for different types (authored by kmaterka).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=81031=81092

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a reviewer: Frameworks.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson, #frameworks
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka updated this revision to Diff 81031.
kmaterka added a comment.


  Rebase to master (includes D29102 )

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=80148=81031

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  Any other comment? Is it OK and can be approved?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka updated this revision to Diff 80148.
kmaterka marked an inline comment as done.
kmaterka added a comment.


  Do not inherit from QObject

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=79283=80148

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka marked an inline comment as done.
kmaterka added a comment.


  In D28470#647757 , @davidedmundson 
wrote:
  
  > Note there's a unit test for IconItem worth running if you haven't already.
  
  
  I've checked that already, these test were really useful!
  
  > The refactor in general makes sense - it's a lot cleaner.
  >  Though I'm not sure what our super long term KF6 plan for IconItem is, 
it'll definitely be changing quite a bit.
  
  If there is no harm now, then it should be easier to change it in the future 
:)

INLINE COMMENTS

> mart wrote in iconitem.cpp:40
> does it have to be a QObject? it doesn't have properties, signals or 
> invokables.. qobject is an expensive class so if you don't have to use oits 
> features is better to avoid

There is one use of `connect` in `SvgSource` but I can change the 
implementation.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: mart, davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, michaelh, 
ngraham, bruns

D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added a reviewer: Frameworks.

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, kmaterka, broulik, mart, #plasma, #vdg, #frameworks
Cc: bruns, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  ping, review needed :)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  Is it OK now? Any other review comments?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka updated this revision to Diff 79283.
kmaterka added a comment.


  Change class name: *Strategy -> *Source

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=79281=79283

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  In D28470#641230 , @davidre wrote:
  
  > In D28470#640468 , @cblack wrote:
  >
  > > Splitting into multiple classes seems like a good idea, but "strategy"? 
Seems like an odd choice of name to me.
  >
  >
  > I had assumed it's because of 
https://en.m.wikipedia.org/wiki/Strategy_pattern
  
  
  Yeah, I totally agree, it is really confusing now. I'm bad at creating class 
names :)
  I wanted to extract source handling (logic, internal state) to separate 
classes. "Handler" is overused, maybe something like this:
  IconItemSource (or AbstractIconItemSource, or AbstractSource), then:
  QImageSource (or QImageIconItemSource)
  QIconSource (or QIconIconItemSource)
  SvgSource (or SvgIconItemSource)
  What do you think?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: davidre, cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D28470: [PlasmaCore.IconItem] Refactor source handling for different types

[Konrad Materka]

kmaterka retitled this revision from "WIP: IconItem: Refactor source handling 
for different types" to "[PlasmaCore.IconItem] Refactor source handling for 
different types".
kmaterka edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D28470: WIP: IconItem: Refactor source handling for different types

[Konrad Materka]

kmaterka updated this revision to Diff 79281.
kmaterka marked an inline comment as done.
kmaterka added a comment.


  be consistent with old implementation

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=79279=79281

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D28470: WIP: IconItem: Refactor source handling for different types

[Konrad Materka]

kmaterka marked 2 inline comments as done.
kmaterka added inline comments.

INLINE COMMENTS

> cblack wrote in iconitem.cpp:58
> Is this class necessary? I feel like this class's behaviour should be what 
> its parent does without a child implementation.

I wanted `IconItemStrategy` to be just an abstract base class, so that to have 
no strategy selected one needs to select null strategy explicitly.

> cblack wrote in iconitem.cpp:80
> Seems unused.

removed unused variable from NullStrategy

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D28470: WIP: IconItem: Refactor source handling for different types

[Konrad Materka]

kmaterka updated this revision to Diff 79279.
kmaterka added a comment.


  fix some silly errors

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28470?vs=79011=79279

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: cblack, kde-frameworks-devel, #plasma, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D28470: WIP: IconItem: Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  Do you think it is a good direction?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: kde-frameworks-devel, #plasma, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28470: WIP: IconItem: Refactor source handling for different types

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma, broulik, apol, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  There are 3 possible strategies: QIcon, QImage and SVG. This change moves 
logic of each of these strategies into separate class.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/iconitem.cpp
  src/declarativeimports/core/iconitem.h

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: kde-frameworks-devel, #plasma, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28197: [KSortFilterProxyModel QML] Make invalidateFilter public

[Konrad Materka]

kmaterka accepted this revision.

REPOSITORY
  R275 KItemModels

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

To: broulik, ahiemstra, davidedmundson, iasensio, kmaterka
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> ahiemstra wrote in ksortfilterproxymodel_qml.cpp:111
> I don't think a unit test is the right place for example code. It's probably 
> better to either improve the documentation of the property or add an example 
> somewhere where it makes sense.

Sure, you are right, documentation first. Logic of sortRole uses sortColumn, 
unit test would be a nice addition :)

> ahiemstra wrote in ksortfilterproxymodel.cpp:165
> In that example, you'd be sorting on the "user" role of column 0, which seems 
> like a reasonable default to me. I would actually suggest to make sortColumn 
> always at least 0, I don't think there really is much of a use case for -1 as 
> default.

sortColumn = -1 comes directly from QSortFilterProxyModel and it has use case 
(disables sorting). 
In `PlasmaCore.SortFilterModel` this worked incorrectly (or confusingly), even 
if you set sortRole it is not sorting.
This is good now and I like it.

I added separate comment for documenting this.

> ahiemstra wrote in ksortfilterproxymodel.h:67
> Let's keep this a bit constrained, if we add a stub implementation of 
> `lessThan()`, we can later on add a property to expose a JS callback for it 
> somehow.

Agree, you are right.

> ksortfilterproxymodel.h:86
> + * Specify which column shoud be used for sorting
> + */
> +Q_PROPERTY(int sortColumn READ sortColumn WRITE setSortColumn NOTIFY 
> sortColumnChanged)

Please add:

  The default value is -1. If \a sortRole is set, the default value is 0.

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kmaterka, iasensio, ahmadsamir, broulik, ahiemstra, mart, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

[Konrad Materka]

kmaterka added a comment.


  Few comments from someone who was recently using `PlasmaCore.SortFilterModel` 
and had trouble understanding sorting :)

INLINE COMMENTS

> ksortfilterproxymodel_qml.cpp:111
> +
> +void tst_KSortFilterProxyModelQml::testSortRole()
> +{

Can you add a test for sortColumn? It might be useful for newcomers (like me) 
to understand how it works. I had real trouble understanding when it is needed 
and when not (ConfigEntries.qml 
)

> ksortfilterproxymodel.cpp:165
> +{
> +sort(std::max(sortColumn(), 0), order);
> +Q_EMIT sortOrderChanged();

When using PlasmaCore.SortFilterModel sortColumn is sometimes set to -1 and 
sorting is not working when `sortColumn` is not set. If I remember correctly, 
-1 is the default in QSortFilterProxyModel. Is `std::max(sortColumn(), 0)` 
added to fix that? What will happen in this situation:

  KSortFilterProxyModel {
sourceModel: testModel
sortColumn: -1
sortRole: "user"
  }

Maybe is should be documented in `sortRole` and `sortOrder` properties that 
these two set sortColumn to 0 (or -1 in case of empty role)?

> ksortfilterproxymodel.cpp:175
> +sort(column, sortOrder());
> +emit sortColumnChanged();
> +}

Which is preferred: `emit` or `Q_EMIT`?

> ksortfilterproxymodel.h:67
> + */
> +Q_PROPERTY(QJSValue filterColumnCallback READ filterColumnCallback WRITE 
> setFilterColumnCallback NOTIFY filterColumnCallbackChanged)
> +

can we have something similar for sorting? `sortColumnCallback` and use it in 
overridden `lessThan`?

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kmaterka, iasensio, ahmadsamir, broulik, ahiemstra, mart, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25223: Avoid side effects during menu initialization

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R135:3c78453b: Avoid side effects during menu 
initialization (authored by kmaterka).

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25223?vs=69634=69924

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

AFFECTED FILES
  src/platformtheme/kdeplatformsystemtrayicon.cpp
  src/platformtheme/kdeplatformsystemtrayicon.h

To: kmaterka, #plasma, #frameworks, broulik
Cc: nicolasfella, davidedmundson, cgiboudeaux, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka added a comment.


  In D25223#561638 , @nicolasfella 
wrote:
  
  > My long-term goal is to get rid of the application side KStatusNotifierItem 
and amend the QSystemTrayIcon API
  
  
  OK, I will think about this. This change is only to fix one regression.
  @broulik Is it OK now? Or should I use std::optional (and require C++17)?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, #plasma, #frameworks, broulik
Cc: nicolasfella, davidedmundson, cgiboudeaux, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka added a comment.


  In D25223#561570 , @davidedmundson 
wrote:
  
  > I had this. I abandoned because we ended up forking some special wayland 
stuff in our DBus menu, so would always want our implementation.
  >
  > > ?https://codereview.qt-project.org/c/qt/qtbase/+/168458
  
  
  Having an option to fallback to default/predefined theme would be great, but 
I guess KDE needs to have it's own SystemTray implementation anyway (?). Some 
cleanup is required, because currently:
  
  - in Plasma-integration (QPA theme):
- there is a forked QDBusMenuBar from Qt
- for tray icon it uses KStatusNotifierItem which has it's drawbacks 
(additional layer)
  - in KNotifications, KStatusNotifierItem uses libdbusmenu-qt (not forked). It 
is optional dependency, is there any reason for that in 2019?
  - in plasma-workspace StatusNotifierItemSource uses DBusMenuImporter forked 
from libdbusmenu-qt
  - Qt has it's own implementation of DBus menu exporter:
- in genericunix qpa theme, unfortuantelly QDBusPlatformMenu is private
- it uses undocumented "NewMenu 
" 
signal
- and is buggy: QTBUG-79287 , 
KDE 383202 
  - libdbusmenu-qt was not actively updated/maintained for years, I see that 
David made few commits in 2015. Shouldn't DBusMenuImporter be upstreamed?
  - Freedesktop spec 
 is still 
in draft phase
  
  Anyway, this is not part of this code review, sorry for the off topic.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, #plasma, #frameworks, broulik
Cc: nicolasfella, davidedmundson, cgiboudeaux, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka marked an inline comment as done.
kmaterka added a comment.


  Off-topic idea: This QPA integration uses KStatusNotifierItem, which then 
translates it to DBus. Wouldn't it be better to talk to DBus directly? From the 
other side, this may be a duplication of work... Was the idea discussed?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, #plasma, #frameworks, broulik
Cc: cgiboudeaux, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka updated this revision to Diff 69634.
kmaterka marked an inline comment as done.
kmaterka added a comment.


  QVariant related changes suggested in comments

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25223?vs=69548=69634

BRANCH
  master

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

AFFECTED FILES
  src/platformtheme/kdeplatformsystemtrayicon.cpp
  src/platformtheme/kdeplatformsystemtrayicon.h

To: kmaterka, #plasma, #frameworks, broulik
Cc: cgiboudeaux, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka marked an inline comment as done.
kmaterka added a comment.


  I will send fixes in 5 minutes

INLINE COMMENTS

> broulik wrote in kdeplatformsystemtrayicon.cpp:33
> Is this explicit initialization neccessary?

Not mandatory, works without this. I just wanted to be... explicit. 
Documentation does not say what is the result of "isNull" when QVariant is not 
valid.
Remove?

> broulik wrote in kdeplatformsystemtrayicon.cpp:188
> Prefer the specific methods, if exist, `toBool()`

OK

> broulik wrote in kdeplatformsystemtrayicon.h:60
> (This looks like a job for `std::optional`?)

Exactly! But I checked the code of KDE and it is never used. There are even 
some custom implementations (optional_view, 3 duplicates). QVariant was the 
closest thing available.

BTW, is KDE officially using C++17?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, #plasma, #frameworks, broulik
Cc: cgiboudeaux, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> kdeplatformsystemtrayicon.cpp:191
> +if (!m_visible.isNull()) {
> +m_menu->setVisible(m_visible.value());
> +}

This line was causing issues, even if menu is (or should be) visible, setting 
true has side effects.
I checked the code of QMenu, QPlatformMenu::setVisible is never used anyway...

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, #plasma, #frameworks, broulik
Cc: cgiboudeaux, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka updated this revision to Diff 69548.
kmaterka added a comment.


  Use QVariant for tri-state boolean. This way atributes are set only when they 
were really changed in the past.

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25223?vs=69489=69548

BRANCH
  master

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

AFFECTED FILES
  src/platformtheme/kdeplatformsystemtrayicon.cpp
  src/platformtheme/kdeplatformsystemtrayicon.h

To: kmaterka, #plasma, #frameworks, broulik
Cc: cgiboudeaux, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka added a comment.


  The problem was with:
  
m_menu->setVisible(m_visible);
  
  setters/getters should not have side effects or any logic, but this method 
has. Detached menu stayed as top level window and rendered itself.
  
  VLC has a bug, each systray menu update creates new QMenu, but old one is not 
deleted:
  https://github.com/videolan/vlc/blob/master/modules/gui/qt/menus.cpp#L655
  
  QMenu::clear() deletes all attached actions (if not used somewhere else) but 
not menus! It is weird, but someone explained it here:
  https://bugreports.qt.io/browse/QTBUG-11070

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, #plasma, #frameworks, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka edited the summary of this revision.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, #plasma, #frameworks, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

[Konrad Materka]

kmaterka added a comment.


  In D24843#560188 , @kmaterka wrote:
  
  > In D24843#560023 , @broulik 
wrote:
  >
  > > This causes menus (mostly submenus) to randomly show up when the SNI is 
updated, e.g. every time VLC changes a track I get its "speed (slower, normal, 
faster)" menu open:
  >
  >
  > I will fix that right away!
  
  
  Fixed in D25223 

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka updated this revision to Diff 69489.
kmaterka added a comment.


  Final version, ready for review.

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25223?vs=69484=69489

BRANCH
  master

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

AFFECTED FILES
  src/platformtheme/kdeplatformsystemtrayicon.cpp

To: kmaterka, #plasma, #frameworks, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka added a comment.


  Too soon, it is still not finished.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, #plasma, #frameworks, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25223: Avoid side effects during menu initialization

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Plasma, Frameworks, broulik.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  Setting some attributes, like visible, enabled, etc has side effects. Do
  not set them if these are not changed.

TEST PLAN
  Without this change menus (mostly submenus) randomly shows up when the SNI is 
updated, e.g. every time VLC changes a track I get its "speed (slower, normal, 
faster)" menu open.

REPOSITORY
  R135 Integration for Qt applications in Plasma

BRANCH
  master

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

AFFECTED FILES
  src/platformtheme/kdeplatformsystemtrayicon.cpp

To: kmaterka, #plasma, #frameworks, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

[Konrad Materka]

kmaterka added a comment.


  In D24843#560023 , @broulik wrote:
  
  > This causes menus (mostly submenus) to randomly show up when the SNI is 
updated, e.g. every time VLC changes a track I get its "speed (slower, normal, 
faster)" menu open:
  >  F7748305: Screenshot_20191108_151344.png 

  >  (note the thick shadow, this is multiple menus stacked ontop of each other)
  
  
  I will fix that right away!

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:96b7fc21f5b5: Add hideOnWindowDeactivate to 
PlasmaComponents.Dialog (authored by kmaterka).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24825?vs=68424=69323

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents/qml/Dialog.qml

To: kmaterka, #plasma, #frameworks, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

[Konrad Materka]

kmaterka added a comment.


  Should I proceed? This component has serious issues anyway, maybe there is no 
point in fixing this?
  For example:
  There is Loader that load always the same QML because... "if (true || )" 
- line 241. Even if this is obsolete, cleanup would be good idea.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R135:bc1c85144adb: [KDEPlatformSystemTrayIcon] Recreate 
deleted menu (authored by kmaterka).

REPOSITORY
  R135 Integration for Qt applications in Plasma

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24843?vs=68482=69313

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kdeplatformsystemtrayicon_unittest.cpp
  src/platformtheme/kdeplatformsystemtrayicon.cpp
  src/platformtheme/kdeplatformsystemtrayicon.h

To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

[Konrad Materka]

kmaterka added a comment.


  Any chance to get this reviewed? :)

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

[Konrad Materka]

kmaterka added a comment.


  This is a proper solution to BUG 365105. Can someone review it? Is everything 
OK with this?

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24843: [KDEPlatformSystemTrayIcon] Recreate deleted menu

[Konrad Materka]

kmaterka added reviewers: Frameworks, broulik, nicolasfella.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: kmaterka, apol, davidedmundson, #plasma, #frameworks, broulik, nicolasfella
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka added a comment.


  Moved to D24843 

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka added a comment.


  > First, it will not have memory leaks, we take ownership on parentless menu, 
on menu that has a parent, it will destroy it by parent-child cleanups.
  
  Yes, eventually menu will be deleted. I'm thinking about the situation when 
someone has:
  
if (configuration-trayIconEnabled) {
   m_sni = new KStatusNotifierItem();
   m_sni->setContextMenu(new QMenu(**this**));
} else {
  delete m_sni;
}
  
  and user is playing with this setting and changes it 100 times during one 
session. It will create 100 QMenu instances.
  
  > I want to be more clear why SystemTrayMenu is not destroyed on hide, the 
idea behind that code is to be created a new try menu. On updateMenu you call 
it by same object that's why it's not destroyed, did you have stack strace, 
that's not normal to me.
  
  That's how QSystemTrayIcon works, I checked the source code. 
QSystemTrayIcon::show() and QSystemTrayIcon::hide will create and destroy 
KDEPlatformSystemTrayIcon (this is a little bit more complicated, because there 
are also two additional methods involved: init and cleanup). 
  For menu, QSystemTrayIcon uses:
  
QPlatformMenu* KDEPlatformSystemTrayIcon::createMenu()
  
  which is kept alive until QSystemTrayIcon instance exist.
  So something like this:
  
QSystemTrayIcon *sysTray = new QSystemTrayIcon(this);
sysTray->setContextMenu(new QMenu()); // create QPlatformMenu (AFAIR it is 
postponed, but you get the idea)
sysTray->show(); // create QPlatformSystemTrayIcon
sysTray->hide(); // delete QPlatformSystemTrayIcon
sysTray->show(); // create second QPlatformSystemTrayIcon and reuse 
QPlatformMenu
  
  Will create two instances of KDEPlatformSystemTrayIcon 
(QPlatformSystemTrayIcon) and only one of SystemTrayMenu (QPlatformMenu).
  
  Anyway, this whole "do not take ownership" is a dead end. Even if menu is not 
deleted, it won't work, there is another issue in dbusmenu-qt library... I 
abandon this idea, sorry for taking your time. I have another solution, in:
  
void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu)
{
if (SystemTrayMenu *ourMenu = qobject_cast(menu)) {
m_sni->setContextMenu(ourMenu->menu());
}
}
  
  SystemTrayMenu::menu() will return new QMenu and keep track of changes.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka abandoned this revision.
kmaterka added a comment.


  This is a dead end, I will implement different solution in QPA 
(KDEPlatformSystemTrayIcon) plugin.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka added a comment.


  In D24755#550415 , @anthonyfieroni 
wrote:
  
  > That should be fine, in QPA we have a tray icon per sni, update menu should 
be on same object which will not be a problem, check it.
  
  
  There are two objects in QPA that live independently:
  
  - KDEPlatformSystemTrayIcon (QPlatformSystemTrayIcon), with KSNI instance, 
KSNI and KDEPlatformSystemTrayIcon **are** destroyed on QSystemTrayIcon->hide() 
and new instance (with new KSNI) is created on QSystemTrayIcon->show()
  - SystemTrayMenu (QPlatformMenu) is **not** destroyed on 
QSystemTrayIcon->hide() and will be reused later on QSystemTrayIcon->show()
  
  kdeplatformsystemtrayicon.cpp#L339 

  
void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu)
{
//...
if (SystemTrayMenu *ourMenu = qobject_cast(menu)) {
m_sni->setContextMenu(ourMenu->menu());
}
}
  
  About you patch: I understand your idea, but it changes API contract and is 
not backward-compatible. Current documentation says:
  
  > The KStatusNotifierItem instance takes ownership of the menu, and will 
delete it upon its destruction.
  
  This is quite clear, I want to be really careful here - I don't want to be 
blamed for memory leaks :) I think that we need to keep:
  
  > d->menu->setParent(nullptr);
  
  in setContextMenu.
  
  Then, to prevent menu deletion, developer needs to explicitly retake 
ownership, for example:
  
QMenu *menu = ourMenu->menu()
QWidget *parent = menu->parent();
m_sni->setContextMenu(menu);
menu->setParent(parent);
  
  The problem is that SystemTrayMenu->menu() has no parent and there is no 
QWidget to use...

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

[Konrad Materka]

kmaterka added a comment.


  This is a first step to solve 401016 
:
  "Touchpad plasmoid [confirmation dialog] remains open when losing focus, I 
think that it is inconsistent since other elements of tray close themselves 
after switching away."
  
  AFAIK PlasmaComponents 2 are deprecated. Is there newer QueryDialog 
replacement?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

[Konrad Materka]

kmaterka edited the summary of this revision.
kmaterka added reviewers: Plasma, Frameworks.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24825: Add hideOnWindowDeactivate to PlasmaComponents.Dialog

[Konrad Materka]

kmaterka created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  When using Dialog or QueryDialog it should be possible to set 
hideOnWindowDeactivate flag.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents/qml/Dialog.qml

To: kmaterka
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kstatusnotifieritem.cpp:790
> That's easy checkable
> 
>   if (!qApp->closingDown() && (!d->menu->parent() || d->menu->parent() == 
> d->associatedWidget) {
>   delete d->menu;
>   }
> 
> We should not own the menu, that's not tight approach at least.

This check is not reliable, assosiatedWidget can change. Anyway, this doesn't 
matter here.
Did you read whole comment? Probably KSNI should not own the menu but it is 
doing that for 10 (more?) years. It is even documented in the API.
Your idea will not fix the main issue, we can't set a parent to menu in 
KDEPlatformSystemTrayIcon. Main purpose of this hack is to prevent deletion of 
menu when it is *not* possible to set parent.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kstatusnotifieritem.cpp:454
> Do not take ownership of the menu and delete it when it does not have a 
> parent. takeOwnership is wrong approach, you can remove it.

In theory that should be the correct approach, the "Qt way". But we have 
existing contract (also discussed in D24667 
), documentation is clear, menu ownership 
is always transferred and menu removed, regardless of the parent (please check 
my comment in line 790, parent might be null or might not be null).

> kstatusnotifieritem.cpp:790
>  //create a default menu, just like in KSystemtrayIcon
>  QMenu *m = new QMenu(associatedWidget);
>  

"associatedWidget" is a KSNI parent (line 780). It might be or might not be 
set. If parent is not set, then "associatedWidget" is null and QMenu does not 
have parent. This is fine, we will delete it. But if there is parent then menu 
won't be deleted and we will have memory leak. Eventually this menu will be 
deleted, when parent is destroyed, but current contract is different.
To make things even worse, associatedWidget can change, so we can't compare the 
parent of the menu with associatedWidget during the destruction...
Let's say we will change it to:

  new QMenu()

Then it will be removed, because there is no parent. It should not have any 
important side effects. So far so good.

What we want to achieve is have an ability to retake ownership after it is 
passed to setContextMenu. With your approach, it could be done this way:
QMenu *menu =new QMenu(); // null parent, doesn't matter here
tray->setContextMenu(menu);
menu->setParent(parent);
This way menu won't be deleted. There are two problems with this approach:

- we don't know if no-one is doing that - most probably not and this can be 
ignored
- the parent needs to be a QWidget type. This is serious issue, because there 
are cases when it is not possible.

The final goal is to fix KDEPlatformSystemTrayIcon and there is no QWidget to 
use as a parent :( Exactly:
kdeplatformsystemtrayicon.cpp:339

  m_sni->setContextMenu(ourMenu->menu());

ourMenu can live longer than KStatusNotifierItem *m_sni and can be reused for 
another KStatusNotifierItem instance. The situation is described in BUG 365105 
. In other word, in QPA, menu can 
and should live independently to system tray icon, which is not the case in 
KStatusNotifierItem.

I really like your idea! Maybe I'm missing something obvious and is possible to 
set parent in KDEPlatformSystemTrayIcon...

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka marked 5 inline comments as done.
kmaterka added a comment.


  In D24755#550140 , @davidedmundson 
wrote:
  
  > I don't exactly like it, but I don't have anything better. +1
  
  
  Me neither. Probably the best would be to use shared pointer, but it is not 
backward compatible.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka updated this revision to Diff 68292.
kmaterka added a comment.


  Wrap menu in QPointer

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24755?vs=68229=68292

BRANCH
  master

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

AFFECTED FILES
  src/kstatusnotifieritem.cpp
  src/kstatusnotifieritem.h
  src/kstatusnotifieritemprivate_p.h

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kstatusnotifieritemprivate_p.h:158
> QPointer here won't solve the other hacks needed.
> 
> We have MenuSource, Menu and KSNI
> 
> QPointer would fix the case of MenuSource still owning the Menu and KSNI 
> lasting longer than the Menu.
> 
> But we need things the other way round.
> 
> We have MenuSource lasting longer than KSNI we need to supply a menu to the 
> KSNI but keep the menu after the KSNI is deleted. QPointer won't solve that.
> 
> (might be worth switching this to a QPointer anyway, now that ksni doesn't 
> control the menu lifespan though)

I will wrap m_menu in QPointer. When mane "ownership" is outside of the KSNI 
this is a valid case when menu is deleted before KSNI. That's a good practice 
anyway.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kstatusnotifieritemprivate_p.h:158
> It should be QPointer, no other changes are needed.

Sure, but this is not my code and this is not related. What I want to do here 
is more like hack/workaround, so it is better to keep it as simple as possible.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka added a comment.


  Continuation of D24667 . I know this 
solution sucks, but I have no idea how this can be done better.
  
  I have another idea, probably even crazier: set "DO_NOT_DELETE" flag as a 
QObject property. Then in destructor check it:
  if (d->m_menu->property("DO_NOT_DELETE") == true) // ignore
  
  Obviously that would be undocumented, ugly hack... Any other idea how to fix 
bug 365105?

REPOSITORY
  R289 KNotifications

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

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Konrad Materka]

kmaterka created this revision.
kmaterka added reviewers: Frameworks, davidedmundson, broulik, nicolasfella.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  It is not possible to handle ownership of QMenu the Qt way, because
  QMenu takes QWidget as a parent, but KSNI is a QObject. In some
  situations (Qt QPA plugin) we don't want QMenu to be destroyed.
  
  CCBUG: 365105

REPOSITORY
  R289 KNotifications

BRANCH
  master

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

AFFECTED FILES
  src/kstatusnotifieritem.cpp
  src/kstatusnotifieritem.h
  src/kstatusnotifieritemprivate_p.h

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: [KStatusNotifierItem] Allow left click when menu is null

[Konrad Materka]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:aa6bb9b06cfb: [KStatusNotifierItem] Allow left click when 
menu is null (authored by kmaterka).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24667?vs=67986=68165

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

AFFECTED FILES
  src/kstatusnotifieritem.cpp
  src/kstatusnotifieritemdbus_p.cpp

To: kmaterka, davidedmundson, broulik, nicolasfella, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: [KStatusNotifierItem] Allow left click when menu is null

[Konrad Materka]

kmaterka marked 2 inline comments as done.
kmaterka added a comment.


  In D24667#548245 , @broulik wrote:
  
  > > bool takeOwnership = true);
  >
  > If only Qt/we used modern C++ features to communicate object ownership :)
  
  
  Hehe, yes. Unfortunately KSNI messes with parent of menu object :/
  
  >> do not delete menu in KSNI
  > 
  > Makes the most sense in some form.
  >  It /could/ be done without additional API by abusing QObject parentship: 
https://phabricator.kde.org/P479
  > 
  > Then this code being a special case would just set the parent back 
afterwards. Whether that's actually any better than any explicit boolean method 
is debatable though.
  
  Believe me, I checked that :) I was thinking about this, but it won't work in 
this form. There are two reasons. First:
  
void KStatusNotifierItemPrivate::init(const QString )
//...
QMenu *m = new QMenu(associatedWidget);
  
  associatedWidget can be null or not. I don't know if this is needed, what is 
the purpose of this code and what are side effects. Probably this can be safely 
removed.
  Second reason is more serious: QMenu parent must be of QWidget type and KSNI 
is not a widget.

REPOSITORY
  R289 KNotifications

BRANCH
  master

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

To: kmaterka, davidedmundson, broulik, nicolasfella, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: [KStatusNotifierItem] Allow left click when menu is null

[Konrad Materka]

kmaterka added a comment.


  I think the best would be to add additional parameter to KSNI. Change from:
  
void KStatusNotifierItem::setContextMenu(QMenu *menu);
  
  to something like this:
  
void KStatusNotifierItem::setContextMenu(QMenu *menu, bool takeOwnership = 
true);
  
  Then, after next release of Frameworks, change implementation in 
plasma-integration.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, davidedmundson, broulik, nicolasfella, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: [KStatusNotifierItem] Allow left click when menu is null

[Konrad Materka]

kmaterka added a reviewer: Frameworks.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, davidedmundson, broulik, nicolasfella, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: [KStatusNotifierItem] Allow left click when menu is null

[Konrad Materka]

kmaterka retitled this revision from "Activate when both associatedWidget and 
menu are null" to "[KStatusNotifierItem] Allow left click when menu is null".

REPOSITORY
  R289 KNotifications

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

To: kmaterka, davidedmundson, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: Activate when both associatedWidget and menu are null

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> kstatusnotifieritem.cpp:619
>  
> -if (d->associatedWidget == d->menu) {
> +if (d->associatedWidget && d->associatedWidget == d->menu) {
>  d->statusNotifierItemDBus->ContextMenu(pos.x(), pos.y());

This should fix a situation when both are NULL

> kstatusnotifieritemdbus_p.cpp:293
>  {
> -if (m_statusNotifierItem->d->associatedWidget == 
> m_statusNotifierItem->d->menu) {
> -ContextMenu(x, y);

This check is duplicated, it should be safe to remove it here.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, davidedmundson, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: Activate when both associatedWidget and menu are null

[Konrad Materka]

kmaterka added a comment.


  This fixes issue from first comment 
 made by @davidedmundson . 
Second  is still not fixed. Let 
me copy it here:
  
  > We have one menu for the whole tray instance
  >  We create and delete KSNI object in init() and cleanup() which are called 
on show() hide()
  > 
  > The SNI takes ownership of the menu
  > 
  > so for the the second init, we set the SNI to a now deleted menu.
  
  The assumption in QPA is that context menu is persistent, tray icon is not.
  
  There are few way to fix that, but I don't like any of these:
  
  - create new QMenu when old one is destroyed, try to recover menu content
- menu actions are stored safely in m_items, but title, icon and other 
flags are not
- we can have two menus, one for KSNI, second to store values for recovery 
but this is silly
  - create a proxy/delegate to internal QMenu, so that the internal QMenu won't 
be deleted
- overkill? a lot of methods/signals/slots to delegate
  - do not delete menu in KSNI
- not possible, other apps are relaying on this behavior
- add new parameter to skip menu deletion? Isn't it another overkill ad 
unnecessary API change?
  - remove SystemTray support from KdePlatformTheme
- easiest, but...
- Qt has implementation based on DBus, but it behaves differently to KSNI: 
different position of context menu, different implementation of activated, 
scroll is not supported etc
  
  What do you think?

REPOSITORY
  R289 KNotifications

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

To: kmaterka, davidedmundson, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: Activate when both associatedWidget and menu are null

[Konrad Materka]

kmaterka added reviewers: broulik, nicolasfella.

REPOSITORY
  R289 KNotifications

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

To: kmaterka, davidedmundson, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24667: Activate when both associatedWidget and menu are null

[Konrad Materka]

kmaterka created this revision.
kmaterka added a reviewer: davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kmaterka requested review of this revision.

REVISION SUMMARY
  If associatedWidget and menu are the same then instead of "activate"
  action context menu is used. When both are null coparition is always
  true but context menu can't be shown, since it is null.
  It is partial solution for a problem described in BUG 365105.

TEST PLAN
  As described in the bug:
  
  - create QSystemTrayIcon
  - assign QMenu
  - show, hide and show again
  
  Both right click and left click do nothing.
  
  After change left click works

REPOSITORY
  R289 KNotifications

BRANCH
  master

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

AFFECTED FILES
  src/kstatusnotifieritem.cpp
  src/kstatusnotifieritemdbus_p.cpp

To: kmaterka, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns