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

RESOLVED FIXED in Future

Status

defect
RESOLVED FIXED
Last year
6 months ago

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

Tracking

Trunk
Future
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.53 affected, seamonkey2.57esr fixed, seamonkey2.60 wontfix, seamonkey2.63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

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

Updated

Last year
Depends on: 1475867
Assignee

Updated

Last year
Blocks: 1475874
Assignee

Updated

10 months ago
No longer blocks: 1475874
Assignee

Comment 1

10 months ago
Posted patch Image names for OSX (obsolete) — Splinter Review
Not been able to test so some feedback on OSX would help.
Attachment #9004305 - Flags: feedback?(stefanh)

Comment 2

10 months ago
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

Updated

10 months ago
Flags: needinfo?(iann_bugzilla)
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

10 months ago
Ah, i see. Hmm, that patch doesn't apply clean either :/
Flags: needinfo?(iann_bugzilla)
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

10 months ago
(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

10 months ago
IOTW, I always assume trunk if no one says anything.
Assignee

Comment 8

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

Sorry, yes it is against ESR60.
Assignee

Comment 9

10 months ago
(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?
Flags: needinfo?(frgrahl)
> 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

10 months ago
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

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

Minus because of the above.
Attachment #9004305 - Flags: feedback?(stefanh) → feedback-
> - 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
Flags: needinfo?(frgrahl)

Comment 14

10 months ago
My addition to what Frank said was actually wrong.
> I would probably just take this bug here which seems to work.

And this comment was actually for bug 1475867
Assignee

Comment 16

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

Hopefully that has fixed the regression
Attachment #9004305 - Attachment is obsolete: true
Attachment #9005893 - Flags: review?(stefanh)

Comment 17

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

Comment 18

9 months ago
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

9 months ago
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

9 months ago
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);'?
Flags: needinfo?(iann_bugzilla)

Comment 21

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

Comment 22

9 months ago
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.
Flags: needinfo?(iann_bugzilla)
Attachment #9005893 - Flags: review?(stefanh) → review+
Assignee

Updated

8 months ago
Keywords: checkin-needed
Assignee

Comment 23

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

a=me for esr-60
Attachment #9005893 - Flags: approval-comm-esr60+

Comment 24

8 months ago
Pushed by frgrahl@gmx.net:
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
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.