192458 - Need option to insert table in plain text mail

Status: NEW

(MailNews Core :: Composition, enhancement)

Description

[Matthias Bockelkamp]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030208
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030208

I need an option to insert a table construct in a plain text mail which could
then be filled by user in insert mode, e.g.

#------#------#------#
|      |      |      |
#------#------#------#
|      |      |      |
#------#------#------#
|      |      |      |
#------#------#------#

Perhaps cell width could be a preference, so that the user is only asked which
dimensions (rows and colums) the table construct should have.

Maybe the code to be written for Bug 18012 could be used.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Ben Bucksch (:BenB)]

If you're using the HTML-editor, this is a dup of bug 18012 insofar as fixing
that bug would fix this one, I think. The HTML composer already allows you to
insert (HTML tables). If they are then converted to plaintext during send, you
should get the desired result.

Comment 2

[Matthias Bockelkamp]

I think there should be an easier way to do this without using the HTML-editor,
so I wouldn't call this a dup. It doesn't need to be in the context menu, an
entry in the edit menu would be OK since this won't be used that much.

Comment 3

[Matthias Bockelkamp]

Attachment: Patch

This is mostly stolen from the HTML editor. I'm not sure about the licence
header for the three new files. Licence Boilerplate sais all new Mozilla files
should use the MPL/LGPL/GPL tri-license, but the files I reused contain the
NPL.

Of course this patch were much more useful if Bug #38415 was resolved. I'll
attach a dependency.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 128781 [details] [diff] [review]
Patch

I don't really know any of this code. changing review request to neil; but
glazou or brade may be a better choice

Comment 5

[[e-mail]]

Comment on attachment 128781 [details] [diff] [review]
Patch

I'm not sure that you put the menuitem in the right place. it looks as if you
see the menuitem in HTML compose too, which is somewhat pointless unless you
force a monospace font, although it's probably not worth the trouble.

>+  doCommand: function(aCommand)
>+  {
>+    EditorInsertPlaintextTable();
>+  }
As there's no such function, I wasn't able to test the code. In fact you
probably don't need a separate function, this is probably supposed to be a
window.openDialog() thingy.

>+function onAccept()
>+{
>+  gRows    = gDialog.rowsInput.value;
>+  gColumns = gDialog.columnsInput.value;
>+  gWidth   = gDialog.widthInput.value;
>+  gHeight  = gDialog.heightInput.value;
These shouldn't be global.

>+  gActiveEditor.beginTransaction();
If you created one big string for all the table you wouldn't need a
transaction. Also your string should begin and end with a newline.

>+  // Create necessary rows and cells for the table
>+  for (var r = 0; r < gRows; r++)
>+  {
>+    gActiveEditor.insertText(CreateTableLine(gColumns,gWidth,'#','-'));
>+    for (var h = 0; h < gHeight; h++)
>+    {
>+      gActiveEditor.insertText(CreateTableLine(gColumns,gWidth,'|',' '));
>+    }
>+  }
>+  gActiveEditor.insertText(CreateTableLine(gColumns,gWidth,'#','-'));
You shouldn't be calling these functions in loops. Just create one of each type
of line and then join them together.

>+function CreateTableLine(cols, width, c1, c2)
>+{
>+  res = "";
>+  for (var c = 0; c < cols; c++)
>+  {
>+    res += c1;
>+    for (var w = 0; w < width; w++)
>+    {
>+      res += c2;
>+    }
>+  }
>+  res += c1;
In fact you shouldn't even be nesting these loops. Use one loop to repeat "-"
to create "#----------" and another loop to repeat that.

Comment 6

[Matthias Bockelkamp]

Attachment: Patch v2

Comment 7

[Scott MacGregor]

Comment on attachment 142738 [details] [diff] [review]
Patch v2

You should have Daniel glazman look at this first to make sure you have module
owner approval.

Of the bat, I'd suggest using some constants instead of raw integers for 

+  // Set initial number to 4 rows, 4 columns, cell width 16, cell height 1
+  gDialog.rowsInput.value    = 2;
+  gDialog.columnsInput.value = 2;
+  gDialog.widthInput.value   = 8;
+  gDialog.heightInput.value  = 1;

Comment 8

[Matthias Bockelkamp]

Comment on attachment 142738 [details] [diff] [review]
Patch v2

Changing review request to Daniel Glazman. The magic numbers will be replaced
by constants as soon as I know what else needs to be polished.

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Holy cow... Isn't ASCII art meant to be edited only by hand ?-)

mscott: thanks for heads up

matthias: I'll review that tomorrow but I can already see that you did not
use the tri-license boilerplate for the new files you are creating. According
to the new licensing terms IIRC , NPL should be moved to tri-license.

brade,akkana,jfrancis: please have a look to this bug if you have 5 minutes. Is
it something we could/should add to the plaintext Composer too ? I have no
opinion yet, just wondering.

Comment 10

[Kathleen :Brade]

Comment on attachment 142738 [details] [diff] [review]
Patch v2

personally, I don't find it useful for the class names to be aligned in the
registerCommand block.	I wouldn't change all of those lines; just add your own
line with one space before the class.  [Daniel or someone else may disagree.]

I'd like to see more comments in these places:	class definition of
nsInsertPlaintextTableCommand,	and explaining how "rowsInput.value = 2" gives
4 rows (same with column and width)

Shouldn't this really be an extension?	I'm not thrilled that we have to bloat
Composer for this functionality that seems mail-specific (but perhaps I just
don't understand well enough what the feature is?)

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

This is clearly an excellent comment from Kathy : Matthias, could your solution
become an XPI for the Mozilla Application Suite and Thunderbird instead of
something built by default ?

Mscott, what do you think ? From a Composer point of view, I think we don't need
the feature so the primary client of this extension is email. Is this a feature
that a majority of Thunderbird users is likely to use, or is it a geeky feature
made for a very little number of ascii art lovers ?

Comment 12

[Akkana Peck]

> or is it a geeky feature
> made for a very little number of ascii art lovers ?

Almost certainly that.  Though it is a cool geeky feature, no doubt about that.

I'm inclined to say it should be an extension, if possible, because I doubt very
many people would want or discover this feature.  I think that should be easy,
since mostly it's just JS creating a string then telling the editor to insert a
string.

(Aside: I sure would love to see it solved instead by using the html editor and
fixing bug 18012 (table output in html-to-text conversion) ...  in case you ever
get the urge to try that.)  Then we could argue about whether there should be a
(hidden?) UI to insert a table in the plaintext compose window ...

Comment 13

[Matthias Bockelkamp]

Comment on attachment 142738 [details] [diff] [review]
Patch v2

doesn't apply anymore