lxqt / libqtxdg

Qt implementation of freedesktop.org xdg specs
https://lxqt.github.io
GNU Lesser General Public License v2.1
72 stars 35 forks source link

Available apps #228

Closed luis-pereira closed 3 years ago

tsujan commented 3 years ago

Using Categories is a good idea that didn't occur to me; should modify https://github.com/lxqt/lxqt-config/pull/641 after this is merged. I'll test it and tell you. Thanks!

tsujan commented 3 years ago

It passed my tests with a modified version of https://github.com/lxqt/lxqt-config/pull/641, except for one "issue" that I'm not sure is worth "fixing". I explain it with an example:

I have Enlightenment , which brings its file manager with OnlyShowIn=Enlightenment; in its desktop file. Now, XdgMimeApps::recommendedApps() correctly ignores it under LXQt but XdgDefaultApps::fileManagers() adds it to the list.

The reason is that XdgDefaultApps::fileManagers() calls XdgMimeApps::categoryApps(), which calls XdgMimeApps::allApps(). Therefore, OnlyShowIn=Enlightenment; is ignored.

tsujan commented 3 years ago

This seems to work as expected, without XdgMimeApps::categoryApps():

static QList<XdgDesktopFile *> categoryAndMimeTypeApps(const QString &category, const QStringList &protocols)
{
    QList<XdgDesktopFile *> apps;
    XdgMimeApps db;
    const QString cat = category.toUpper();
    for (const QString &protocol : protocols) {
        const auto recomApps = db.recommendedApps(protocol);
        for (auto const app : recomApps) {
            bool alreadyAdded = false;
            for (auto const added : apps) {
                if (*added == *app) {
                    alreadyAdded = true;
                    break;
                }
            }
            if (alreadyAdded) {
                delete app;
            } else {
                const QStringList cats = app->value(QL1S("Categories")).toString().toUpper().split(QL1C(';'));
                if (!cats.isEmpty() && (cats.contains(cat) || cats.contains(QL1S("X-") + cat))) {
                    apps.append(app);
                }
                else
                    delete app;
            }
        }
    }
    return apps;
}

EDIT: Added a missing condition later.

luis-pereira commented 3 years ago

@tsujan Good ideia. Yes, OnlyShowIn should be considered. It completely forgot about it. Will update to include it. The code snippet you posted, produces in some conditions incorrect (duplicated) results.

qtxdg-mat def-web-browser -l
firefox.desktop
chromium.desktop
firefox-developer-edition.desktop
org.kde.falkon.desktop
firefox.desktop
chromium.desktop
firefox-developer-edition.desktop
org.kde.falkon.desktop
firefox.desktop
chromium.desktop
firefox-developer-edition.desktop
org.kde.falkon.desktop
tsujan commented 3 years ago

@luis-pereira, you replied exactly when I modified the above code; a condition was missing. Please retry!

tsujan commented 3 years ago

With the modified code I get:

qtxdg-mat def-web-browser -l
firefox.desktop
org.kde.falkon.desktop
chromium.desktop

EDIT: I had checked it only with lxqt-config, which removes duplicated items. Afterward, I remembered that we have a command-line too ;)

luis-pereira commented 3 years ago

@tsujan I fixed it before reading your comments.

-        if (appSupportsSet.contains(protocolsSet)) {
+        if (appSupportsSet.contains(protocolsSet) && (*it)->isShown()) {

fixes it, is more robust and handles the Hidden entry.

tsujan commented 3 years ago

@luis-pereira Yes, isShown() can be used for hidden desktop files but the question arises as to whether GLib only handles OnlyShowIn or considers more properties when giving the list of recommended apps. Since we can't be sure, I think it's better to use XdgMimeApps::recommendedApps() directly.

tsujan commented 3 years ago

Interesting! GLib really considers other factors too. This screenshot is taken by using the current PR:

1

While this one is taken with the PR + the above-mentioned code:

2

"LXQt PCManFM-Qt File Manager" is a desktop file that I've added to ~/.local/share/applications for use under KDE (its Exec enforces LXQt icon/style handling). For whatever reason, XdgMimeApps::recommendedApps() includes it (as it does in the first screenshot too, but in the second section).

I think this shows a deeper issue unrelated to the current PR. I'd seen it before but accepted the problematic handling of GLib as it was.

P.S. The main "problem" was negligible from start. The discrepancy in the logic has no practical consequence, IMO, and I have no problem with the PR. Feel free to merge it as it is.

luis-pereira commented 3 years ago

I think this shows a deeper issue unrelated to the current PR. I'd seen it before but accepted the problematic handling of GLib as it was.

Can you pls open a Issue ?

P.S. The main "problem" was negligible from start. The discrepancy in the logic has no practical consequence, IMO, and I have no problem with the PR. Feel free to merge it as it is.

The OnlyShowIn is important. XdgDesktopFile::isShow() enforces it's "correctness" for use. Yes, both approaches, produce the same results, IMO.

Thanks for your insights.

tsujan commented 3 years ago

Can you pls open a Issue ?

Sure.