Install kontact's kmailplugin and summaryplugin into kontact5, with JSON metadata.
ClosedPublic

Authored by dfaure on Apr 6 2020, 7:24 AM.

Details

Summary

This will allow kontact to use KPluginLoader one day.

Test Plan

rm $prefix/lib64/plugins/kontact_kmailplugin.so ; make install
kontact still finds the kmail plugin

Diff Detail

Repository
R206 KMail
Branch
kontact_kmailplugin
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24885
Build 24903: arc lint + arc unit
dfaure created this revision.Apr 6 2020, 7:24 AM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 6 2020, 7:24 AM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
dfaure requested review of this revision.Apr 6 2020, 7:24 AM
dfaure updated this revision to Diff 79446.Apr 6 2020, 7:33 AM
dfaure edited the summary of this revision. (Show Details)

Fix commit message, those aren't KParts but kontact plugins.

dfaure updated this revision to Diff 79448.Apr 6 2020, 7:35 AM

Increase kontactinterface version dependency

dfaure updated this revision to Diff 79449.Apr 6 2020, 7:40 AM
dfaure retitled this revision from Install kontact_kmailplugin into kf5/kontactparts, with JSON metadata. to Install kontact_kmailplugin into kf5/kontactplugins, with JSON metadata..

Actually if they aren't KParts let's call the dir kontactplugins for consistency.

Sorry for the noise

mlaurent accepted this revision.Apr 6 2020, 11:09 AM
This revision is now accepted and ready to land.Apr 6 2020, 11:09 AM

Comment from the side of playing field: why "kontactplugins" and not just "kontact"? Given this is below some other "plugins/" in the path, that extra "plugin" seems duplicated and only makes path longer with no obvious reason.

Fair point. But then again, there are different sorts of things that kontact loads. These are actually called "kontactplugin"s everywhere (source dirs, Kontact/Plugin servicetype).

But for instance KCM plugins for kontact don't belong in there, and would go into some different subdir. Half-good half-bad example though because they are normal KCM modules so they'll probably all go together with other KCM modules into some kcm subdir.

Then there are the actual KParts, which shouldn't go in there either.

Yeah this is about "kontactplugin" vs "plugin for kontact" (for N types of plugins), possibly confusing, I agree.

I think a too-generically named subdir creates a risk that people put other kinds of things in there, that's my main idea behind this name.

I think a too-generically named subdir creates a risk that people put other kinds of things in there, that's my main idea behind this name.

Comparing "lib/qt5/plugins/kf5/kontakt" & "lib/qt5/plugins/kf5/kontaktplugins", I would not think that it help making sure that people see this as plugins "for" kontact, not plugins "from" kontakt. Given also that the "for" aspect is already a thing for any other subdirs in the plugins/ folder.
(Personally I would even use "lib/qt5/plugins/kontakt5" instead, as this is not about plugins for anything from "kf5". And the "qt5" in the path is only because of technology needs, due to using Qt plugin loading logic, where any non-qt5 path would need more setup in normal system installations. Otherwise one would even expect "lib/plugins/kontakt5").

I see the point of following a naming pattern already seen elsewhere in the code. Though like the service type already modifies it, splitting the name and inserting a / -> "Kontact/Plugin", I would argue to do the same here, as in reusing the "plugins" from earlier in the path, and using the "kontact" only at the lowest ordering level.

My 2 cents as outsider :)

kossebau added a comment.EditedApr 6 2020, 12:56 PM

Oh, and looking at the currently proposed "lib/qt5/plugins/kf5/kontaktplugins/kontact_kmailplugin.so", there is even 3x "plugin" in a binary path ;)

"lib/qt5/plugins/kontakt5/kontact_kmail.so" is shorter and still would make sense, no?

Edit: even the additional "kontact_" prefix could be removed:
"lib/qt5/plugins/kontakt5/kmail.so".
I think that would be all that is needed to make unique and self-describing plugin binary naming pattern. Why make names more complicated than they need to be? :)

dfaure added a comment.Apr 6 2020, 2:42 PM

Ah, naming, we can spend all day on it ;)

Yep the kf5 is a technology reason, to avoid mixing with kf6-based plugins.
On your system the qt5 does that already, but that's not the default layout.
With a self-built Qt and KDE I get <kdeprefix>/lib64/plugins and <qtprefix>/plugins, no qt5 in there.

Indeed kontact5 would do that too, assuming kontact keeps using the Qt/KF major version number for itself.
(BTW it's kontact, not kontakt, the second 'k' would be redundant, LOL)

I don't really like kmail.so. Yes, once installed it's kind of clear what it is, but in the CMakeLists it would mean a target "kmail" (ouch) (yes I know one can change the executable name, but that's just more tricks and confusion, let's keep it simple). Also in the builddir, bin/kontact_kmailplugin.so is pretty clear what it is.

I think clarity of CMakeLists.txt and builddir are important too, even at the price of a bit of harmless duplication in the install dir.

Yes, "kcontactt5" with the suffix 5 is proposed as technology namespace (though if the actual executable binaries are not namespaced as it seems they are not?, co-installability is not possible anyway).
Using "kf5/" as an intermediate level for technology reasons would make kontact stay out from the other applications though. IMHO this should be really left as prefix for plugins extending KF modules only.
Cmp. Okular, KDevelop, Falkon, etc or Plasma.

(And I mentioned the "qt5/" prefix only in case it was not obvious to some why qt5 in in the system path, to make clear there is no real need besides simple setup).

So I really would like you to consider switching to "kontact5" or "kontact5/plugins" instead of "kf5/kontactplugins". To not make the plugins/ subdir more random in the patterns used there.

dfaure updated this revision to Diff 79531.Apr 6 2020, 10:21 PM

plugins/kontact5

dfaure updated this revision to Diff 79537.Apr 6 2020, 10:27 PM
dfaure retitled this revision from Install kontact_kmailplugin into kf5/kontactplugins, with JSON metadata. to Install kontact's kmailplugin and summaryplugin into kontact5, with JSON metadata..
dfaure removed a subscriber: kossebau.

and the last plugin: summaryplugin

mlaurent accepted this revision.Apr 8 2020, 4:50 AM
dfaure closed this revision.Apr 8 2020, 9:52 PM