Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[Kai Uwe Broulik]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/#review99953
---



Either this patch or some other patch in the kplugin series broke the 
"externalData" for many applets!

Basically d->init(QString(), args.mid(2)); creates the applet with 
(QVariant(QVariantList, ()) argument and thus, since the args passed to init 
isn't empty, it emits onExternalData on the applet with this.

Most applets don't do anything with it but for example FolderView then sets 
this as its url ("cannot assign QJSValue to QString") and the FolderView 
becomes blank on next (or 2nd after that) startup.

- Kai Uwe Broulik


On Okt. 10, 2016, 2:28 nachm., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129102/
> ---
> 
> (Updated Okt. 10, 2016, 2:28 nachm.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Makes it possible to use plugins that offer a metadata.json file.
> Define the service type when falling back to the desktop file parser, so the 
> type system is proper.
> Don't destroy a KPluginMetadata tuple to instanciate it right away.
> 
> 
> Diffs
> -
> 
>   src/plasma/applet.cpp 5eb529c 
>   src/plasma/containment.h 0b7a3ef 
>   src/plasma/containment.cpp 1840f24 
>   src/plasma/pluginloader.h 566461d 
>   src/plasma/pluginloader.cpp db2b2c9 
>   src/plasma/private/applet_p.cpp fafb450 
> 
> Diff: https://git.reviewboard.kde.org/r/129102/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass, plasma still loads, even with RR #129103.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/
---

(Updated Oct. 10, 2016, 2:28 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Plasma.


Changes
---

Submitted with commit 0ebe2ca1fa50670366cf130db66b5774b7249da6 by Aleix Pol to 
branch master.


Repository: plasma-framework


Description
---

Makes it possible to use plugins that offer a metadata.json file.
Define the service type when falling back to the desktop file parser, so the 
type system is proper.
Don't destroy a KPluginMetadata tuple to instanciate it right away.


Diffs
-

  src/plasma/applet.cpp 5eb529c 
  src/plasma/containment.h 0b7a3ef 
  src/plasma/containment.cpp 1840f24 
  src/plasma/pluginloader.h 566461d 
  src/plasma/pluginloader.cpp db2b2c9 
  src/plasma/private/applet_p.cpp fafb450 

Diff: https://git.reviewboard.kde.org/r/129102/diff/


Testing
---

Tests still pass, plasma still loads, even with RR #129103.


Thanks,

Aleix Pol Gonzalez

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[Marco Martin]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/#review99906
---




src/plasma/pluginloader.cpp (line 539)


right, so plugininfos for some releases still


- Marco Martin


On Oct. 7, 2016, 3:23 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129102/
> ---
> 
> (Updated Oct. 7, 2016, 3:23 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Makes it possible to use plugins that offer a metadata.json file.
> Define the service type when falling back to the desktop file parser, so the 
> type system is proper.
> Don't destroy a KPluginMetadata tuple to instanciate it right away.
> 
> 
> Diffs
> -
> 
>   src/plasma/applet.cpp 5eb529c 
>   src/plasma/containment.h 0b7a3ef 
>   src/plasma/containment.cpp 1840f24 
>   src/plasma/pluginloader.h 566461d 
>   src/plasma/pluginloader.cpp db2b2c9 
>   src/plasma/private/applet_p.cpp fafb450 
> 
> Diff: https://git.reviewboard.kde.org/r/129102/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass, plasma still loads, even with RR #129103.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[David Edmundson]

> On Oct. 10, 2016, 11:23 a.m., Marco Martin wrote:
> > src/plasma/pluginloader.cpp, line 561
> > 
> >
> > this note isn't valid anymore?

yes and no.

It was, even in Plasma 5.8 the plasmoids runner used it. 

After all of Aleix's patches it is not valid, but changing it would crash all 
previous releases.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/#review99902
---


On Oct. 7, 2016, 3:23 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129102/
> ---
> 
> (Updated Oct. 7, 2016, 3:23 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Makes it possible to use plugins that offer a metadata.json file.
> Define the service type when falling back to the desktop file parser, so the 
> type system is proper.
> Don't destroy a KPluginMetadata tuple to instanciate it right away.
> 
> 
> Diffs
> -
> 
>   src/plasma/applet.cpp 5eb529c 
>   src/plasma/containment.h 0b7a3ef 
>   src/plasma/containment.cpp 1840f24 
>   src/plasma/pluginloader.h 566461d 
>   src/plasma/pluginloader.cpp db2b2c9 
>   src/plasma/private/applet_p.cpp fafb450 
> 
> Diff: https://git.reviewboard.kde.org/r/129102/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass, plasma still loads, even with RR #129103.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[Marco Martin]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/#review99902
---


Fix it, then Ship it!




tested several cases, normal load, first startup, plasmoidviewer and 
plasmawindowed, all seems fine


src/plasma/pluginloader.cpp (line 539)


this note isn't valid anymore?


- Marco Martin


On Oct. 7, 2016, 3:23 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129102/
> ---
> 
> (Updated Oct. 7, 2016, 3:23 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Makes it possible to use plugins that offer a metadata.json file.
> Define the service type when falling back to the desktop file parser, so the 
> type system is proper.
> Don't destroy a KPluginMetadata tuple to instanciate it right away.
> 
> 
> Diffs
> -
> 
>   src/plasma/applet.cpp 5eb529c 
>   src/plasma/containment.h 0b7a3ef 
>   src/plasma/containment.cpp 1840f24 
>   src/plasma/pluginloader.h 566461d 
>   src/plasma/pluginloader.cpp db2b2c9 
>   src/plasma/private/applet_p.cpp fafb450 
> 
> Diff: https://git.reviewboard.kde.org/r/129102/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass, plasma still loads, even with RR #129103.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/#review99900
---



+1

- David Edmundson


On Oct. 7, 2016, 3:23 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129102/
> ---
> 
> (Updated Oct. 7, 2016, 3:23 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Makes it possible to use plugins that offer a metadata.json file.
> Define the service type when falling back to the desktop file parser, so the 
> type system is proper.
> Don't destroy a KPluginMetadata tuple to instanciate it right away.
> 
> 
> Diffs
> -
> 
>   src/plasma/applet.cpp 5eb529c 
>   src/plasma/containment.h 0b7a3ef 
>   src/plasma/containment.cpp 1840f24 
>   src/plasma/pluginloader.h 566461d 
>   src/plasma/pluginloader.cpp db2b2c9 
>   src/plasma/private/applet_p.cpp fafb450 
> 
> Diff: https://git.reviewboard.kde.org/r/129102/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass, plasma still loads, even with RR #129103.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/
---

(Updated Oct. 7, 2016, 5:23 p.m.)


Review request for KDE Frameworks and Plasma.


Changes
---

address David's remarks


Repository: plasma-framework


Description
---

Makes it possible to use plugins that offer a metadata.json file.
Define the service type when falling back to the desktop file parser, so the 
type system is proper.
Don't destroy a KPluginMetadata tuple to instanciate it right away.


Diffs (updated)
-

  src/plasma/applet.cpp 5eb529c 
  src/plasma/containment.h 0b7a3ef 
  src/plasma/containment.cpp 1840f24 
  src/plasma/pluginloader.h 566461d 
  src/plasma/pluginloader.cpp db2b2c9 
  src/plasma/private/applet_p.cpp fafb450 

Diff: https://git.reviewboard.kde.org/r/129102/diff/


Testing
---

Tests still pass, plasma still loads, even with RR #129103.


Thanks,

Aleix Pol Gonzalez

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/#review99835
---




src/plasma/applet.cpp (lines 96 - 97)


Whilst this change looks sensible, it's not related.



src/plasma/pluginloader.cpp 


This code path effectively breaks the the custom loader.


- David Edmundson


On Oct. 5, 2016, 2:18 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129102/
> ---
> 
> (Updated Oct. 5, 2016, 2:18 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Makes it possible to use plugins that offer a metadata.json file.
> Define the service type when falling back to the desktop file parser, so the 
> type system is proper.
> Don't destroy a KPluginMetadata tuple to instanciate it right away.
> 
> 
> Diffs
> -
> 
>   src/plasma/applet.cpp 5eb529c 
>   src/plasma/containment.h 0b7a3ef 
>   src/plasma/containment.cpp 1840f24 
>   src/plasma/pluginloader.h 566461d 
>   src/plasma/pluginloader.cpp db2b2c9 
>   src/plasma/private/applet_p.cpp fafb450 
> 
> Diff: https://git.reviewboard.kde.org/r/129102/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass, plasma still loads, even with RR #129103.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/#review99834
---



Ping?

- Aleix Pol Gonzalez


On Oct. 5, 2016, 4:18 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129102/
> ---
> 
> (Updated Oct. 5, 2016, 4:18 a.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> Makes it possible to use plugins that offer a metadata.json file.
> Define the service type when falling back to the desktop file parser, so the 
> type system is proper.
> Don't destroy a KPluginMetadata tuple to instanciate it right away.
> 
> 
> Diffs
> -
> 
>   src/plasma/applet.cpp 5eb529c 
>   src/plasma/containment.h 0b7a3ef 
>   src/plasma/containment.cpp 1840f24 
>   src/plasma/pluginloader.h 566461d 
>   src/plasma/pluginloader.cpp db2b2c9 
>   src/plasma/private/applet_p.cpp fafb450 
> 
> Diff: https://git.reviewboard.kde.org/r/129102/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass, plasma still loads, even with RR #129103.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/
---

(Updated Oct. 5, 2016, 4:18 a.m.)


Review request for KDE Frameworks and Plasma.


Changes
---

fix system tray, turns out we needed the weird QVariantList constructor.


Repository: plasma-framework


Description
---

Makes it possible to use plugins that offer a metadata.json file.
Define the service type when falling back to the desktop file parser, so the 
type system is proper.
Don't destroy a KPluginMetadata tuple to instanciate it right away.


Diffs (updated)
-

  src/plasma/applet.cpp 5eb529c 
  src/plasma/containment.h 0b7a3ef 
  src/plasma/containment.cpp 1840f24 
  src/plasma/pluginloader.h 566461d 
  src/plasma/pluginloader.cpp db2b2c9 
  src/plasma/private/applet_p.cpp fafb450 

Diff: https://git.reviewboard.kde.org/r/129102/diff/


Testing
---

Tests still pass, plasma still loads, even with RR #129103.


Thanks,

Aleix Pol Gonzalez

Re: Review Request 129102: Don't enforce metadata.desktop, cleanup constructor

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129102/
---

(Updated Oct. 5, 2016, 1:58 a.m.)


Review request for KDE Frameworks and Plasma.


Repository: plasma-framework


Description
---

Makes it possible to use plugins that offer a metadata.json file.
Define the service type when falling back to the desktop file parser, so the 
type system is proper.
Don't destroy a KPluginMetadata tuple to instanciate it right away.


Diffs (updated)
-

  src/plasma/applet.cpp 5eb529c 
  src/plasma/containment.h 0b7a3ef 
  src/plasma/containment.cpp 1840f24 
  src/plasma/pluginloader.h 566461d 
  src/plasma/pluginloader.cpp db2b2c9 

Diff: https://git.reviewboard.kde.org/r/129102/diff/


Testing
---

Tests still pass, plasma still loads, even with RR #129103.


Thanks,

Aleix Pol Gonzalez