Add watchers for xdg desktop file changes #174

mariogrip · 2019-08-14T20:57:37Z

This adds watchers for xdg desktop file changes. It also figures out if
the appid id is "ubuntu-app-launch" or "standard xdg" style.

This spawns the first code to replace most (if not all) of
ubuntu-app-launch, but with QT!

This fixes: ubports/ubuntu-touch#1058

plugins/Unity/Launcher/xdgwatcher.cpp

plugins/Unity/Launcher/xdgwatcher.h

plugins/Unity/Launcher/xdgwatcher.cpp

tests/plugins/Unity/Launcher/xdgwatcher.cpp

tests/plugins/Unity/Launcher/xdgwatcher.h

mariogrip · 2019-08-15T19:47:34Z

@dobey Should be fixed up now :)

plugins/Unity/Launcher/appdrawermodel.cpp

plugins/Unity/Launcher/xdgwatcher.cpp

mariogrip · 2019-08-16T17:51:28Z

@mardy There we go, that should fix it :) Also found a little error in my toStandardAppId() thats fixed now.

dobey · 2019-08-18T20:07:26Z

plugins/Unity/Launcher/appdrawermodel.cpp


-void AppDrawerModel::appRemoved(const QString &appId)
+// ubuntuAppId is appID without version suffix, see xdgwatcher.cpp.
+void AppDrawerModel::appRemoved(const QString &appId, const QString &ubuntuAppId)


Instead of passing both in here, why not make the functions (which seem to not require the class object itself to be useful), into public static methods, and then just call XdgWatcher::stripAppIdVersion(appid) when needed below? This avoids a semi-duplicated argument, and makes the code easier to follow.

problem with that is that the "normal/standard" appID might not be the same as strippedAppId as the "ubuntu style" app id does not add the parent folder into app id.

example:
.local/share/desktop/kde/kate.desktop would be kde-kate as appID.

example where these conflics:

.local/share/desktop/ubports/system-settings_ubports_1.1.1.desktop would then be passed as ubports-system-settings_ubports_1.1.1 to appRemoved, then stripped to ubports-system-settings_ubports and that would be wrong, as "ubuntu style" appID does not append "folder name" as xdg one does

But, click hook does not create desktop files in a subdir, and we get the app ID for these from inside the desktop file, so this isn't an issue. I don't see any need to have extra special handling in here. The model has a list of app IDs, and one gets removed. there doesn't need to be extra logic for special cases in here.

dobey · 2019-08-18T20:11:55Z

plugins/Unity/Launcher/xdgwatcher.cpp

+}
+
+// Ubuntu appID is filename without versionNumber after last "_"
+const QString XdgWatcher::toUbuntuAppId(const QString rawAppID) const {


I think this should be renamed to not have "Ubuntu" in the name. Perhaps it should be stripAppIdVersion instead.

Yeah make sense, will update this

dobey · 2019-08-18T20:14:02Z

plugins/Unity/Launcher/appdrawermodel.cpp

    int idx = -1;
    for (int i = 0; i < m_list.count(); i++) {
-        if (m_list.at(i)->appId() == appId) {
+        if (m_list.at(i)->appId() == appId || m_list.at(i)->appId() == ubuntuAppId) {


Do we even need the second comparison here (and thus the argument or need to strip the version) at all? Shouldn't the appId that gets passed in already have the version stripped always? In what cases would it not be?

The reason for this is to make sure we don't strip an "standard" appID by accident. Example is kate_kde.desktop, we would strip "_kde" in that case. Since the file has been removed we have no way of knowing if it's a "ubuntu style" app id.

OK, I re-read the comment in XdgWatcher, and this is a hack to deal with the fact the file was removed. I will have to think a bit on this, but I'm certain we can handle it a better way.

plugins/Unity/Launcher/xdgwatcher.cpp

dobey

Because previous comments and GitHub is broke.

UniversalSuperBox

https://ci.ubports.com/blue/organizations/jenkins/ubports%2Funity8/detail/xenial_-_edge_-_appwatcher/4/pipeline#step-38-log-192

mariogrip · 2019-09-13T17:28:21Z

@dobey that should be better, now its a static method that gets called instead of passing it with the signal.

mariogrip · 2019-09-14T01:19:51Z

Cool! It builds now. Just ignore the others as they are only here due to changes to Jenkins.

dobey

A few more concerns that came up thinking about this.

dobey · 2019-09-16T20:44:29Z

plugins/Unity/Launcher/xdgwatcher.cpp

+// "Ubuntu style" appID is filename without versionNumber after last "_"
+const QString XdgWatcher::stripAppIdVersion(const QString rawAppID) {
+    auto appIdComponents = rawAppID.split("_");
+    appIdComponents.removeLast();


I think we should only do this, if the appIdComponents.length() == 3, otherwise it is not a valid versioned app ID for the purposes here. This would also solve the concern above that we might strip kde_kate improperly. It would also simplify the code a fair bit, as we could then avoid special-casing below, and above, only passing the result of toAppId() around.

Yeah that make sense, will add that :)

This has still not been updated?

Thinking about this, we may still improperly strip for example org_kde_kate, so we would still need to open the file to check

dobey · 2019-09-16T20:52:31Z

plugins/Unity/Launcher/xdgwatcher.cpp

+
+const QString XdgWatcher::toAppId(const QFileInfo fileInfo) const {
+    QFile qFile(fileInfo.absoluteFilePath());
+    qFile.open(QIODevice::ReadOnly);


Actually, I think we can just get rid of the whole need to open the file and check for X-Ubuntu-Application-ID entirely, and only use toStandardAppId() to get the app ID, since click hooks place the files as the correct ${APP_ID}.desktop filename. Then we can just always strip the version.

Yeah i guess that should also work just fine, since we use to remove the app anyway.

This has still not been updated?

Thinking about this, we may still improperly strip for example org_kde_kate, so we would still need to open the file to check

I don't think this is a real concern. KDE does not install applications in this manner.

kde was just an example, but one app that does do this on my local system is krita_jpg_jpeg.desktop

plugins/Unity/Launcher/xdgwatcher.cpp

dobey · 2019-11-20T01:46:55Z

plugins/Unity/Launcher/xdgwatcher.cpp

+    connect(m_watcher, &QFileSystemWatcher::directoryChanged, this, &XdgWatcher::onDirectoryChanged);
+    connect(m_watcher, &QFileSystemWatcher::fileChanged, this, &XdgWatcher::onFileChanged);
+
+    const auto paths = QStandardPaths::standardLocations(QStandardPaths::ApplicationsLocation);


As this will only watch directories in the host, this PR doesn't solve the issue for install/remove of apps in Libertine. How are we going to handle those?

So the idea to that was to make symlinks the .desktop to the standard locations instead, this makes it easier for libertine work on other DE (if needed)

mariogrip · 2019-12-01T03:14:55Z

So, I added an internal registry, this way we will always give the correct id. This also really simplifies the code overall. and I think i covered all your comments @dobey. Please re-review :)

tests/plugins/Unity/Launcher/xdgwatcher.cpp

tests/plugins/Unity/Launcher/xdgwatcher.h

dobey · 2019-12-11T18:00:34Z

plugins/Unity/Launcher/xdgwatcher.cpp

+
+const QString XdgWatcher::toAppId(const QFileInfo fileInfo) const {
+    QFile qFile(fileInfo.absoluteFilePath());
+    qFile.open(QIODevice::ReadOnly);


I don't think this is a real concern. KDE does not install applications in this manner.

This adds watchers for xdg desktop file changes. It also figures out if the appid id is "ubuntu-app-launch" or "standard xdg" style. This spawns the first code to replace most (if not all) of ubuntu-app-launch, but with QT! This fixes: ubports/ubuntu-touch#1058

dobey

I still think this is not quite right, but I don't want to hold it up any longer on semantics.

mariogrip requested a review from UniversalSuperBox August 14, 2019 20:57

dobey reviewed Aug 15, 2019

View reviewed changes

plugins/Unity/Launcher/xdgwatcher.cpp Outdated Show resolved Hide resolved

dobey reviewed Aug 15, 2019

View reviewed changes

plugins/Unity/Launcher/xdgwatcher.cpp Show resolved Hide resolved

dobey reviewed Aug 15, 2019

View reviewed changes

plugins/Unity/Launcher/xdgwatcher.h Outdated Show resolved Hide resolved

dobey reviewed Aug 15, 2019

View reviewed changes

plugins/Unity/Launcher/xdgwatcher.cpp Outdated Show resolved Hide resolved

dobey reviewed Aug 15, 2019

View reviewed changes

tests/plugins/Unity/Launcher/xdgwatcher.cpp Outdated Show resolved Hide resolved

dobey suggested changes Aug 15, 2019

View reviewed changes

tests/plugins/Unity/Launcher/xdgwatcher.h Outdated Show resolved Hide resolved

mariogrip force-pushed the xenial_-_edge_-_appwatcher branch from 428d5cd to c581048 Compare August 15, 2019 19:46

mardy suggested changes Aug 16, 2019

View reviewed changes

plugins/Unity/Launcher/appdrawermodel.cpp Outdated Show resolved Hide resolved

plugins/Unity/Launcher/xdgwatcher.cpp Show resolved Hide resolved

plugins/Unity/Launcher/xdgwatcher.cpp Outdated Show resolved Hide resolved

plugins/Unity/Launcher/xdgwatcher.cpp Outdated Show resolved Hide resolved

mariogrip force-pushed the xenial_-_edge_-_appwatcher branch from c581048 to ae2df97 Compare August 16, 2019 17:50

mardy approved these changes Aug 16, 2019

View reviewed changes

mariogrip requested a review from dobey August 16, 2019 20:50

dobey reviewed Aug 18, 2019

View reviewed changes

UniversalSuperBox mentioned this pull request Aug 22, 2019

I would like to manage apps more easily ubports/ubuntu-touch#1199

Closed

mariogrip force-pushed the xenial_-_edge_-_appwatcher branch from ae2df97 to 593caba Compare August 22, 2019 12:07

mariogrip requested a review from dobey August 22, 2019 15:55

dobey suggested changes Aug 22, 2019

View reviewed changes

UniversalSuperBox suggested changes Aug 26, 2019

View reviewed changes

mariogrip force-pushed the xenial_-_edge_-_appwatcher branch from 593caba to 443fc3f Compare September 13, 2019 17:27

mariogrip force-pushed the xenial_-_edge_-_appwatcher branch from 443fc3f to eed86d5 Compare September 13, 2019 21:14

dobey suggested changes Sep 16, 2019

View reviewed changes

mariogrip requested review from UniversalSuperBox and dobey November 6, 2019 14:27

dobey reviewed Nov 20, 2019

View reviewed changes

mariogrip force-pushed the xenial_-_edge_-_appwatcher branch from eed86d5 to 598f92f Compare December 1, 2019 03:10

mariogrip changed the base branch from xenial_-_edge to xenial December 1, 2019 03:15

mariogrip force-pushed the xenial_-_edge_-_appwatcher branch from 598f92f to 126b2b9 Compare December 1, 2019 03:16

mariogrip requested a review from dobey December 11, 2019 14:28

dobey suggested changes Dec 11, 2019

View reviewed changes

mariogrip force-pushed the xenial_-_edge_-_appwatcher branch from 126b2b9 to 0a52673 Compare December 11, 2019 18:52

dobey approved these changes Dec 11, 2019

View reviewed changes

mariogrip merged commit 5e3b45c into xenial Dec 11, 2019

mariogrip deleted the xenial_-_edge_-_appwatcher branch December 11, 2019 19:35

UniversalSuperBox mentioned this pull request Dec 15, 2019

Application list not updated when installing via pkcon #242

Closed

Add watchers for xdg desktop file changes #174

Add watchers for xdg desktop file changes #174

Uh oh!

Conversation

mariogrip commented Aug 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mariogrip commented Aug 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mariogrip commented Aug 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dobey left a comment

Choose a reason for hiding this comment

Uh oh!

UniversalSuperBox left a comment

Choose a reason for hiding this comment

Uh oh!

mariogrip commented Sep 13, 2019

Uh oh!

mariogrip commented Sep 14, 2019

Uh oh!

dobey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariogrip Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariogrip commented Dec 1, 2019

mariogrip Sep 23, 2019 •

edited

Loading