Instead of using KDBusService we can just use Window
Messages to communicate with other processes on Windows.
Details
Details
- Reviewers
mlaurent - Maniphest Tasks
- T1391: Kleopatra: Make direct dependency on KDBusAddons optional
Diff Detail
Diff Detail
- Repository
- R43 KDE PIM
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Laurent can you give me a review for this please? I've written this class in a way that I think it could be included in a Tier 1 Framework somewhere. (Have not found an appropiate one).
This is also my first real use of the pimpl idiom. Which i partly decided to use as to better understand it, as this is used all over Kleopatra.
I don't expect you to review the Windows specifics. I tested there and ran the unit test :-)
This is Gpg4all.
| kleopatra/utils/kuniqueservice_dbus.cpp | ||
|---|---|---|
| 2 | Uhm,.. this is not what my patch shows. Tis is a new file and kwatchgnupg/main.cpp was also modified in this patch. | |
| kleopatra/utils/kuniqueservice_win.cpp | ||
|---|---|---|
| 220 | this should not be needed because we have automoc nowadays, and the private calls is not a inherit from QObject. | |
| kleopatra/utils/kuniqueservice_win.cpp | ||
|---|---|---|
| 220 | Doesn't automoc just mean that we don't have to declare moc files in cmake anymore? Without the include you get errors like: because kuniqueservice.h contains Signals / Slots that have to be defined somewhere. | |
| kleopatra/utils/kuniqueservice_win.cpp | ||
|---|---|---|
| 220 | no normally you don't have to add the moc line in the cpp with automoc, but there are special cases where you have to. The reason is if in the cpp file itself is a QObject class is created. But I don't know the internals... If it does not compile without leave it. | |
| kleopatra/autotests/kuniqueservicetest.cpp | ||
|---|---|---|
| 47 | m_proc(Q_NULLPTR) | |
| 123 | QStringLiteral("..."); | |
| 125 | Same here use QStringLiteral when possibke | |
| kleopatra/utils/kuniqueservice_win.cpp | ||
| 55 | const QString get...() const ? | |
| 220 | Try to compile from scratch and see if you have an moc warning about it. | |
Comment Actions
Would it make sense to use QtSingleApplication?
https://github.com/qtproject/qt-solutions/tree/master/qtsingleapplication
Comment Actions
+10.
This looks like an exact reimplementation of the features provided by QtSingleApplication.
@apol: Any idea if we could put a copy of QtSingleApplication (maybe rename, too) somewhere in KF5? This class is needed by several apps (can think of kate + kdevelop) on OS X (optional) + Windows (much needed, we don't want DBUS there).
Comment Actions
No this was intended as a reimplementation of the Command Line call features provided by KDBusService(Unique). (no Application class)
@apol: Any idea if we could put a copy of QtSingleApplication (maybe rename, too) somewhere in KF5? This class is needed by several apps (can think of kate + kdevelop) on OS X (optional) + Windows (much needed, we don't want DBUS there).
I've looked at QtSingleApplication beforehand and decided against it. As a qt-solution it's not to use as easy as a framework. Either I've made it in a Library / packaged it myself or copied the code into Kleopatra :-/ . I also wanted to have an interface Similar to KDBusService Unique. Like not Ifdef'ing the Class of the Application. And I still wanted to keep the dbusservice interface on Linux. I think you could implement the interface also with sockets in a more portable way. But I found Window Messages the simplest way to get this done without locking etc.
Kleopatra already handled DBusService activate signals with arguments nicely and emitted the exitStatus signal for errors. No extra glue for sendMessage / message received required.
Comment Actions
@apol: Any idea if we could put a copy of QtSingleApplication (maybe rename, too) somewhere in KF5? This class is needed by several apps (can think of kate + kdevelop) on OS X (optional) + Windows (much needed, we don't want DBUS there).
I'm not too sure how it's meant to be used. Is it meant to be copied over into every application?
Maybe it would make sense to come up with a KSingleApplication framework and get the code in as a tier1. Or see if we can get it in QtCore somehow. It bothers me slightly the unnecessary coupling with Q*Application and lack of integration with QCommandLineParser.
Either way, I can't but agree that it needs work, although it might fill the needs of Kleopatra as is.
Comment Actions
Updated with fixes from review.
| kleopatra/autotests/kuniqueservicetest.cpp | ||
|---|---|---|
| 47 | I'm also changing the nullptr's in my code to Q_NULLPTR as this is used (nearly) everywhere else in KDEPIM. | |
| kleopatra/utils/kuniqueservice_win.cpp | ||
| 55 | Right and for getForeignResponder, too. | |
| 220 | I've tested this and get compile errors. Probably because the header which declares the Q_OBJECT is called kuniqueapplication and the .cpp files have the _dbus and _win suffixes? If I remove the line from kuniqueservice_dbus.cpp I also get compile errors. | |
Comment Actions
This was my question, too Krita copied it in.
Maybe it would make sense to come up with a KSingleApplication framework and get the code in as a tier1. Or see if we can get it in QtCore somehow. It bothers me slightly the unnecessary coupling with Q*Application and lack of integration with QCommandLineParser.
I also dislike the coupling and the interface for message sending there would be need for some ifdef'ed glue code. I did not want.
Lokalize btw. does a similar thing to implement a Unique Application like behavior (also using window messages).
Either way, I can't but agree that it needs work, although it might fill the needs of Kleopatra as is.
Do you mean this patch needs work or that a KSingleApplication needs work?
For this patch I think we could add locking in the responder to avoid to have to check and kill if there is already a process it is responding to. But I don't see this as a Problem for Kleopatra and even in that case by killing the Application already responding too the only information lost should be the return code.
Do you see other Problems?
Comment Actions
Do you see other Problems?
With the exclusive Kleopatra use-case, my impression is that it would be better to copy over QtSingleApplication rather than coming up with a new implementation that possibly duplicates features and is also possibly buggy. I don't want to see such scary implementations in each and every Qt5-based application that wants to enforce a single instance.
I am not a KDE PIM maintainer, feel free to take my opinion with a grain of salt.
Comment Actions
I totally agree. Copying + extending/refactoring QtSingleApplication + putting it in a framework is the way to go here.
It's robust, QtCreator has been using it on all major platforms as 'unique application' solution for ages. I agree the coupling to Q*Application is not perfect, and could be relaxed.
Note this change here only solves the issue on Windows; if we ever want to get Kleopatra in shape for OS X, we'll have to add another few hundred LOCs of platform specific code...
Why not (finally!) fix this in a way all KDE applications can benefit from? :\
I am not a KDE PIM maintainer, feel free to take my opinion with a grain of salt.
Dito.
Comment Actions
Yes. That's the way it's currently done.
Maybe it would make sense to come up with a KSingleApplication framework and get the code in as a tier1. Or see if we can get it in QtCore somehow. It bothers me slightly the unnecessary coupling with Q*Application and lack of integration with QCommandLineParser.
As told in the other comments: +1 on everything
Comment Actions
I don't see the feature duplication here. There is no alternative to get the nice KDBusService Unique style interface available afaik.
I totally agree. Copying + extending/refactoring QtSingleApplication + putting it in a framework is the way to go here.
If by refactoring you mean implementing the same interface that's used on Linux, so that you won't have to add special code to handle this in every application I agree.
I really dislike the sendMessage / recieveMessage API which means that every application needs to have some special handling to check if it's already running and passing arguments through a QString.
It's robust, QtCreator has been using it on all major platforms as 'unique application' solution for ages. I agree the coupling to Q*Application is not perfect, and could be relaxed.
Note this change here only solves the issue on Windows; if we ever want to get Kleopatra in shape for OS X, we'll have to add another few hundred LOCs of platform specific code...
I'm not sure. I can see your point that Window messages are only something for Windows. But implementing this feature with Socket + Lockfile would have been more complex for me.
And I didn't want a dependency on QtNetwork when I can do this with WinAPI anyway. But yes a platform independent solution would be better.
Why not (finally!) fix this in a way all KDE applications can benefit from? :\
Ok. Let's give it a try. I think KCoreAddons could be a place for this as I'd like to avoid the QtNetwork dependency anyway and If we don't depend on QtNetwork we already have a place in a Framwork for this, which would solve the problem of "where to put this". The Qlockfile code is already available in qt so we can use that. Then a KUniqueService class with a similar API as in this patch that is based on File communication (instead of socket / pipe).
Something like:
First App creates a "Server"-Lockfile in the pattern from QSingeApplication. A client that can't take the lock, trys to lock a client lockfile (to avoid race / parallelism problems). And write's it's command line args into a file in the lockfile pattern with a predefined suffix. The server has a QFileSystem watcher monitoring for this, handles the request and writes the return code in another file (probably including the pid of the client). The client in turn watched for that file, reads it and exits according to that return code. The client lock file is released and the server can handle the next request. I think this sounds simple and robust enough for most cases and is portable.
Could you agree with something like that (and would use it)?
Comment Actions
Could you agree with something like that (and would use it)?
I'm not sure we could adopt it right away for many projects (Konversation, Discover, KTorrent, yakuake come to mind) that could definitely use this and would also benefit from dropping the KDBusAddons dependency.
Regarding the implementation, my thinking is that it might make sense to leave it up to the platform to define it. It could even be in a separate new tier1 framework so that we can have a wider spectrum than strict QtCore dependency. I'd recommend to send an e-mail to kdeframeworks-devel@kde.org if you want to discuss how to make this a reality.
Comment Actions
I've commited this now in Kleopatra as I was carring it around as a patch in Gpg4win and that was bad. It's easier for me to maintain this as part of Kleopatra until we have a better solution.
http://commits.kde.org/kleopatra/24a0b52a2241e827903b53a9585bd16eba902a02
A more generic variant of this is still on my todo as gpgtoolchain (MacOS) said they might be interested in including Kleopatra. I'll add a new backlog item and link it in the task.