526717 - remove non-working "Set as desktop background" from the UI for unsupported desktops

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch

In openSUSE we have the attached patch to remove the context menu option if not running under Gnome.
It's no general Gnome vs. other desktops approach but only fixes that one thing but still it's a pretty easy way so we should consider if that makes sense for the other distros and if so just take it as a quick fix.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 410449 [details] [diff] [review]
patch

Asking for comments on that.

Comment 2

[Dão Gottwald [:dao]]

Doesn't that patch disable it for Windows and OS X?

Comment 3

[Wolfgang Rosenauer [:wolfiR]]

oops, yes, sorry. I had my Linux-only glasses on. I'll massage it a bit to make these work again but still wondering if the sense of it would be accepted at all.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 410449 [details] [diff] [review]
patch

This doesn't need ui-review.

I think this is acceptable, but it would probably be better if we could somehow figure out whether setting the desktop background is actually supported rather than making guesses based on DESKTOP_SESSION.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

So the gnome shell service is available even when not running under gnome? What other calls to it will fail in these scenarios?

Would it make sense to just make the shell service fail to initialize entirely in these cases (which is already handled everywhere it's used)?

Comment 6

[Mike Hommey [:glandium]]

The gnome shell service is available any time the gnome libraries are available. Not initializing the shell service entirely as you suggest would make OpenApplicationWithURI and OpenApplication unavailable when not running under Gnome. This would mean the execution of an external mailto application, or feed viewer would fail when not running under Gnome. Is that something you want ?

Comment 7

[Mike Hommey [:glandium]]

(In reply to comment #6)
> This would mean the execution of an external mailto application,
> or feed viewer would fail when not running under Gnome. Is that something you
> want ?

Maybe not mailto, as it is maybe handled by the uriloader. But definitely the feed viewer.

Comment 8

[Wolfgang Rosenauer [:wolfiR]]

As Mike said. The shell service is still needed basically to get some stuff working under non-Gnome environments as long as there is no other integration available.
(In reply to comment #4)

> I think this is acceptable, but it would probably be better if we could somehow
> figure out whether setting the desktop background is actually supported rather
> than making guesses based on DESKTOP_SESSION.

The problem is that technically setting the desktop background is always supported if the shell service is available. The thing is that we set the background always just for Gnome. So it's working but just not for the running environment if it's not Gnome. If we don't check the environment in that location we would have to do the same in the shell service itself. In the end we only know that way if Gnome is running or not. Just the existence of gconf (and therefore the shell service) is not enough to know that.

Comment 10

[Alexander Sack]

xdg-mime uses a different approach/env to guess current DE:

detectDE()
{
    if [ x"$KDE_FULL_SESSION" = x"true" ]; then DE=kde;
    elif [ x"$GNOME_DESKTOP_SESSION_ID" != x"" ]; then DE=gnome;
    elif xprop -root _DT_SAVE_MODE | grep ' = \"xfce4\"$' >/dev/null 2>&1; then
DE=xfce;
    fi
}

Comment 11

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 410449 [details] [diff] [review]
patch

needs fix

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Perhaps we should add a canSetDesktopBackground attribute on nsIShellService, then?

Comment 13

[Wolfgang Rosenauer [:wolfiR]]

Uh, long time ago and still in my patch queue.
About comment 12: What is your actual suggestion in light of comment 8?
Something like just adding that attribute to the interface and within the "Gnome" shell implementation checking if Gnome is really running and set the attribute accordingly?
If that would be an accepted solution I can try to come up with a patch.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

You could add it to nsIShellService and just have it always return true on other platforms, but yeah, that sounds reasonable.

Comment 15

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch

This patch adds an attribute to the nsIShellService interface if setting the desktop background is possible (note the difference between technically possible and actually effective in the user's environment).
So for Mac and Windows this always returns true while for Linux it checks if Gnome is running beneath Firefox.
There are several ways to determine if Gnome is running (see my previous proposal or the one xdg-open is using). I was told the latest way to do it was asking something through dbus but most apps are using one of both environment variables. I'm currently waiting for feedback from a Gnome developer if it's safe (for now) to use the env in the patch but do assume so at the moment.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 611250 [details] [diff] [review]
patch

>diff --git a/browser/components/shell/public/nsIShellService.idl b/browser/components/shell/public/nsIShellService.idl

>+   * Used to determine whether or not to offer "Set as desktop background"
>+   * context menu item.

nit: say "functionality" instead of "context menu item".

Comment 17

[Wolfgang Rosenauer [:wolfiR]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15fc1cf4e5b2

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/15fc1cf4e5b2