Merge lp://qastaging/~osomon/unity-2d/preferred-applications into lp://qastaging/unity-2d/3.0

Works well, except if one tries to set KDE applications as defaults. See the end of the review for details.

Some questions, suggestions first:

# LauncherApplication::setDesktopFile()
- Should the if newDesktopFile != oldDesktopFile be something like this instead?

"""
if (newDesktopFile == oldDesktopFile) {
    return;
}
Q_EMIT desktopFileChanged(newDesktopFile);
"""

- Not really related to this MR but g_object_unref(m_appInfo) should be called before creating a new GDesktopAppInfo.

# places/HomeButtonApplication.qml

- Having both desktopFile and desktop_file is quite confusing. application.desktop_file should be renamed to application.desktopFile and code inside it should use parent.desktopFile to disambiguate.

# Problem with KDE applications

Assuming desktopFile == binaryName + ".desktop" makes it unpractical to set KDE applications as default. To set a KDE4 application as default I have to prefix its name with "kde4-" in gnome-default-application-properties.

The reason behind this is that KDE4 desktop files are all installed in /usr/share/applications/kde4, meaning their desktop file id is "kde4-$app.desktop" (see http://standards.freedesktop.org/menu-spec/1.0/go01.html#term-desktop-file-id ).

> # LauncherApplication::setDesktopFile()
> - Should the if newDesktopFile != oldDesktopFile be something like this
> instead?
>
> """
> if (newDesktopFile == oldDesktopFile) {
> return;
> }
> Q_EMIT desktopFileChanged(newDesktopFile);
> """

For some reason which I still need to investigate, applying this change seems to break the update of icons for newly created web favorites.

> - Not really related to this MR but g_object_unref(m_appInfo) should be called
> before creating a new GDesktopAppInfo.

I believe you fixed this one already, in a separate merge request.

> # places/HomeButtonApplication.qml
>
> - Having both desktopFile and desktop_file is quite confusing.
> application.desktop_file should be renamed to application.desktopFile and code
> inside it should use parent.desktopFile to disambiguate.

This is a more general problem with naming consistency of the properties of some classes, particularly LauncherApplication. 'desktop_file' should be renamed 'desktopFile' indeed, but I don’t think it’s a good idea to do this change here and now. Renaming should be performed in a separate branch and fix all the properties, not just this one.
I tried to make the code more explicit by using parent.desktopFile nevertheless, but this doesn’t seem to work: 'parent' is undefined in this block of code…

> # Problem with KDE applications
>
> Assuming desktopFile == binaryName + ".desktop" makes it unpractical to set
> KDE applications as default. To set a KDE4 application as default I have to
> prefix its name with "kde4-" in gnome-default-application-properties.
>
> The reason behind this is that KDE4 desktop files are all installed in
> /usr/share/applications/kde4, meaning their desktop file id is
> "kde4-$app.desktop" (see http://standards.freedesktop.org/menu-
> spec/1.0/go01.html#term-desktop-file-id ).

We have a more general issue with KDE4 applications throughout unity-2d’s applications. I know this solution is far from perfect, but it’s a cheap one that doesn’t involve writing custom C++ code to infer the correct desktop file from the path of an executable. It’s also the way it’s done in Unity, meaning Unity will fail to display KDE4 applications too. I can definitely give it a try, if we think the effort is worth the benefit. Do we?

> > # LauncherApplication::setDesktopFile()
> > - Should the if newDesktopFile != oldDesktopFile be something like this
> > instead?
> >
> > """
> > if (newDesktopFile == oldDesktopFile) {
> > return;
> > }
> > Q_EMIT desktopFileChanged(newDesktopFile);
> > """
>
> For some reason which I still need to investigate, applying this change seems
> to break the update of icons for newly created web favorites.

I figured it out: the desktop files are monitored for changes, and whenever the contents of a file changes, setDesktopFile(…) is called with the same filename. As the contents have changed, some properties of the applications may have changed, which is why we should not return early if newDesktopFile == oldDesktopFile.

> > > # LauncherApplication::setDesktopFile()
> > > - Should the if newDesktopFile != oldDesktopFile be something like this
> > > instead?
> > >
> > > """
> > > if (newDesktopFile == oldDesktopFile) {
> > > return;
> > > }
> > > Q_EMIT desktopFileChanged(newDesktopFile);
> > > """
> >
> > For some reason which I still need to investigate, applying this change
> seems
> > to break the update of icons for newly created web favorites.
>
> I figured it out: the desktop files are monitored for changes, and whenever
> the contents of a file changes, setDesktopFile(…) is called with the same
> filename. As the contents have changed, some properties of the applications
> may have changed, which is why we should not return early if newDesktopFile ==
> oldDesktopFile.

Not really happy we can't fix support for KDE applications, but having
configurable default applications is better than hardcoded applications, even
if not all applications are supported. Let's merge your branch.

Can you just add a comment to the code with your explanation about
desktopFileChanged() and file a report about support for KDE applications?

Please mark the MR approved when you're done.

Thanks for the review Aurélien (and sorry about the lack of support for KDE4 applications…).

I added the comment you requested.
I also filed bug #750081 to track the issue with KDE4 applications.