need a wrapping xul box around the message reader

RESOLVED FIXED in Thunderbird 3.3a1

Status

Thunderbird
Message Reader UI
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: clarkbw, Assigned: clarkbw)

Tracking

Trunk
Thunderbird 3.3a1
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird3.1 rc1-fixed)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

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

8 years ago
Created attachment 425133 [details] [diff] [review]
wrap the message elements with an hbox so we can provide an extensible area for meta-message information
Assignee: nobody → clarkbw
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 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

8 years ago
Created attachment 425301 [details]
a sidebar that pushes over everything
(Assignee)

Comment 4

8 years ago
Created attachment 425303 [details]
a sidebar that only pushes over the message reader
(Assignee)

Comment 5

8 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 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

8 years ago
Created attachment 425346 [details] [diff] [review]
[checked-in on trunk] updated patch with proper line wrapping

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

8 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 9

8 years ago
meant to add the flag for checkin-needed
Keywords: checkin-needed
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?
Attachment #425346 - Attachment description: updated patch with proper line wrapping → [checked-in on trunk] updated patch with proper line wrapping
Pushed to comm-central as http://hg.mozilla.org/comm-central/rev/e91d06525b72
Keywords: checkin-needed
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.
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.)
Created attachment 425399 [details] [diff] [review]
My penitent patch to add tests.

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 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 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 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?
Attachment #425399 - Flags: approval-thunderbird3.1? → approval-thunderbird3.1+
Checked in:
http://hg.mozilla.org/comm-central/rev/edc66465261f
http://hg.mozilla.org/releases/comm-1.9.2/rev/6d3bde407940
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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.