Closed
Bug 544162
Opened 14 years ago
Closed 14 years ago
need a wrapping xul box around the message reader
Categories
(Thunderbird :: Message Reader UI, defect)
Thunderbird
Message Reader UI
Tracking
(thunderbird3.1 rc1-fixed)
RESOLVED
FIXED
Thunderbird 3.3a1
Tracking | Status | |
---|---|---|
thunderbird3.1 | --- | rc1-fixed |
People
(Reporter: clarkbw, Assigned: clarkbw)
References
()
Details
Attachments
(4 files, 1 obsolete file)
99.15 KB,
image/png
|
Details | |
75.14 KB,
image/png
|
Details | |
2.38 KB,
patch
|
clarkbw
:
review+
standard8
:
approval-thunderbird3.0.2-
|
Details | Diff | Splinter Review |
4.12 KB,
patch
|
dmosedale
:
review+
standard8
:
approval-thunderbird3.1+
|
Details | Diff | Splinter Review |
It would make a number of extensions possible if we had a wrapping box around the message reader elements. The goal is to be able to offer a side bar which could contain extra meta information about a message. To do this we should wrap all the elements we feel "contain" the message. I'm avoiding wrapping the message header because of it's size and the existing problem with moving the buttons around overrunning the header text. One current example is the Data Miners extension. Data Miners has extra information about the message like attachments, links, phone numbers, email addresses, and other various interesting pieces we can recognize. Will attach example patch shortly.
Assignee | ||
Comment 1•14 years ago
|
||
Assignee: nobody → clarkbw
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•14 years ago
|
||
More details to discuss this problem a bit more. Here's the current single message layout in trimmed code: <vbox id="singlemessage" flex="1"> <hbox id="msgHeaderView"/> <hbox id="editMessageBox"/> <deck id="msgNotificationBar"/> <hbox id="messagepanewrapper" flex="1"> <browser id="messagepane"/> </hbox> <splitter id="attachment-splitter"/> <hbox id="attachmentView"/> <findbar id="FindToolbar"/> </vbox> ---------------------------------------- | header | ---------------------------------------- | edit-draft | ---------------------------------------- | notifications (junk, phish) | ---------------------------------------- | message | | reader | ---------------------------------------- | attachments | ---------------------------------------- | find bar | ---------------------------------------- We want under the header but I'm not sure that we want just in the message reader. example: ---------------------------------------- | header | ---------------------------------------- | edit-draft | ---------------------------------------- | notifications (junk, phish) | ---------------------------------------- | message | meta | | reader | sidebar | ---------------------------------------- | attachments | ---------------------------------------- | find bar | ---------------------------------------- alternate possibility: ---------------------------------------- | header | ---------------------------------------- | edit-draft | | -----------------------------| | | notifications (junk, phish)| | -----------------------------| | | message | meta | | reader | sidebar | -----------------------------| | | attachments | | -----------------------------| | | find bar | | ---------------------------------------- The problem with this design is that the notifications bar is already tight for horizontal space so taking away from that could cause issues. A final note is that the meta sidebar could be on either side of the message reader. A question that was raised by someone else, an overlay would allow you to insert something into either side of the message reader area.
Assignee | ||
Comment 3•14 years ago
|
||
Assignee | ||
Comment 4•14 years ago
|
||
Assignee | ||
Comment 5•14 years ago
|
||
Comment on attachment 425133 [details] [diff] [review] wrap the message elements with an hbox so we can provide an extensible area for meta-message information Going with just the messagepane being wrapped. I'm not worried about the find bar coming under any sidebars, I don't see that have a real detrimental effect. This hbox is just to show off information about the message content and nothing else so it seems fitting to make it just wrap that piece.
Attachment #425133 -
Flags: review?(bwinton)
Comment 6•14 years ago
|
||
Comment on attachment 425133 [details] [diff] [review] wrap the message elements with an hbox so we can provide an extensible area for meta-message information Works for me, and I think we've picked the correct set of things to wrap. As a nit, it would be nice if you re-wrapped stuff to not exceed 80-characters. Later, Blake.
Attachment #425133 -
Flags: review?(bwinton) → review+
Assignee | ||
Comment 7•14 years ago
|
||
do I get to say, "and that's a wrap!"? carrying forward the r+ from bwinton
Attachment #425133 -
Attachment is obsolete: true
Attachment #425346 -
Flags: review+
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #6) > (From update of attachment 425133 [details] [diff] [review]) > Works for me, and I think we've picked the correct set of things to wrap. > > As a nit, it would be nice if you re-wrapped stuff to not exceed 80-characters. Note to the reader that I also wrapped the splitter at 80 char while I was right there next to it.
Assignee | ||
Comment 10•14 years ago
|
||
Comment on attachment 425346 [details] [diff] [review] [checked-in on trunk] updated patch with proper line wrapping would it be possible to land this in the 3.0s?
Attachment #425346 -
Flags: approval-thunderbird3.0.2?
Updated•14 years ago
|
Attachment #425346 -
Attachment description: updated patch with proper line wrapping → [checked-in on trunk] updated patch with proper line wrapping
Comment 11•14 years ago
|
||
Pushed to comm-central as http://hg.mozilla.org/comm-central/rev/e91d06525b72
Keywords: checkin-needed
Comment 12•14 years ago
|
||
Can you add an XML comment into there to explain what the point of the box is? When reading the XUL I have no clue why we need a wrapper in there. It's the kind of thing that makes the codebase brittle because we don't know why things are there and we are afraid to touch them without undertaking a search of the code base. In this case, since the point of this is to enable an extension, we would find nothing in our search and infer it's fine to remove the XUL box.
Comment 13•14 years ago
|
||
Sounds good. I'll do that as part of the patch to add a test, which should be coming sometime soon. (I'm thinking tonight or tomorrow, depending on when I pack.)
Comment 14•14 years ago
|
||
asuth jokingly suggested that we could just check that there was an element with the correct id, but that's kind of actually what we're depending on, and a test for that would catch the case where someone cleaned up the element (by causing the "test_messagepane_extension_points_exist" test to fail, which is hopefully enough of a hint that that element is necessary for extensions). Anyways, I've added a comment to the XML, as suggested, and a new test, which can also provide us a place to document the other extension points we promise to provide. Thanks, Blake.
Attachment #425399 -
Flags: review?(dmose)
Comment 15•14 years ago
|
||
Comment on attachment 425346 [details] [diff] [review] [checked-in on trunk] updated patch with proper line wrapping The more I think about this, the more I don't like it for the stable branch. If any extension depends on the location of the messagepane browser in the DOM tree (e.g. insertafter/before), then we'll break them. Additionally any extension developer who wants to use this in the 3.0.x series will have to have a min version of 3.0.2 (which AMO won't allow unless we explicitly ask for it) or some other hack to account for the version, and when we get that far, we realise this is equivalent to an API change on a stable branch which we just don't allow.
Attachment #425346 -
Flags: approval-thunderbird3.0.2? → approval-thunderbird3.0.2-
Comment 16•14 years ago
|
||
Comment on attachment 425399 [details] [diff] [review] My penitent patch to add tests. r=dmose; thanks for the patch.
Attachment #425399 -
Flags: review?(dmose) → review+
Comment 17•14 years ago
|
||
Comment on attachment 425399 [details] [diff] [review] My penitent patch to add tests. Does increasing test coverage get an a+? Thanks, Blake.
Attachment #425399 -
Flags: approval-thunderbird3.1?
Updated•14 years ago
|
Attachment #425399 -
Flags: approval-thunderbird3.1? → approval-thunderbird3.1+
Comment 18•14 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/edc66465261f http://hg.mozilla.org/releases/comm-1.9.2/rev/6d3bde407940
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
status-thunderbird3.1:
--- → rc1-fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.2a1
You need to log in
before you can comment on or make changes to this bug.
Description
•