362239 - Replace 'desktop background' monitor image on Windows with something nicer

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jeff Walden [:Waldo]]

See bug 296583, particularly bug 296583 comment 1.

Comment 1

[Dão Gottwald [:dao]]

Attachment: proposed monitor.png

Comment 2

[Dão Gottwald [:dao]]

Attachment: attachment 247003 in action

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #1)
> Created an attachment (id=247003) [edit]
> proposed monitor.png

Empty pixels are cropped at the edge. I can add them back to match the dimensions of the old image, if needed.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 247003 [details]
proposed monitor.png

Dao, this is on the right track, but let's go with a full black frame since it will set up a better contrast with the preview of the background image. Also, adding a power button to the lower right would be a nice touch.

Comment 5

[Dão Gottwald [:dao]]

Attachment: v2

Comment 6

[Dão Gottwald [:dao]]

Attachment: v2 preview

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 249529 [details]
v2

Nice! Thanks, Dao.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch

I believe the top and right attributes should already have been translated to CSS as part of bug 296583, but I'm not sure if margin is the right thing.

If you don't like that approach, I can also make the image fit the old dimensions, like the pinstripe one. Or, I could adjust the pinstripe image and set left="11" top="12".

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> Created an attachment (id=249540) [edit]
> patch
> 
> I believe the top and right attributes should already have been translated to
> CSS as part of bug 296583, but I'm not sure if margin is the right thing.

Just checked this, margin seems to work fine.

Comment 10

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

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

Shouldn't the width and height be moved into the theme file too?

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> (From update of attachment 249540 [details] [diff] [review])
> Shouldn't the width and height be moved into the theme file too?

Yep, but then the script needs an update, too (this._monitor.style.width won't work).
What about style="overflow: hidden;"? Move that to the style sheets as well?

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch

Comment 13

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

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

>Index: components/shell/content/setDesktopBackground.js

>-    img.width = parseInt(this._monitor.style.width);
>-    img.height = parseInt(this._monitor.style.height);
>+    img.width = this._monitor.boxObject.width;
>+    img.height = this._monitor.boxObject.height;

This makes the background image stretch below the boundaries of the monitor, presumably because this._monitor.boxObject.height is the height of the entire monitor image, including the base (see http://gavinsharp.com/tmp/monitor.png).

>Index: themes/pinstripe/browser/setDesktopBackground.css
>Index: themes/winstripe/browser/setDesktopBackground.css

>+#monitor {
>+  width: 153px;
>+  height: 114px;
>+  margin-top: 18px;
>+  margin-left: 13px;
>+}

Can't you just remove these explicit height/width/margin rules? You can probably remove the hardcoded overflow: auto in the xul file too, no?

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch, makes the desktop match the monitor (checked in)

(In reply to comment #13)
> Can't you just remove these explicit height/width/margin rules?

I don't think so ... It has to fit into the transparent area of the monitor image.

Btw, you did use the wrong monitor.png. (Although it has the same dimensions, so that shouldn't matter for the review.)

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Can someone check this in please? It's not a blocker, but it's ready to go.

Comment 16

[Kenneth Herron]

Checked in the patch "patch, makes the desktop match the monitor". Checked in the patch "v2" as browser/themes/winstripe/browser/monitor.png. Clearing checkin-needed status.

Comment 17

[Dão Gottwald [:dao]]

thanks, Kenneth!

Comment 18

[mcdavis941 (sporadically reading bugmail)]

setDesktopBackground.css in winstripe contains the lines:

#noPreviewAvailable {
  background-color: white !important;
  font-size: 12px !important;
}

What condition is that supposed to handle? 

Comment 19

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

(In reply to comment #18)
> What condition is that supposed to handle?

Since no element with that ID exists, it's clearly no longer needed. Please file a new bug on removing it, and CC me?

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> (In reply to comment #18)
> > What condition is that supposed to handle?
> 
> Since no element with that ID exists, it's clearly no longer needed.

Bug 386163 will cover that.