Closed Bug 526717 Opened 15 years ago Closed 12 years ago

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

Categories

(Firefox :: Shell Integration, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 14

People

(Reporter: wolfiR, Assigned: wolfiR)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #410449 - Flags: ui-review?(beltzner)
Attachment #410449 - Flags: review?(gavin.sharp)
Comment on attachment 410449 [details] [diff] [review]
patch

Asking for comments on that.
Doesn't that patch disable it for Windows and OS X?
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 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.
Attachment #410449 - Flags: ui-review?(beltzner)
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)?
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 ?
(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.
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.
Assignee: nobody → mozilla
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 on attachment 410449 [details] [diff] [review]
patch

needs fix
Attachment #410449 - Attachment is obsolete: true
Attachment #410449 - Flags: review?(gavin.sharp)
Blocks: 528510
Perhaps we should add a canSetDesktopBackground attribute on nsIShellService, then?
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.
You could add it to nsIShellService and just have it always return true on other platforms, but yeah, that sounds reasonable.
Attached patch patchSplinter Review
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.
Attachment #611250 - Flags: review?(gavin.sharp)
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".
Attachment #611250 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/15fc1cf4e5b2
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/15fc1cf4e5b2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: