A QApplication object needs to be instantiated for kio-kdeconnect to work on KDE Neon
ClosedPublic

Authored by eduisters on Nov 5 2018, 6:50 PM.

Details

Summary

kio-kdeconnect fails to start on KDE Neon because it does not instantiate a QApplication object

BUG: 400178

Test Plan

Install on a KDE Neon 5.14 system or VM
Pair an android phone
Browse the phone's filesystem using dolphin

Diff Detail

Repository
R224 KDE Connect
Branch
kde-neon
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4621
Build 4639: arc lint + arc unit
eduisters created this revision.Nov 5 2018, 6:50 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptNov 5 2018, 6:50 PM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
eduisters requested review of this revision.Nov 5 2018, 6:50 PM
eduisters retitled this revision from A QApplication object needs to be instantiated for kio-kdeconnect to work on KDE Neon Embed protocol data via json into the plugin meta data to A QApplication object needs to be instantiated for kio-kdeconnect to work on KDE Neon.Nov 5 2018, 6:51 PM
eduisters edited the summary of this revision. (Show Details)
albertvaka requested changes to this revision.Nov 5 2018, 11:20 PM
albertvaka added a subscriber: albertvaka.

In my opinion this is a regression in KIO and it would be nice to check if it can somehow be fixed there: Upgrading KIO should not break existing apps.

Even if we make a release with this fix, it doesn't guarantee all distros will pick it up.

kio/kiokdeconnect.cpp
155

Also don't change insert to fastInsert, as it requires KIO 5.48 and we want to support older than that.

This revision now requires changes to proceed.Nov 5 2018, 11:20 PM

I can recreate the problem with 1.3 branch build of kdeconnect-kde in a virtualmachine install of KDE neon user edition
The problem remains when building and installing it with the D16692 patch

Building 1.3 branch on KDE neon developer edition I can not recreate the problem. So the problem may be in some other area such as kio framework.

apol added a subscriber: apol.Nov 6 2018, 12:53 AM

In my opinion this is a regression in KIO and it would be nice to check if it can somehow be fixed there: Upgrading KIO should not break existing apps.

Even if we make a release with this fix, it doesn't guarantee all distros will pick it up.

Yet it's the preferred way to list plugins, I'd say we should do it. If KIO regressed that's something for whoever broke it to figure out.

The fastInsert change is indeed unrelated and it would at least need a separate patch (or just not have it if Albert is opposing it).

sredman added a subscriber: sredman.Nov 6 2018, 1:48 AM

This is going to be our cleanest patch ever once we've had everybody and their grandmother look at it. I guess this is what happens when you fix a hot bug! 🙂

I can recreate the problem with 1.3 branch build of kdeconnect-kde in a virtualmachine install of KDE neon user edition
The problem remains when building and installing it with the D16692 patch

Building 1.3 branch on KDE neon developer edition I can not recreate the problem. So the problem may be in some other area such as kio framework.

On my KDE Neon user edition VM, this patch solves the issue 😬

kio/CMakeLists.txt
23

Is there some reason to keep this line as a comment?

eduisters marked 2 inline comments as done.Nov 6 2018, 8:10 AM
eduisters updated this revision to Diff 44954.Nov 6 2018, 10:02 AM

Implemented requested changes

Removed commented out line from CMakeLists.txt
Reverted to using entry.insert to support older KIO versions

albertvaka accepted this revision.Nov 6 2018, 10:52 AM

Let's ship this, but I still think we should see if something can be done on the KIO side to not break old apps.

This revision is now accepted and ready to land.Nov 6 2018, 10:52 AM
This revision was automatically updated to reflect the committed changes.
fvogt added a subscriber: fvogt.Nov 6 2018, 1:27 PM

I'm wondering why this specifically mentions "KDE Neon" both in the title and in the commit message.

Is this workaround/fix only needed on neon or is the message wrong?

I'm wondering why this specifically mentions "KDE Neon" both in the title and in the commit message.

Is this workaround/fix only needed on neon or is the message wrong?

I think only Neon ships frameworks 5.51

fvogt added a comment.Nov 6 2018, 3:22 PM

I'm wondering why this specifically mentions "KDE Neon" both in the title and in the commit message.

Is this workaround/fix only needed on neon or is the message wrong?

I think only Neon ships frameworks 5.51

Why would you think that? 5.51 got released almost a month ago.

I'm wondering why this specifically mentions "KDE Neon" both in the title and in the commit message.

Is this workaround/fix only needed on neon or is the message wrong?

I think only Neon ships frameworks 5.51

Why would you think that? 5.51 got released almost a month ago.

Of curse I don't know every distro in the planet, but we got like a gazillion bug reports and I would say all of them are from Neon users, so this fix was quite directed to them.

See: https://bugs.kde.org/show_bug.cgi?id=400178

You can rename it, although it has been merged already.

ngraham added a subscriber: ngraham.Nov 6 2018, 3:32 PM

The point is, there is no reason to reference distos (which are downstream from us) when we fix issues here. It does not matter that many or even most reports came from Neon. All rolling release distros have Frameworks 5.51, so it certainly was not just Neon users experiencing the issue.

wbauer added a subscriber: wbauer.EditedNov 6 2018, 7:07 PM

The point is, there is no reason to reference distos (which are downstream from us) when we fix issues here. It does not matter that many or even most reports came from Neon. All rolling release distros have Frameworks 5.51, so it certainly was not just Neon users experiencing the issue.

IIANM, the crash should be fixed by this commit in kcrash:
https://cgit.kde.org/kcrash.git/commit/?id=6dbd0edec10f65ff00341ccca94a052bbd55a876
( see also https://mail.kde.org/pipermail/release-team/2018-October/011106.html )

Maybe that is not included yet in KDE Neon for some reason?
IIRC this needed a respin on release day: https://mail.kde.org/pipermail/release-team/2018-October/011108.html

Edit: Indeed, AFAICS Neon's kcrash package seems to be from Oct. 12th...
https://archive.neon.kde.org/user/pool/main/k/kcrash/

fvogt added a comment.Nov 11 2018, 4:32 PM

Edit: Indeed, AFAICS Neon's kcrash package seems to be from Oct. 12th...

https://archive.neon.kde.org/user/pool/main/k/kcrash/

I can confirm that the actual kcrash 5.51 release does not need QCoreApplication.

So only neon is affected as it ships a pre-release tarball with the fix missing, so this commit should be reverted.