Build kdeconnect on sailfish and port some simple plugins
ClosedPublic

Authored by piggz on Feb 20 2018, 11:59 PM.

Details

Summary

Below is a lost of the commits, but, in summary
Port the build system for Sailfish, which means selectively building only the bits we need/can, and only against the KF5 libs that are available.
Allow to build on Qt 5.6
Switch from knotification to nemo notification (not complete!)
Add a very simple example sailfish app.

Note, there is still much missing functionality. Notifications dont work, pairing sort of works but not really, but when it is paired you can send a ping to the desktop client

Dont build kio for Sailfish

Port core build system

Port daemon buld system

Require CoreAddons on Sailfish

Port plugins build for sailfish and include the ping plugin for now

Final build changes for sailfish.

Disable tests and other not needed parts

Add includes for QCA

Fix build errors on sailfish

Get core/ to build on sailfish

Get interfaces/ to build on sailfish

Build daemon on sailfish

On sailfish, dont install the kcm file

Start port plugin to sailfish

Fixup installed files

Add sfos app

Hack declarative plugin to give a public interface

Build sfos app

Compile declarativeplugin into the sfos app for now

Redefine qAsConst for qt 5.6

Packaging fixes

Use official icon

Package .desktop

Diff Detail

Repository
R224 KDE Connect
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes

@nicolasfella Thanks for pointing me to this. Happyness++

@piggz Hi :) By pure chance some hours before I came across https://github.com/R1tschY/harbour-sailfishconnect, is this some duplicated effort possibly or how does this relate? (no clue yet about KDE Connect details)

Can you give some build instructions? Seeing set(KF5_REQUIRED_COMPONENTS I18n DBusAddons CoreAddons), so not just using plain Qt, my old MarbleMaps@SailfishOS knowledge might not be sufficient :)

Happy to help out soon at least when it comes to MPRIS stuff (given master plan hinted to in D10972)

piggz updated this revision to Diff 28464.Mar 3 2018, 9:05 AM
  • Properly build the declarative plugin
piggz updated this revision to Diff 28465.Mar 3 2018, 9:10 AM
  • Use qasconst.h in notification listener
piggz added a comment.Mar 3 2018, 9:15 AM

Guys, whats your thoughts about this?

Ive marked everything as 'done' so far, and quite happy to have removed the hack which compiled the declarative plugin into the app, and installed it properly. I should have done this from the start!

Also fixed the qAsConst issue by taking the code from qt 5.7. (btw, your min qt version is 5.2, which surely cant work?)

Perhaps the next change could be to abstract out the notifications code, so there was just one set of ifdef's for the different notification libs, but I dont want this to get too big a diff.

mtijink requested changes to this revision.Mar 3 2018, 3:03 PM

I got some build errors, which I've added a comment for.

Nice to see KDE Connect spreading to a different platform. Do you have some screenshots?

CMakeLists.txt
50

Is there a reason you didn't add -DQT_NO_KEYWORDS?

core/qasconst.h
4 ↗(On Diff #28465)

This definitely needs the copyright notice from Qt. Also, a note were it came from would be good.

plugins/battery/CMakeLists.txt
7 ↗(On Diff #28465)

I think this should be KF5::Notifications.

plugins/ping/CMakeLists.txt
7 ↗(On Diff #28465)

I think this should be KF5::Notifications.

plugins/sendnotifications/sendnotificationsplugin.cpp
29 ↗(On Diff #28465)

Why is this separate file needed?

sfos/kdeconnect-sfos.desktop
13

This line shouldn't be here, right?

sfos/rpm/kdeconnect-sfos.changes.in
2

I don't think rpm files normally live inside a KDE application repository. Is it possible to maintain these files in a separate repository?

This revision now requires changes to proceed.Mar 3 2018, 3:03 PM
piggz added a comment.Mar 3 2018, 7:41 PM

@nicolasfella Thanks for pointing me to this. Happyness++

@piggz Hi :) By pure chance some hours before I came across https://github.com/R1tschY/harbour-sailfishconnect, is this some duplicated effort possibly or how does this relate? (no clue yet about KDE Connect details)

Can you give some build instructions? Seeing set(KF5_REQUIRED_COMPONENTS I18n DBusAddons CoreAddons), so not just using plain Qt, my old MarbleMaps@SailfishOS knowledge might not be sufficient :)

Happy to help out soon at least when it comes to MPRIS stuff (given master plan hinted to in D10972)

@kossebau you can build by using the sfossdk and the supplied .spec file, so long as you have the deps from http://repo.merproject.org/obs/home:/piggz:/kf5/sailfishos_2.1.0.9_latest_armv7hl/ and https://build.merproject.org/project/show/nemo:devel:mw

How do you think this will work compared to a fully stanadalone client like the other you found?

piggz added a comment.Mar 3 2018, 7:43 PM

I got some build errors, which I've added a comment for.

Nice to see KDE Connect spreading to a different platform. Do you have some screenshots?

If fix up the issues ... there is a screenshot of my desktop getting notifications here, https://twitter.com/adampigg/status/968600024316809218 ... i'll get some shots of the (currently very bare) client app later

In my theory it should work this way:

The daemon/ folder contains stuff for the desktop daemon, so it should not be included on sfos at all and should not need #ifdefs in the code.
The Sailfish daemon itself entirely lives within the sfos/ folder

piggz marked 5 inline comments as done.Mar 3 2018, 10:14 PM
piggz added inline comments.
CMakeLists.txt
50

Yes, the nemo notification library uses signal/slot terminology instead of Q_SIGNAL/Q_SLOT

plugins/sendnotifications/sendnotificationsplugin.cpp
29 ↗(On Diff #28465)

It is just so that on sfos I can set the plugin to be enabled by default. Is there a better way?

sfos/kdeconnect-sfos.desktop
13

It is part of the default template, but ive commented it out.

apol added a comment.Mar 4 2018, 1:01 AM

I'm really concerned about how this is removing compilation of most plugins for sailfish. This means that this whole dance will be needed there once more for each plugin.

CMakeLists.txt
44

Why?

core/daemon.cpp
45
interfaces/notificationsmodel.cpp
34

Use your fancy header?

piggz updated this revision to Diff 28564.Mar 4 2018, 9:30 AM
piggz marked 8 inline comments as done.
  • Address review comments
  • Remove unnescessary changes
piggz added inline comments.Mar 4 2018, 9:33 AM
CMakeLists.txt
44

See above, the nemo notification library uses signal/slot terminology, so requires this. Normal builds retain -DQT_NO_KEYWORDS

core/daemon.cpp
45

Ive replaced my version with the one from kdevelop

sfos/rpm/kdeconnect-sfos.changes.in
2

Ok, i'll remove the rpm files, and put them on the mer obs instead

apol added inline comments.Mar 5 2018, 12:44 AM
sfos/rpm/kdeconnect-sfos.changes.in
2

I think it's fine to have it here to be honest. This way if something in the application changes the packaging can be tweaked.
For 1:1 mappings it's okay, of course it wouldn't scale to have 25 linux distros packaged here. But it's kind of how we also have the AndroidManifest.xml file in the kdeconnect-android repo.

apol added inline comments.Mar 5 2018, 12:53 AM
plugins/battery/batteryplugin.cpp
76

Could we use something like Daemon::reportError to abstract such notifications?

Something like`Daemon::instance()->sendNotification(notificationName, icon, title, text)` could also be used in other places.

Another thing to consider is to adapt knotifications to work on sailfish would be great (solving this problem elsewhere too). Or maybe considering using snorenotify. Another possibility would be to have a proxy for xdg notifications for sailfish.

plugins/sendnotifications/sendnotificationsplugin.cpp
29 ↗(On Diff #28465)

There should be. Having this line is ugly as is, let alone having to ifdef it on every plugin.

albertvaka added inline comments.
plugins/CMakeLists.txt
10

Isn't clipboard sync working out of the box? It just uses QClipboard, which should be there in SFOS.

plugins/notifications/notificationsdbusinterface.cpp
34

Inside qtcompat this is already checked, don't do it again.

kossebau added a comment.EditedMar 17 2018, 6:07 PM

Has there been any talk to other Qt5-based projects which might be interested in sharing KDE Connect efforts? E.g. AsteroidOS might come into mind or Unity8, who both might be happy to be able to reuse at least the kdeconnect core lib, and then add their own UI to that.

What would you think about some kdeconnect-qtcore which get the content of "core/", ideally even without k18n (error messages could be done with UI-textless ids) and some hooks for config storage system.

Has there been any talk to other Qt5-based projects which might be interested in sharing KDE Connect efforts? E.g. AsteroidOS might come into mind or Unity8, who both might be happy to be able to reuse at least the kdeconnect core lib, and then add their own UI to that.

What would you think about some kdeconnect-qtcore which get the content of "core/", ideally even without k18n (error messages could be done with UI-textless ids) and some hooks for config storage system.

Someone had this idea sometime, but it didn't get much traction. Before we pursue this we should have close collaboration with at least one stakeholder, otherwise we might just waste our time. This could also be beneficial for alternative UIs like @andyholmes Gnome extension. Since he decided not to use the official KDE Connect core it would be nice to hear about his reasoning why he didn't so we know where we have to improve.

I think it could be a idea worth pursuing if we actually find people interested in it

I don't know much about Sailfish, but I heard that SailfishOS 3 with updated Qt is coming. Do you know how fast the rollout can be expected? It it better than Android where months after the release only 1% run the latest major? If version 3 is spreading reasonably fast we might require that and drop the Qt 5.6 hacks entirely

Has there been any talk to other Qt5-based projects which might be interested in sharing KDE Connect efforts? E.g. AsteroidOS might come into mind or Unity8, who both might be happy to be able to reuse at least the kdeconnect core lib, and then add their own UI to that.

What would you think about some kdeconnect-qtcore which get the content of "core/", ideally even without k18n (error messages could be done with UI-textless ids) and some hooks for config storage system.

Someone had this idea sometime, but it didn't get much traction.

Meanwhile found it (https://github.com/Bjoe/KConnectClientLib), next to @albertvaka 's take on it (https://albertvaka.wordpress.com/2014/10/23/kde-connect-feature-brainstorming/#comment-1599):

Actually I think this changes should be merged into the main git repo, so we don’t have to maintain two versions of the code. Right now the main repo is already split in more small parts, and for example the kded related code is already in a different directory. The last goal should be to have a Qt-only repo with a backend without user interface, and a different repo for the UI and KDE integration (plus another one for Gnome, Unity…), that should use DBus to communicate with the desktop-agnostic backend. This separation is quite done already, but not completely.

Which I understand as principle support for such a qtcore repo, at least at that time :)

Before we pursue this we should have close collaboration with at least one stakeholder, otherwise we might just waste our time.

Agreed.

This could also be beneficial for alternative UIs like @andyholmes Gnome extension. Since he decided not to use the official KDE Connect core it would be nice to hear about his reasoning why he didn't so we know where we have to improve.

I would assume the "improvement" needed would be to drop the usage of Qt libs, so possibly not to happen ;) BTW, actually I was surprised to see the KDE Connect Android not being done with Qt, but with native Java (or whatever the oficial language name is?). Similar motivation, I assume.

I think it could be a idea worth pursuing if we actually find people interested in it

If I look at this patch as well as the recent parallel effort https://github.com/R1tschY/harbour-sailfishconnect (who so created a fork for their needs) I would think some people are showing up now.Including myself, slowly getting my fingers dirty here :) Never managed to release something for Sailfish OS, only did some experiments with writing an app variant of Marble Maps for SailfishOS (as well as minimal rebuilding of Sailfish Office with Calligra code), so pretty interested to finally get something done and released, also for my usage needs. And with that experience, I also very much would prefer to have some non-QWidget-dependent code as common shared base.

For now though I would agree it is best to do the SFOS variant in the same repo. But then see to slowly prepare splitting things out. Not my call, just my 2 cents and showing up willing hands :) Time to get my SFOS SDK updated and working again, so I can do more than commenting. Even more as my latest commits made this patch no longer apply.

BTW, actually I was surprised to see the KDE Connect Android not being done with Qt, but with native Java (or whatever the oficial language name is?). Similar motivation, I assume.

I think this way it's easier to interact with the Android API which we rely on heavily.

Regarding the future of the SFOS port:
It would be great if we could get SnoreNotify on Sailfish. @kossebau would you consider trying to get a backend for that working?

Regarding the core lib thing: It would be great to get potentially interested people in a (virtual) room to talk about it

nicolasfella requested changes to this revision.Mar 18 2018, 11:47 PM

Regarding the future of this patch:
The Cmake stuff needs rebasing to master and you should make use of your notification helper

Can the Kirigami app be used on SFOS instead of creating a new one?

This revision now requires changes to proceed.Mar 18 2018, 11:47 PM

Regarding the future of the SFOS port:
It would be great if we could get SnoreNotify on Sailfish. @kossebau would you consider trying to get a backend for that working?

No promises, but note made. Never heard of that before.

Regarding the core lib thing: It would be great to get potentially interested people in a (virtual) room to talk about it

Note made as well :)

Starting to get it building, virtual sdk machine for now :)
Though had run into this:

CMake Error: File /home/src1/kdeconnect-kde/sfos/org.kde.kdeconnect.service.in does not exist.
CMake Error at sfos/CMakeLists.txt:24 (configure_file):
  configure_file Problem configuring file

Added locally a file sfos/org.kde.kdeconnect.service.in with this content:

[D-BUS Service]
Name=org.kde.kdeconnect
Exec=@DBUS_SERVICES_INSTALL_DIRR@/kdeconnectd

Still have to deploy to emulator, and then go for the real arch. Hoping that is without bigger hurdles as well, so I am soon ready to join development.

@piggz : I could not find this yet on merproject OBS, do you have a package spec/yaml file somewhere handy, so I can spare myself drafting one? :)

piggz added a comment.Mar 21 2018, 2:06 PM

its not on obs yet. there is a spec in an earlier version of the diff, also take a look on my github maybe

its not on obs yet. there is a spec in an earlier version of the diff, also take a look on my github maybe

Ah, missed that, thanks.

BTW, would you mind if I try to rebase this patch to latest master and then update the diff? Or are you already working on that?

piggz added a comment.Mar 21 2018, 4:06 PM

Feel free, you'll have to implement the simple notification method in the Damon.

albertvaka requested changes to this revision.Mar 23 2018, 6:58 PM
albertvaka added inline comments.
core/pluginloader.cpp
34

Can we target latest Sailfish, so we don't need to keep compatibility with Qt 5.6?

plugins/sendnotifications/sendnotificationsplugin.cpp
29 ↗(On Diff #28465)

+1

piggz marked an inline comment as done.Mar 27 2018, 5:41 PM
piggz added inline comments.
core/pluginloader.cpp
34

latest sailfish _is_ 5.6! ;)

plugins/CMakeLists.txt
10

i havnt tried....will do

Ping? It needs to be rebased to current master and then I think it can go in

Restricted Application added a subscriber: kdeconnect. · View Herald TranscriptMay 13 2018, 11:26 AM
piggz marked an inline comment as done.May 14 2018, 6:08 AM

Ping? It needs to be rebased to current master and then I think it can go in

Yep, sorry, been working on other stuff. There was one outstanding issue, I added a plugin JSON file with a one line change, ideally I'd do this programmatically, any suggestions? I'll make some time this evening.

piggz marked an inline comment as done.Jun 22 2018, 1:13 PM
piggz updated this revision to Diff 38274.Jul 23 2018, 7:14 PM
  • Address review comments
  • Move rpm folder
  • Remove test code
  • Fix icon
  • Improve cover item
  • Move sailfishdaemon to seperate file
  • Port battery plugin to sailfish
  • Copy qascont from qt 5.7
  • Properly build the declarative plugin
  • Address review comments
  • Remove unnescessary changes
  • Add features to app
  • Tidy up plugin items
  • Add rpm files for sailfish
  • Merge branch 'pgz-sfos-initial' of https://github.com/piggz/kdeconnect-kde into pgz-sfos-initial
  • Cleanups
  • Build fixes
  • Dont use a seperate json file for sailfish
  • no need to check version as it is done in the header
  • Merge branch 'master' into pgz-sfos-initial
  • Merge remote-tracking branch 'origin/master' into pgz-sfos-initial
piggz marked 8 inline comments as done.Jul 23 2018, 7:17 PM

Addressed remaining comments

piggz added a comment.Jul 23 2018, 7:19 PM

Please note, the main change here is renaming the .json file, for the sendnotification plugin to .json.in, and using cmake to process the file

The diff contains a few unrelated changes, please remove them and do them in another commit if it seems necessary to you.
Looks quite good to me, but I can't test anything in regard to sailfish.
We already have a QML/Kirigami app in the app/ folder for e.g. Plasma Mobile. Can we use that one instead of creating a new one for SFOS?

CMakeLists.txt
92

fileitemactionplugin has been removed. Remove that line

piggz updated this revision to Diff 38275.Jul 23 2018, 8:12 PM
  • Fix removed plugin
piggz marked an inline comment as done.Jul 23 2018, 8:13 PM

The diff contains a few unrelated changes, please remove them and do them in another commit if it seems necessary to you.
Looks quite good to me, but I can't test anything in regard to sailfish.
We already have a QML/Kirigami app in the app/ folder for e.g. Plasma Mobile. Can we use that one instead of creating a new one for SFOS?

We cant im afraid, Kirigami requires Qt > than 5.6, i already asked around the time of the initial port.

Can you point out the unrelated changes, as, combined with the large merge, im getting lost myself!

Cheers

apol added a comment.Jul 25 2018, 10:55 AM

I'd clean up the patch first but it looks good to me overall.

daemon/CMakeLists.txt
3 ↗(On Diff #38275)

This is already being searched in the parent directory. This file is not parsed when on sailfishos.

4 ↗(On Diff #38275)

Please remove just new lines.

albertvaka accepted this revision.Jul 27 2018, 5:23 PM

This patch looks so much better than its initial version. Let's merge it and see what happens :)

apol accepted this revision.Jul 27 2018, 5:24 PM
nicolasfella accepted this revision.Jul 27 2018, 9:37 PM
piggz updated this revision to Diff 38920.Aug 1 2018, 9:15 PM
piggz marked 2 inline comments as done.
  • Remove unnescessary change
piggz added a comment.Aug 1 2018, 9:17 PM

Reverted the unnecessary change to daemon/CMakeLists.txt which came about as part of the merge. Hope all ok now.

nicolasfella accepted this revision.Aug 1 2018, 9:28 PM
nicolasfella removed a reviewer: mtijink.

Looks good to me

interfaces/CMakeLists.txt
15 ↗(On Diff #38920)

Here's still a unrelated change. No need to upload a new diff, just remove it before pushing

This revision is now accepted and ready to land.Aug 1 2018, 9:28 PM
This revision was automatically updated to reflect the committed changes.