D9207: Don't load KDE platform module in kglobalaccel5
hein abandoned this revision. hein added a comment. As mentioned, I'm OK with abandoning it. I think the change is hygienic, but it's also a micro-optimization. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D9207 To: hein, #plasma, davidedmundson, mart, graesslin Cc: plasma-devel, #frameworks, michaelh, ZrenBot, ngraham, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9207: Don't load KDE platform module in kglobalaccel5
graesslin added a comment. In https://phabricator.kde.org/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. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D9207 To: hein, #plasma, davidedmundson, mart, graesslin Cc: plasma-devel, #frameworks, michaelh, ZrenBot, ngraham, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9207: Don't load KDE platform module in kglobalaccel5
hein added a comment. Well, the maintainer spoke out against it, so not much I can do. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D9207 To: hein, #plasma, davidedmundson, mart, graesslin Cc: plasma-devel, #frameworks, michaelh, ZrenBot, ngraham, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9207: Don't load KDE platform module in kglobalaccel5
mart added a comment. can this be resurrected? REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D9207 To: hein, #plasma, davidedmundson, mart, graesslin Cc: plasma-devel, #frameworks, michaelh, ZrenBot, ngraham, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9207: Don't load KDE platform module in kglobalaccel5
davidedmundson added a comment. > 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". REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D9207 To: hein, #plasma, davidedmundson, mart, graesslin Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9207: Don't load KDE platform module in kglobalaccel5
hein removed subscribers: apol, graesslin. hein added a reviewer: graesslin. hein added a comment. I consider this patch in some sense a micro-optimization. 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. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D9207 To: hein, #plasma, davidedmundson, mart, graesslin Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9207: Don't load KDE platform module in kglobalaccel5
graesslin added a comment. In https://phabricator.kde.org/D9207#178257, @apol wrote: > In https://phabricator.kde.org/D9207#176322, @graesslin 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. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D9207 To: hein, #plasma, davidedmundson, mart Cc: apol, graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D9207: Don't load KDE platform module in kglobalaccel5
apol added a comment. In https://phabricator.kde.org/D9207#176322, @graesslin wrote: > I'm against a risky change here. Especially as this becomes irrelevant with Wayland. What makes it risky? REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D9207 To: hein, #plasma, davidedmundson, mart Cc: apol, graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D9207: Don't load KDE platform module in kglobalaccel5
graesslin added a comment. I'm against a risky change here. Especially as this becomes irrelevant with Wayland. REPOSITORY R268 KGlobalAccel REVISION DETAIL https://phabricator.kde.org/D9207 To: hein, #plasma, davidedmundson, mart Cc: graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9207: Don't load KDE platform module in kglobalaccel5
hein created this revision. hein added reviewers: Plasma, davidedmundson, mart. Restricted Application added projects: Plasma, Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY I'm not as sure on this one as I was with https://phabricator.kde.org/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. REPOSITORY R268 KGlobalAccel BRANCH master REVISION DETAIL https://phabricator.kde.org/D9207 AFFECTED FILES src/runtime/main.cpp To: hein, #plasma, davidedmundson, mart Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
