Closed Bug 477445 Opened 15 years ago Closed 15 years ago

What is this "orientation" of which you speak?

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philor, Assigned: philor)

Details

Attachments

(1 file)

Attached patch Hide my shameSplinter Review
I'm pretty sure the original reviewer of this code was drunk at the time: there's really no other explanation for approving of things like |<box orientation="horizontal"| or the strange and terrible |<vbox orientation="vertical"|.
Attachment #361118 - Flags: review?(philipp)
Assignee: nobody → philringnalda
Status: NEW → ASSIGNED
Comment on attachment 361118 [details] [diff] [review]
Hide my shame

>diff --git a/mail/base/content/messenger.xul b/mail/base/content/messenger.xul
>--- a/mail/base/content/messenger.xul
>+++ b/mail/base/content/messenger.xul
>@@ -169,17 +169,17 @@
> 
> <popup id="messageIdContext"/>
> 
> <tooltip id="folderpopup" class="folderSummaryPopup"/>
> 
>   <toolbox id="mail-toolbox" class="toolbox-top">
>   </toolbox>
>   <tabmail id="tabmail" flex="1" panelcontainer="tabpanelcontainer">
>-  <box id="tabmail-buttons" orientation="horizontal"/>
>+  <hbox id="tabmail-buttons"/>
>   <tabpanels id="tabpanelcontainer" flex="1" class="plain" selectedIndex="0">
>    <box id="mailContent" orient="vertical" flex="1">
>     <box id="messengerBox" orient="horizontal" flex="1" minheight="100" height="100" persist="height">

Is there a reason, why you are not converting those two boxes to a vbox/hbox (same for the box in line 220)?

And since you are touching mail/ as well, don't you need a Thunderbird reviewer here as well?
Oh, I missed them because I was only looking for the nonexistent orientation attribute, not the not really needed orient attribute.

And no, I'm claiming that I can delegate to you the power to review removing an attribute that doesn't exist from an element that only exists for you to hook into it :)
Sigh. Coffee first, bug comments second. Through the fog, those looked like something in your overlay, not messenger.xul.

Those two being <box>es with an orient attribute is part of the dynamic layout thing: View - Layout - Wide needs to set the orient property of a splitter to the value of the orient property on the mailContent box. While it's certainly possible that a vbox does a perfect job of having an orient property without having an orient attribute, we stole that code from Neil, who knows vastly more about XUL than I do and who did it with <box>es, so I'd want a really good reason for switching.
Attachment #361118 - Flags: review?(philipp) → review+
Comment on attachment 361118 [details] [diff] [review]
Hide my shame

Looks good to me, r=philipp
http://hg.mozilla.org/comm-central/rev/f6f782376998
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Adding orient attributes to h/vbox would have been confusing, so
I used a <box> as a reminder that the orientation was variable.
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.