Closed Bug 953981 Opened 7 years ago Closed 7 years ago

Text Modifier for plaintext formatting

Categories

(Instantbird :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 543 at 2010-10-12 15:26:00 UTC ***

We're currently using mozTXTToHTMLConv to convert plaintext links, etc. into HTML links. There's also another mode that converts plaintext formatting (i.e. *bold*, /italics/ and _underline_ and |code|). Personally I'd really like this mode turned on since I tend to use a lot of plaintext formatting and certain protocols (IRC) don't support HTML, which would force plaintext formatting if you wish to do any.

If this is wanted I'd be willing to look at it since I'm relatively familiar with this code currently.
Attached patch Links & plaintext formatting (obsolete) — Splinter Review
*** Original post on bio 543 as attmnt 374 at 2010-10-13 21:53:00 UTC ***

Switched the code to use constants instead of the magic number "2" and enable both links and plaintext formatting.

I do have a question why do we use:
> msg.replace(/&/g, "&")
This same service has a function to escape "<", ">" and "&" automatically (and why are we not escape "<" & ">"?
Attachment #8352117 - Flags: review?(florian)
Assignee: nobody → clokep
Attached patch Links & plaintext formattin v2 (obsolete) — Splinter Review
*** Original post on bio 543 as attmnt 375 at 2010-10-13 22:39:00 UTC ***

Mic gave me a quick review on IRC. Changed the patch around for better work flow.
Comment on attachment 8352117 [details] [diff] [review]
Links & plaintext formatting

*** Original change on bio 543 attmnt 374 at 2010-10-13 22:39:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352117 - Attachment is obsolete: true
Attachment #8352117 - Flags: review?(florian)
*** Original post on bio 543 as attmnt 376 at 2010-10-13 22:42:00 UTC ***

An alternate way to do it by linking & formatting together using a flags statement. (I think this makes the code a lot cleaner.)

Some debate about how to make the flags variable.
I ended up using a ternary operator (not sure how we feel about those), although 

I originally had
> !!aMsg.noLinkifcation * txt2htmlconverter.kURLs
Which is a really ugly type conversion to force false to 0 and !false to be true, which is 1.
Attachment #8352119 - Flags: review?(florian)
Comment on attachment 8352119 [details] [diff] [review]
Links & plaintext formatting together

*** Original change on bio 543 attmnt 376 at 2010-10-13 23:53:29 UTC ***

>diff --git a/instantbird/content/convbrowser.xml b/instantbird/content/convbrowser.xml
>--- a/instantbird/content/convbrowser.xml
>+++ b/instantbird/content/convbrowser.xml
>+            // Automatically find and link freetext URLs and/or autoformat for
>+            // plaintext formatting (i.e. *bold* /italics/ _underline_ |code|)
>+            let converterFlags = aMsg.noLinkifcation ? 0 : txt2htmlconverter.kURLs |
>+                                 txt2htmlconverter.kStructPhrase;

I just realized what Mic was asking me earlier and that this is wrong. It should be:
>+            let converterFlags = (aMsg.noLinkifcation ? 0 : txt2htmlconverter.kURLs) |
>+                                 txt2htmlconverter.kStructPhrase;
I will post an updated patch soon.
Attachment #8352119 - Flags: review?(florian)
*** Original post on bio 543 at 2010-10-14 07:35:24 UTC ***

(In reply to comment #4)
> (From update of attachment 8352119 [details] [diff] [review] (bio-attmnt 376) [details])
> >diff --git a/instantbird/content/convbrowser.xml b/instantbird/content/convbrowser.xml
> >--- a/instantbird/content/convbrowser.xml
> >+++ b/instantbird/content/convbrowser.xml
> >+            // Automatically find and link freetext URLs and/or autoformat for
> >+            // plaintext formatting (i.e. *bold* /italics/ _underline_ |code|)
> >+            let converterFlags = aMsg.noLinkifcation ? 0 : txt2htmlconverter.kURLs |
> >+                                 txt2htmlconverter.kStructPhrase;
> 
> I just realized what Mic was asking me earlier and that this is wrong. It
> should be:
> >+            let converterFlags = (aMsg.noLinkifcation ? 0 : txt2htmlconverter.kURLs) |
> >+                                 txt2htmlconverter.kStructPhrase;
> I will post an updated patch soon.

There's a typo in "aMsg.noLinkifcation" btw.

For me the ternary if with the OR behind it looks unreadable. I think it might be clearer after exchanging the two pieces:

let a = b | (c ? d : e);

Or leave the conditional operator away at all (I don't like the 'dead part' (i.e. 0) in it) and use an old-school-if

// kStructPhrase creates tags for plaintext-markup like *bold*, /italics/, ..
//  we do it always as it will get filtered by the content filter if the 
//  user disapproves, blabla
let csFlags = cs.kStructPhrase;
if (!aMsg.noLinkification)
  cs.Flags |= cs.kURLs;

cs is for 'converter service' here. It's only on about five lines of code, that should be good enough for anyones short-term memory and reduces the character count considerably.
*** Original post on bio 543 as attmnt 387 at 2010-10-24 18:46:00 UTC ***

Takes some of what Mic suggested as well as some things I talked to flo about on IRC.  Tested the following test cases:
Input (Display of Proper Output)
&nbsp; (&nbsp;)
/raw &nbsp; ( )
&amp; (&amp;)
/raw &amp; (&)
&quot; (&quot;)
/raw &quot; (")
Attachment #8352130 - Flags: review?(florian)
Comment on attachment 8352118 [details] [diff] [review]
Links & plaintext formattin v2

*** Original change on bio 543 attmnt 375 at 2010-10-24 18:46:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352118 - Attachment is obsolete: true
Comment on attachment 8352119 [details] [diff] [review]
Links & plaintext formatting together

*** Original change on bio 543 attmnt 376 at 2010-10-24 18:46:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352119 - Attachment is obsolete: true
Attached patch Patch v3Splinter Review
*** Original post on bio 543 as attmnt 389 at 2010-10-25 23:11:00 UTC ***

Fixed the _underline_ feature.
Attachment #8352132 - Flags: review?(clokep)
Comment on attachment 8352132 [details] [diff] [review]
Patch v3

*** Original change on bio 543 attmnt 389 at 2010-11-03 15:28:02 UTC ***

Looks good. I'm going to file a bug with mozilla about the .moz-txt-underscore class though -- there's no reason we should have to add that.
Attachment #8352132 - Flags: review?(clokep) → review+
Comment on attachment 8352130 [details] [diff] [review]
Links & plaintext formatting together v2

*** Original change on bio 543 attmnt 387 at 2010-11-03 16:50:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352130 - Attachment is obsolete: true
Attachment #8352130 - Flags: review?(florian)
*** Original post on bio 543 at 2010-11-03 19:19:23 UTC ***

https://hg.instantbird.org/instantbird/rev/2e7b5aaa710c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3a1
*** Original post on bio 543 at 2010-11-03 19:20:41 UTC ***

(In reply to comment #8)
> (From update of attachment 8352132 [details] [diff] [review] (bio-attmnt 389) [details])
> Looks good. I'm going to file a bug with mozilla about the .moz-txt-underscore
> class though -- there's no reason we should have to add that.

The bug is https://bugzilla.mozilla.org/show_bug.cgi?id=609303
Blocks: 954087
You need to log in before you can comment on or make changes to this bug.