Don't load KDE platform module in kglobalaccel5
AbandonedPublic

Authored by hein on Dec 5 2017, 3:45 PM.

Details

Reviewers
davidedmundson
mart
graesslin
Group Reviewers
Plasma
Summary

I'm not as sure on this one as I was with D9194 and the activity
manager daemon, but from code skimming I can't see anything that
requires Plasma settings, and this provides a small speedup.

Diff Detail

Repository
R268 KGlobalAccel
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
hein created this revision.Dec 5 2017, 3:45 PM
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptDec 5 2017, 3:45 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
hein requested review of this revision.Dec 5 2017, 3:45 PM

I'm against a risky change here. Especially as this becomes irrelevant with Wayland.

apol added a subscriber: apol.Dec 10 2017, 11:28 PM

I'm against a risky change here. Especially as this becomes irrelevant with Wayland.

What makes it risky?

In D9207#178257, @apol wrote:

I'm against a risky change here. Especially as this becomes irrelevant with Wayland.

What makes it risky?

The description: "I'm not as sure on this..." and "but from code skimming I can't see anything that
requires Plasma settings". So what I took from it is that it's unknown whether something might break. And that's what I consider risky. Especially in an application in frameworks which affects the stable Plasma releases. If kglobalaccel breaks all users are heavily affected. Due to that I'm extremely afraid of anything which could break.

hein added a reviewer: graesslin.EditedDec 12 2017, 7:06 AM
hein removed subscribers: apol, graesslin.

I consider this patch in some sense a micro-optimization and code hygiene. I think given our time in the release cycle it'd be OK to apply and essentially see what if anything breaks. However I also won't feel bad if it's rejected. For efficiency: If you want to reject it, please commandeer and abandon it for me. I recognize you as kglobalaccel maintainer and am fine with it.

Edit: Not sure why Phab did the reviewer/subscriber changes, sorry. I didn't change the fields manually.

The description: "I'm not as sure on this..." and "but from code skimming I can't see anything that

I went through this code thoroughly recently for a change I made.
We know we don't have any graphical widgets used anywhere; the use of QGuiApplication shows we're not showing in any widgets anywhere, and we can be confident that we're not having a secret manual or QML interface somewhere with any other visual things.

That leaves 3 things:

  • icons (I checked, this and dependencies are fine)
  • QStylehints (also fine)
  • use of Qt standard keys. This is at least used client side, but I don't think on the server side. It's the one part you'll need to "prove".
mart added a comment.Feb 6 2018, 10:10 AM

can this be resurrected?

hein added a comment.Feb 6 2018, 10:18 AM

Well, the maintainer spoke out against it, so not much I can do.

In D9207#201848, @hein wrote:

Well, the maintainer spoke out against it, so not much I can do.

Please be aware that I am no longer the maintainer of kglobalaccel. Nevertheless I recommend against it as the risk of breakage is high especially as nobody notices breakage during the frameworks dev cycle.

hein abandoned this revision.Feb 7 2018, 6:33 AM

As mentioned, I'm OK with abandoning it. I think the change is hygienic, but it's also a micro-optimization.