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

Fixed (recommended) apps list given by GLib backend #216

Closed tsujan closed 4 years ago

tsujan commented 4 years ago

While using the code, I found out that XdgMimeAppsGLibBackend::recommendedApps and XdgMimeAppsGLibBackend::apps are not reliable: they may give wrong results, depending on the order of key values inside ~/.config/mimeapps.list.

The reason is that one app may be removed from the recommended list due to a suspicious logic inside XdgMimeAppsGLibBackend::recommendedApps; and XdgMimeAppsGLibBackend::apps just adds that list to the list returned by XdgMimeAppsGLibBackend::fallbackApps.

Therefore, the current patch follows GLib exactly, by

  1. Removing the suspicious logic from XdgMimeAppsGLibBackend::recommendedApps and also by
  2. Directly using g_app_info_get_all_for_type() in XdgMimeAppsGLibBackend::apps (because it always guarantees a correct result).

NOTE: GLib doc says this about g_app_info_get_recommended_for_type(): "… the first application of the list is the last used one, i.e. the last one for which g_app_info_set_as_last_used_for_type() has been called." We need the first app in our list, whether it is the last used one or not. GLib doesn't remove it; we shouldn't either, especially because we can sort our lists alphabetically or in any other way wherever we use them.

luis-pereira commented 4 years ago

NOTE: GLib doc says this about g_app_info_get_recommended_for_type(): "… the first application of the list is the last used one, i.e. the last one for which g_app_info_set_as_last_used_for_type() has been called." We need the first app in our list, whether it is the last used one or not. GLib doesn't remove it; we shouldn't either, especially because we can sort our lists alphabetically or in any other way wherever we use them.

NOTE: GLib doc says this about g_app_info_get_recommended_for_type(): "… the first application of the list is the last used one, i.e. the last one for which g_app_info_set_as_last_used_for_type() has been called." We need the first app in our list, whether it is the last used one or not. GLib doesn't remove it; we shouldn't either, especially because we can sort our lists alphabetically or in any other way wherever we use them.

Why should g_app_info_set_as_last_used_for_type() be in the list, in the first place ?

  1. It's not part of the standard
  2. and most important there is no guarantee that g_app_info_set_as_last_used_for_type() actually supports that mimetype.

A qtxdg user can do whatever he/she wants with the data returned by libqtxdg. I do not understand the link between sorting and removing g_app_info_set_as_last_used_for_type().

p.s. Did not yet check the code. It was written so long ago, that it's new code (for me).

tsujan commented 4 years ago

Why should g_app_info_set_as_last_used_for_type() be in the list, in the first place ?

Oh, it definitely should. The recommended apps are those apps that specialize in handling the type in question; "last used" isn't relevant at all.

For example, on my system, Firefox, Falkon and Chromium are the recommended apps for HTML, although FeatherPad and other text editors can also open HTML. Now, it isn't relevant whether Firefox, Falkon or Chromium was "last used". Actually, if it was relevant, then:

  1. GLib wouldn't include it in the recommended list.
  2. We would have no way of listing the specialized apps in our app chooser dialog. I could easily edit mimeapps.list so that Firefox, Falkon or Chromium would disappear from the recommended list, without removing any association and just by changing their arrangement. That would be a bug (that doesn't actually exist in GLib).

"last used" is about the order, not about being recommended (specialized).

luis-pereira commented 4 years ago

Oh, it definitely should. The recommended apps are those apps that specialize in handling the type in question; "last used" isn't relevant at all.

This sentence contradicts itself.

Recommended Apps are by GLib own definition are those applications which claim to support the given content type exactly, and not by MIME type subclassing. Adding g_app_info_set_as_last_used_for_type() breaks that definition because there's no guarantee that g_app_info_set_as_last_used_for_type() support the given content type exactly.

The first element of the returned list, directly handles the mimetype or not ?

For example, on my system, Firefox, Falkon and Chromium are the recommended apps for HTML, although FeatherPad and other text editors can also open HTML. Now, it isn't relevant whether Firefox, Falkon or Chromium was "last used". Actually, if it was relevant, then:

I'm not sure if understood correctly. What it's relevant here is that the last used app can't be guaranteed to directly support the mimetype and not by sub-classing it.

1. GLib wouldn't include it in the recommended list.

Everybody makes mistakes.

2. We would have no way of listing the specialized apps in our app chooser dialog. I could easily edit `mimeapps.list` so that Firefox, Falkon or Chromium would disappear from the recommended list, _without removing any association and just by changing their arrangement_. That would be a bug (that doesn't actually exist in GLib).

Sorry, How is this related with removing the g_app_info_set_as_last_used_for_type() ? It seems that we are not talking about the same thing.

"last used" is about the order, not about being recommended (specialized).

last_used breaks the recommend apps definition (support the mimetype directly).

GLib allows for a user to set g_app_info_set_as_last_used_for_type() but doesn't provide a g_app_info_get_last_used_for_type().

tsujan commented 4 years ago

Recommended Apps are by GLib own definition are those applications which claim to support the given content type exactly, and not by MIME type subclassing.

That's exactly what I said ("apps that specialize in handling..."). I don't know how it could be interpreted otherwise.

Adding g_app_info_set_as_last_used_for_type() breaks that definition because there's no guarantee that g_app_info_set_as_last_used_for_type() support the given content type exactly

No one should add or remove anything to/from what GLib gives with g_app_info_get_recommended_for_type(). That's the whole point of the patch. g_app_info_set_as_last_used_for_type() has nothing to do with that.

Everybody makes mistakes

g_app_info_get_recommended_for_type() does what its name says: it gives the list of all recommended apps. "last used" just comes first in that list if it exists and is recommended. There is no mistake in GLib, otherwise, gnome/GTK users would have a big bug for years (missing recommended apps).

GLib allows for a user to set g_app_info_set_as_last_used_for_type() but doesn't provide a g_app_info_get_last_used_for_type().

It's the first member of the recommended apps. g_app_info_set_as_last_used_for_type() first adds the association and so, makes the app recommended. For example, if I assign HTML to FeatherPad, it will be like Firefox in dealing with HTML. Of course, HTML isn't among FeatherPad's default mimetypes (in its desktop file) but the user can add it.

tsujan commented 4 years ago

This is file association app chooser with the the new code (upcoming PR) and HTML:

1

Now, as I said earlier, I can only change some value orders in ~/.config/mimeapps.list (without adding/removing associations) and make Firefox or Chromium disappear from above the list if the current patch isn't applied. That's a bug that this patch fixes.

As for FeatherPad, if I assign HTML to it manually, it'll be listed alongside browsers with regard to HTML, which is the correct behavior:

2

luis-pereira commented 4 years ago

Recommended Apps are by GLib own definition are those applications which claim to support the given content type exactly, and not by MIME type subclassing.

That's exactly what I said ("apps that specialize in handling..."). I don't know how it could be interpreted otherwise.

Adding g_app_info_set_as_last_used_for_type() breaks that definition because there's no guarantee that g_app_info_set_as_last_used_for_type() support the given content type exactly

No one should add or remove anything to/from what GLib gives with g_app_info_get_recommended_for_type(). That's the whole point of the patch. g_app_info_set_as_last_used_for_type() has nothing to do with that.

g_app_info_set_as_last_used_for_type() is the only thing here.

Everybody makes mistakes

g_app_info_get_recommended_for_type() does what its name says: it gives the list of all recommended apps. "last used" just comes first in that list if it exists and is recommended. There is no mistake in GLib, otherwise, gnome/GTK users would have a big bug for years (missing recommended apps).

No, There's no guarantee that the g_app_info_get_last_used_for_type() directly supports the mimetype. It can be anything. In your screenshot example. instead of Featherpad, it could be GIMP or qmmp. It's a circular thing, it's recommended only because, g_app_info_get_last_used_for_type() put it artificially there.

GLib allows for a user to set g_app_info_set_as_last_used_for_type() but doesn't provide a g_app_info_get_last_used_for_type().

It's the first member of the recommended apps. g_app_info_set_as_last_used_for_type() first adds the association and so, makes the app recommended. For example, if I assign HTML to FeatherPad, it will be like Firefox in dealing with HTML. Of course, HTML isn't among FeatherPad's default mimetypes (in its desktop file) but the user can add it.

luis-pereira commented 4 years ago

Our time is limited and things need to go forward. Somebody can approve it or it can be directly merged. I'm Ok with either way.

tsujan commented 4 years ago

In your screenshot example. instead of Featherpad, it could be GIMP or qmmp.

Yes, the user has right to add the HTML association to any app and make it "recommended". Whether that's reasonable or not is up to him/her.

I take full responsibility for this and merge it; it's needed for the lxqt-config patch, whose review will take lots of your time ;)