Closed Bug 362239 Opened 13 years ago Closed 13 years ago

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

Categories

(Firefox :: General, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 alpha6

People

(Reporter: Waldo, Assigned: dao)

References

()

Details

(Keywords: icon, polish)

Attachments

(3 files, 4 obsolete files)

See bug 296583, particularly bug 296583 comment 1.
No longer depends on: 296583
Depends on: 296583
Attached image proposed monitor.png (obsolete) —
Attachment #247003 - Flags: ui-review?
Attached image attachment 247003 in action (obsolete) —
(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.
Status: NEW → ASSIGNED
Assignee: nobody → bugzilla
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attachment #247003 - Flags: ui-review? → ui-review?(mconnor)
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.
Attachment #247003 - Flags: ui-review?(mconnor) → ui-review-
Attached image v2
Attachment #247004 - Attachment is obsolete: true
Attachment #249529 - Flags: ui-review?(beltzner)
Attached image v2 preview
Comment on attachment 249529 [details]
v2

Nice! Thanks, Dao.
Attachment #249529 - Flags: ui-review?(beltzner) → ui-review+
Attached patch patch (obsolete) — Splinter Review
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".
Attachment #249540 - Flags: review?(gavin.sharp)
(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 on attachment 249540 [details] [diff] [review]
patch

Shouldn't the width and height be moved into the theme file too?
(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?
Attached patch patch (obsolete) — Splinter Review
Attachment #249540 - Attachment is obsolete: true
Attachment #250446 - Flags: review?(gavin.sharp)
Attachment #249540 - Flags: review?(gavin.sharp)
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?
Attachment #250446 - Flags: review?(gavin.sharp) → review-
(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.)
Attachment #247003 - Attachment is obsolete: true
Attachment #250446 - Attachment is obsolete: true
Attachment #254356 - Flags: review?(gavin.sharp)
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 beta1
Attachment #254356 - Flags: review?(gavin.sharp) → review+
Whiteboard: [checkin needed - don't forget new monitor.png]
Can someone check this in please? It's not a blocker, but it's ready to go.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [checkin needed - don't forget new monitor.png] → [checkin needed - don't forget new monitor.png][wanted-firefox3]
Whiteboard: [checkin needed - don't forget new monitor.png][wanted-firefox3] → [checkin needed] don't forget new monitor.png [wanted-firefox3]
Target Milestone: Firefox 3 beta1 → Firefox 3 alpha6
Attachment #254356 - Attachment description: patch, makes the desktop match the monitor → patch, makes the desktop match the monitor (checked in)
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.
Whiteboard: [checkin needed] don't forget new monitor.png [wanted-firefox3] → [wanted-firefox3]
thanks, Kenneth!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 385503
setDesktopBackground.css in winstripe contains the lines:

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

What condition is that supposed to handle? 
(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?
(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.
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
You need to log in before you can comment on or make changes to this bug.