Closed Bug 458601 Opened 11 years ago Closed 11 years ago

port the full zoom UI to thunderbird

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b1

People

(Reporter: mkmelin, Assigned: mkmelin)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch proposed fix (obsolete) — Splinter Review
In bug 389628 (and some followups) full zoom UI was implemented for firefox. I think we should follow suit.

That is, change the View > Text Size to be

 Zoom >    Zoom In
           Zoom Out
        -----------------
           Reset
        -----------------
        x  Zoom Text Only      

I'm not sure if we want browser.zoom.full true or not... (this patch doesn't change it from the default false that is false).

Firefox also has per-site zooming (as a hidden pref), the part for handling that I just ripped out.

Also in this patch:
 - noticed we were including mailCore.js twice (since it's now in baseMenuOverlay too)
 - removed a few unused entities (reloadCmd.label and friends)
Attachment #341817 - Flags: ui-review?(clarkbw)
Attachment #341817 - Flags: review?(philringnalda)
Target Milestone: --- → Thunderbird 3.0b1
If per-site zooming is not used, you could probably drop all of the FullZoom controller except for FullZoom.updateMenu() and directly call the ZoomManager from the oncommand handlers, I think.
Attached patch proposed fix, v2Splinter Review
You're right, the previous patch still had some unnecessary code.
Attachment #341817 - Attachment is obsolete: true
Attachment #341834 - Flags: ui-review?(clarkbw)
Attachment #341834 - Flags: review?(philringnalda)
Attachment #341817 - Flags: ui-review?(clarkbw)
Attachment #341817 - Flags: review?(philringnalda)
Attachment #341834 - Attachment description: proposed fix → proposed fix, v2
Comment on attachment 341834 [details] [diff] [review]
proposed fix, v2

Looks good, and having seen just how many many times people who don't like full zoom can comment in bugs saying over and over "we hates it, we does," clearly defaulting to text zoom is the right answer ;)
Attachment #341834 - Flags: review?(philringnalda) → review+
Comment on attachment 341834 [details] [diff] [review]
proposed fix, v2

This looks good over all.  

I'm wondering if there is a better way to phrase the Text Only Zoom.  Maybe by saying "Zoom Message Text Only", however that doesn't really hit it either.  

I'm assuming that the only other thing we can actually zoom is inline images.  And I can't think of an easy way to convey that so maybe it's just fine as is.
Attachment #341834 - Flags: ui-review?(clarkbw) → ui-review+
Well, not only images, but also other elements with absolute width (px not em or %).

Fix checked in.
changeset:   548:2ef2829f4950
http://hg.mozilla.org/comm-central/rev/2ef2829f4950

->FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 341834 [details] [diff] [review]
proposed fix, v2

>-<!ENTITY reloadCmd.label "Reload">
>-<!ENTITY reloadCmd.accesskey "R">
>-<!ENTITY stopCmd.label "Stop">
>-<!ENTITY stopCmd.accesskey "S">

Are you sure you wanted to remove these?
Pretty sure they were unused, yes.
Only since 2002-10-04 (revision 1.3 of mailWindowOverlay.xul, where we stopped having them below the Display Attachments Inline menuitem in the View menu) :)
(In reply to comment #0)

> Firefox also has per-site zooming (as a hidden pref), the part for handling
> that I just ripped out.

i really wish you had left that for rss feed web page viewing, i encounter the problem of needing different zoom levels quite often among feeds.
Since the Fx implementation would just be remembering the mailbox:/// URI, I suspect that would be surprising for people who mix feeds in a single folder, and very surprising for people who zoomed one message in their Inbox and got them all zoomed. A separate bug that only remembered for feeds, and remembered the iframed URI instead of the mailbox:/// one, might be nice, though.
yes, clearly the value of content-base (a uri) would be used for rss (likely stored as the domain being sufficient, even for different feeds from the same aggregator case).  and it would also be nice if the 'from' in pop/imap or 'newsgroup' for news could do the same.  imo, this would be little new code and things like this make for slickness in an app.

filed bug 458948.
Remember zoom level on a per sender basis would be cool.
You need to log in before you can comment on or make changes to this bug.