Bug 391370 - bundled qtsingleapplication
Summary: bundled qtsingleapplication
Status: RESOLVED INTENTIONAL
Alias: None
Product: Falkon
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Mageia RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: David Rosca
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-04 06:26 UTC by David Geiger
Modified: 2019-03-14 10:54 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
falkon-3.0.0-system-qtsingleapplication.patch (3.08 KB, patch)
2018-03-04 21:39 UTC, Kevin Kofler
Details
Difference (3.48 KB, patch)
2018-03-06 06:53 UTC, The_assassin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Geiger 2018-03-04 06:26:18 UTC
Hi,

For now falkon uses only a bundled qtsingleapplication from source, so Is it possible to propose two choices, please? first, as it is, using the bundled one and the second using the system one.

- use bundled qtsingleappliction
- use system qtsingleapplication

Regards,
David
Comment 1 David Rosca 2018-03-04 09:05:19 UTC
It uses modified QtSingleApplication, so you can't build it against system version.
Comment 2 Kevin Kofler 2018-03-04 20:51:32 UTC
On some distros you can, because they carry the required API addition patch in QtSingleApplication, e.g.:
https://src.fedoraproject.org/cgit/rpms/qtsingleapplication.git/tree/qtsingleapplication-qupzilla.patch

We have been building QupZilla against the system QtSingleApplication in Fedora all this time.

For QupZilla, we used:

# unbundle qtsingleapplication
rm -fr src/lib/3rdparty/qtsingleapplication
ln -s %{_qt5_headerdir}/QtSolutions src/lib/3rdparty/qtsingleapplication
sed -i 's,include.*qtsingleapplication.*,,' src/plugins.pri
sed -i 's,include.*qtsingleapplication.*,,' src/lib/lib.pro

but now I have to adapt this for CMake.
Comment 3 Kevin Kofler 2018-03-04 21:39:15 UTC
Created attachment 111189 [details]
falkon-3.0.0-system-qtsingleapplication.patch

This patch, along with "rm -fr src/lib/3rdparty/qtsingleapplication", works for me on Fedora. (Of course, this will only work if you use a system QtSingleApplication with the aforementioned API patch.)
Comment 4 The_assassin 2018-03-05 06:44:17 UTC
„qtsingleapplication-qupzilla.patch“ is sucks, because QupZilla does not clean tmp file on quit with it.
Comment 5 Kevin Kofler 2018-03-05 14:19:06 UTC
Huh? Are you sure? The patch adds the missing removeLockFile function to the system QtSingleApplication. This looks no different to me from what the bundled QtSingleApplication in QupZilla/Falkon does. Am I missing something?
Comment 6 David Rosca 2018-03-05 14:53:10 UTC
No, that patch is fine.
Comment 7 The_assassin 2018-03-05 15:00:13 UTC
I'm absolutely sure. „qtsingleapp-qupzil-xxxx-xxx-lockfile“ is always remains in /tmp with this package: http://dl.fedoraproject.org/pub/fedora/linux/development/rawhide/Everything/source/tree/Packages/q/qtsingleapplication-2.6.1-32.fc28.src.rpm
If build QupZilla with bundled qtsingleapplication, it is always deleted.
So what is the reason that file to stays undeleted!?
Sorry for the bad English!
Comment 8 The_assassin 2018-03-05 15:13:28 UTC
I noticed that the QupZill/Falkon qtsingleapplication source has a lot of corrections which are not applied in the original source, so probably there is the reason.
Comment 9 Kevin Kofler 2018-03-05 16:33:45 UTC
Can you please file a bug against qtsingleapplication at bugzilla.redhat.com?
Comment 10 The_assassin 2018-03-05 17:04:28 UTC
(In reply to Kevin Kofler from comment #9)
Done: https://bugzilla.redhat.com/show_bug.cgi?id=1551678
Comment 11 Christoph Feck 2018-03-06 02:07:52 UTC
QtSingleApplication also leaves temporary files on Tumbleweed (e.g. for SMPlayer and Otter Browser), so it does not look like a downstream bug.
Comment 12 The_assassin 2018-03-06 06:53:52 UTC
Created attachment 111214 [details]
Difference
Comment 13 The_assassin 2018-03-06 06:55:04 UTC
Ah ha, when it works in Open suse the same way, we should accept that is the right way!?
I just attached a diff file between original „qtlocalpeer.cpp“ and Falkon „qtlocalpeer.cpp". You can see the difference yourself.
Comment 14 Kevin Kofler 2018-03-06 10:40:38 UTC
Comment on attachment 111214 [details]
Difference

The added braces around one-line conditionals do not change anything. QT_VERSION >= QT_VERSION_CHECK(4,5,0) is also always true. The addition of removeLockedFile() is already in the patch that we carry. The socket-related changes, I doubt they make any difference either.
Comment 15 The_assassin 2018-03-06 12:30:18 UTC
I was focused most likely to the added „curly braces“....
Also, i'm not a developer, so i really don't know what makes that difference.
I only can see the fact that the bundled app cleans the tmp file, which is the right behavior for me.
If i'm wrong, really sorry!
Comment 16 Kevin Kofler 2018-03-07 03:26:43 UTC
So I think I see where the issue is. First, we construct the QtSingleApplication with the default appId, which calls sysInit() once. Then, setAppId is called, which calls sysInit(appId) and creates a new socket, without deleting the old one first.

I would blame the modifications from QupZilla/Falkon adding that setAppId method. But the bundled version in QupZilla/Falkon has that too, so I don't see why you are not seeing this bug with that version.
Comment 17 The_assassin 2018-03-07 11:56:43 UTC
That's a list of the functions colled from the embeded api:

On start:

QtSingleApplication
setAppId
sysInit
QtLockedFile
QtLocalPeer
open
isRunning
isClient
isLocked
lock

On exit:
removeLockFile
removeLockedFile
~QtLockedFile

Unfortunately, i failed to get that from the external library.
Comment 18 David Rosca 2019-03-14 10:46:59 UTC
It now uses DBus on Linux, so it's now completely different from upstream QtSingleApplication.
Comment 19 Kevin Kofler 2019-03-14 10:51:48 UTC
When will this (and the other post-3.0 changes) get released? The 3.0.1 release is now more than 10 months old.
Comment 20 David Rosca 2019-03-14 10:54:19 UTC
In next few days.