Last Comment Bug 526717 - remove non-working "Set as desktop background" from the UI for unsupported desktops
: remove non-working "Set as desktop background" from the UI for unsupported de...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Shell Integration (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: Firefox 14
Assigned To: Wolfgang Rosenauer [:wolfiR]
:
: Robert Strong [:rstrong] (use needinfo to contact me)
Mentors:
: 266230 (view as bug list)
Depends on:
Blocks: 528510
  Show dependency treegraph
 
Reported: 2009-11-04 23:29 PST by Wolfgang Rosenauer [:wolfiR]
Modified: 2012-04-03 02:05 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (992 bytes, patch)
2009-11-04 23:29 PST, Wolfgang Rosenauer [:wolfiR]
no flags Details | Diff | Splinter Review
patch (5.11 KB, patch)
2012-04-01 00:20 PDT, Wolfgang Rosenauer [:wolfiR]
gavin.sharp: review+
Details | Diff | Splinter Review

Description Wolfgang Rosenauer [:wolfiR] 2009-11-04 23:29:44 PST
Created attachment 410449 [details] [diff] [review]
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] 2009-11-09 22:28:49 PST
Comment on attachment 410449 [details] [diff] [review]
patch

Asking for comments on that.
Comment 2 Dão Gottwald [:dao] 2009-11-10 03:02:37 PST
Doesn't that patch disable it for Windows and OS X?
Comment 3 Wolfgang Rosenauer [:wolfiR] 2009-11-10 03:08:11 PST
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] 2009-11-10 03:15:28 PST
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: gavin@gavinsharp.com] 2009-11-10 08:17:21 PST
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] 2009-11-10 08:24:14 PST
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] 2009-11-10 08:25:41 PST
(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] 2009-11-10 10:16:10 PST
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 9 Reed Loden [:reed] (use needinfo?) 2009-11-10 18:08:00 PST
*** Bug 266230 has been marked as a duplicate of this bug. ***
Comment 10 Alexander Sack 2009-11-11 02:29:28 PST
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] 2009-11-13 13:24:34 PST
Comment on attachment 410449 [details] [diff] [review]
patch

needs fix
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-11-13 13:34:25 PST
Perhaps we should add a canSetDesktopBackground attribute on nsIShellService, then?
Comment 13 Wolfgang Rosenauer [:wolfiR] 2012-03-16 15:20:21 PDT
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: gavin@gavinsharp.com] 2012-03-19 18:53:16 PDT
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] 2012-04-01 00:20:06 PDT
Created attachment 611250 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-04-02 13:00:43 PDT
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] 2012-04-02 13:46:14 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/15fc1cf4e5b2
Comment 18 Marco Bonardo [::mak] 2012-04-03 02:05:39 PDT
https://hg.mozilla.org/mozilla-central/rev/15fc1cf4e5b2

Note You need to log in before you can comment on or make changes to this bug.