1475865 - Port |Bug 1119088 - Set As Desktop Background does not work on OS X| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: OS Integration, defect)

Description

[Ian Neal]

Steps to Reproduce
Right click on an image and select "Set As Desktop Background".

Comment 1

[Ian Neal]

Attachment: Image names for OSX

Not been able to test so some feedback on OSX would help.

Comment 2

[Stefan [:stefanh]]

Does this patch depends on the one in bug 1475867?

patching file suite/components/shell/nsGNOMEShellService.cpp
Hunk #1 FAILED at 284
1 out of 1 hunks FAILED -- saving rejects to file suite/components/shell/nsGNOMEShellService.cpp.rej
patching file suite/components/shell/nsIShellService.idl
Hunk #1 FAILED at 3
Hunk #2 FAILED at 56
2 out of 2 hunks FAILED -- saving rejects to file suite/components/shell/nsIShellService.idl.rej
patching file suite/components/shell/nsMacShellService.cpp
Hunk #1 FAILED at 132
Hunk #2 FAILED at 152
2 out of 2 hunks FAILED -- saving rejects to file suite/components/shell/nsMacShellService.cpp.rej
patching file suite/components/shell/nsWindowsShellService.cpp
Hunk #1 FAILED at 606
1 out of 1 hunks FAILED -- saving rejects to file suite/components/shell/nsWindowsShellService.cpp.rej
avbryter: patch failed to apply

Comment 3

[Frank-Rainer Grahl (:frg)]

Yes, needs the patch from Bug 1475867 in first. Has a minor error. Missing comma after aPosition.

> +nsMacShellService::SetDesktopBackground(nsIDOMElement* aElement,
> +                                        int32_t aPosition 

Mixed results with 10.13.5. Does only work if you actually view the image e.g png name in urlbar and not for all images. The latter is the same in 2.49 but the first one is a  regression.

Comment 4

[Stefan [:stefanh]]

Ah, i see. Hmm, that patch doesn't apply clean either :/

Comment 5

[Frank-Rainer Grahl (:frg)]

Hmm does here in esr60. You can basically forget c-c right now :( I am doing rebasings there when I check in and IanN probably too.

Comment 6

[Stefan [:stefanh]]

(In reply to Frank-Rainer Grahl (:frg) from comment #5)
> Hmm does here in esr60. You can basically forget c-c right now :( I am doing
> rebasings there when I check in and IanN probably too.

Oh, I thought these patches were trunk-only.

Comment 7

[Stefan [:stefanh]]

IOTW, I always assume trunk if no one says anything.

Comment 8

[Ian Neal]

(In reply to Stefan [:stefanh] from comment #7)
> IOTW, I always assume trunk if no one says anything.

Sorry, yes it is against ESR60.

Comment 9

[Ian Neal]

(In reply to Frank-Rainer Grahl (:frg) from comment #3)
> Yes, needs the patch from Bug 1475867 in first. Has a minor error. Missing
> comma after aPosition.
> 
> > +nsMacShellService::SetDesktopBackground(nsIDOMElement* aElement,
> > +                                        int32_t aPosition 
> 
> Mixed results with 10.13.5. Does only work if you actually view the image
> e.g png name in urlbar and not for all images. The latter is the same in
> 2.49 but the first one is a  regression.

Is the regression from this patch or from the patch in bug 1475867?

Comment 10

[Frank-Rainer Grahl (:frg)]

> Is the regression from this patch or from the patch in bug 1475867?

I applied both together. I suspect from this one. I will leave the NI open for further investigation.

Comment 11

[Stefan [:stefanh]]

What Frank said (with some additions). The patch in bug 1475867 made SetDesktopBackground set an image, but not the chosen one - instead one of the stock images were chosen. The patch here made SetDesktopBackground do nothing unless you view the image, but then the image is never saved and used - instead a stock image is chosen (same behaviour like after the patch in bug 1475867).

Hmm, our nsMacShellService::SetDesktopBackground differ a bit from the m-c one.

Comment 12

[Stefan [:stefanh]]

Comment on attachment 9004305 [details] [diff] [review]
Image names for OSX

Minus because of the above.

Comment 13

[Frank-Rainer Grahl (:frg)]

> - instead one of the stock images were chosen.

I think it depends on the image and its size. Some bigger actually worked but this may be a coincidence. Smaller ones never got saved properly and you ended up with small corrupt image files.

I would probably just take this bug here which seems to work. For the background I would then try this in Fx 52 or 56. If it works there I would just take the setDesktopBackground.js and setDesktopBackground.xul from Fx and adapt. Looking at these might be easier instead of finding the error. The Fx dialog seems to have a few more options and fixes in.

Btw. we should drop the name OSX. It is macOS now :)

https://en.wikipedia.org/wiki/MacOS

Comment 14

[Stefan [:stefanh]]

My addition to what Frank said was actually wrong.

Comment 15

[Frank-Rainer Grahl (:frg)]

> I would probably just take this bug here which seems to work.

And this comment was actually for bug 1475867

Comment 16

[Ian Neal]

Attachment: Unbitrotted image names for MacOS

Changes since last version:
* Unbitrotted following changes to patch on bug 1475867
* Added missing change to nsMacShellService::SetDesktopBackground

Hopefully that has fixed the regression

Comment 17

[Stefan [:stefanh]]

Sorry, this fell of my radar. I'll try to look at it during the week.

Comment 18

[Stefan [:stefanh]]

Comment on attachment 9005893 [details] [diff] [review]
Unbitrotted image names for MacOS

So I think the problem here is this:
   setDesktopBackground: function() {
+    let url = (new URL(this.target.ownerDocument.location.href)).pathname;
+    let imageName = url.substr(url.lastIndexOf("/") + 1);

If you try to save the top header logo at http://www.seamonkey-project.org/ the url will be "/start" and imageName will be "" if you choose "Set Desktop Background..." from the context menu (you have to view the image first).

Comment 19

[Stefan [:stefanh]]

The interesting part is that "Set Desktop Background..." from right-clicking the top header logo at http://www.seamonkey-project.org/ works fine without the patch.

Comment 20

[Stefan [:stefanh]]

Comment on attachment 9005893 [details] [diff] [review]
Unbitrotted image names for MacOS

From reading bug 1119088, comment #20 I can't see that we have this issue in SeaMonkey. From what I can see we don't support data URL's?

   setDesktopBackground: function() {
+    let url = (new URL(this.target.ownerDocument.location.href)).pathname;
+    let imageName = url.substr(url.lastIndexOf("/") + 1);
     openDialog("chrome://communicator/content/setDesktopBackground.xul",
-               "_blank", "chrome,modal,titlebar,centerscreen", this.target);
+               "_blank", "chrome,modal,titlebar,centerscreen", this.target,
+               imageName);

I don't understand the m-c code. Does 'this.target.ownerDocument.location.href)).pathname' get you a non-data URL from a data URL (Firefox seems to have the same problem regarding the test with the SM header logo)?


If not, since we pass 'this.target' to the dialog and use that to set gImage in the onLoad() function and then use gImage.src for the preview, why don't we just use what we have in gImage and set the imageName in the onApply() function? Like 'gImage.src.substr(gImage.src.lastIndexOf("/") + 1);'?

Comment 21

[Stefan [:stefanh]]

OK, so I tested with a data URL to an image and that works with the patch (but not without the patch),

Comment 22

[Stefan [:stefanh]]

Comment on attachment 9005893 [details] [diff] [review]
Unbitrotted image names for MacOS

I think we should take this - the only issue I see is the case I described and that happens in Firefox too.

I haven't investigated this that much (still don't get the different cases when we set the image name), but one could work-around the problem by adding a check in the onApply() function to see if gImageName is empty and (if it is) set it by splitting gImage.src.

Comment 23

[Ian Neal]

Comment on attachment 9005893 [details] [diff] [review]
Unbitrotted image names for MacOS

a=me for esr-60

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/46594970c9a2
Port Bug 1119088 [Set As Desktop Background does not work on OS X] to SeaMonkey. r=stefanh

Comment 25

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-esr60/rev/666e0d74cd66309caaaf6e651c71195f5bdf5160