In my previous change I made sure that the dbus service name for SNI is predictable so we can allow it in advance, but it
introduced problem for multiple instances of the same app. This change doesn't require predictable name as org.kde.appName.*
is allowed in flatpak by default if we use same id in flatpak manifest (which we do for all KDE apps) and also makes sure
that the name will be unique, because we cannot rely on pid for apps running in flatpak, pid will be always same for multiple
instances. Btw. this is using same approach as in this review https://phabricator.kde.org/D5775.
Details
Details
- Reviewers
apol
Diff Detail
Diff Detail
- Repository
- R289 KNotifications
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
This is a big solution for a problem that I don't think exists.
There's no reason to register a new service, we pass the service name to the SNI watcher; it could be just the baseService (line 174 in the current code)
If you do do that, use m_dbus.baseName() not QDbusConnection::baseName() as each SNI is in a different DBus connection.
Comment Actions
There are actually two problems:
- You need to guarantee that the SNI dbus service name will be unique, you cannot use pid, because in flatpak the pid is usually 2 as it's isolated in the sandbox
- To let the app have access to create SNI you have to either allow it in manifest, which you can do using i.e --own-name=org.kde.StatusNotifierItem.*, but if you use wildcard, then it means the first instance of the app will own all dbus services starting with org.kde.StatusNotifier. To solve this problem we decided to use org.kde.appName.StatusNotifier.* as this is enabled in flatpak by default without assumption that it will own all possible variants of this name.
There's no reason to register a new service, we pass the service name to the SNI watcher; it could be just the baseService (line 174 in the current code)
Not sure what do you mean by registering a new service, I still register one with just a different name in case we are running in flatpak.
Comment Actions
I explained badly, let me try again with code.
https://paste.kde.org/pvzge2wq8
It will work in flatpak, and the SNI test we have in p-w still works fine.
Comment Actions
That indeed seems to work, I'll discard this review and you can revert my previous patch and fix it your way.