Closed Bug 237538 Opened 16 years ago Closed 13 years ago

Text munger effects depends on nesting order

Categories

(Other Applications :: ChatZilla, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla-mozilla-20000923, Assigned: Gijs)

References

Details

(Whiteboard: [cz-0.9.78])

Attachments

(1 file, 5 obsolete files)

In short, _foo |bar| baz_ works, but |foo _bar_ baz| misses the tele-type
effect. This is, I think, because the munger applies the rules in the order it
finds, and thus matches _/_ before |/|. Fixing will not be that pretty, but
could be done in the following way:
  - apply each regex or lambdaMatch function in turn, and keep track of the one
with the lowest index within the text.
  - run the lambdaReplace or "simple munger" code for the munger found with the
lowest start index.
Product: Core → Other Applications
Assignee: rginda → silver
Attached patch Existing WIP patch (obsolete) — Splinter Review
This patch is ridiculously large and such. I'll spend a moment to try and move everything back so that at least the patch is smaller. I don't want that other version checked in, but it should be easier to see what the ... James and I did in that one.
Assignee: silver → gijskruitbosch+bugs
Attachment #244589 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #244672 - Flags: review?(samuel)
OK, this should do I hope.
self-nit-note-for-checkin: remove the semicolons after the helper functions at the end of munger.js
Depends on: 361914
Comment on attachment 244672 [details] [diff] [review]
Full Patch, moving cz's mungers to new mungers.js file in xul/content/

>+function CMungerEntry(name, regex, className, enable, startPriority, tagName)

This doesn't match how it's called.

> CMunger.prototype.addRule =
>+function mng_addrule(name, regex, className, priority, startPriority, enable)
[...]
>+    var entry = new CMungerEntry(name, regex, className, priority,
>+                                 startPriority, enable);

This is how it's called.

>+    if (!(priority in this.entries))
>+        this.entries.length = Math.max(this.entries.length, priority);

Why is this necessary?

> CMunger.prototype.munge =
>+function mng_munge(text, containerTag, data)
[...]
>+        for (var i = this.entries.length; i >= 0; i--)
>         {
>+            if (i in this.entries)

Why not just |for (var i in this.entries)| like you used elsewhere?

>+            // There is no need to munger the "before" text, as we should

munger -> munge
Attachment #244672 - Flags: review?(samuel) → review-
Depends on: 367903
Re: ssieb's review:
- Corrected the method signature mistake. Oops.
- Removed the array length extension, which was useless.
- Left the loop code as-is, because looping with for ... in would be in the wrong order (which is important there) and it would also take along other things done to the array variable (I'm not sure if it'd list "length" but I seem to remember it would. Which would break the code.
- Corrected the spelling/grammar oopsie.

Additional stuff:
- Added CSS and code to make all kinds of links retain mIRC colour/style markup.
- Added some checks so the munger wouldn't call insertText on empty strings (adding useless empty spans and <wbr/>s)
- Took along fixes for the .mailto and bugzilla mungers from the dep bugs. This patch applies to trunk again. :-)
Attachment #244672 - Attachment is obsolete: true
Attachment #244674 - Attachment is obsolete: true
Attachment #244675 - Attachment is obsolete: true
Attachment #255576 - Flags: review?(samuel)
Attached patch Working patchSplinter Review
Ugh. I was too happy copy-pasting stuff in that last patch. This one actually works. I also fixed some more classes in the CSS and removed the strike-through one because we don't use it at all.
Attachment #255576 - Attachment is obsolete: true
Attachment #255578 - Flags: review?(samuel)
Attachment #255576 - Flags: review?(samuel)
Nit: delete data.isUnder in mircResetColor should be delete data.isUnderline (see http://lxr.mozilla.org/seamonkey/source/extensions/irc/xul/content/static.js#1229 for comparison).
Attachment #255578 - Flags: review?(samuel) → review+
Checking in mozilla/extensions/irc/jar.mn;
new revision: 1.31; previous revision: 1.30
Checking in mozilla/extensions/irc/xul/content/commands.js;
new revision: 1.119; previous revision: 1.118
Checking in mozilla/extensions/irc/xul/content/handlers.js;
new revision: 1.146; previous revision: 1.145

RCS file: /cvsroot/mozilla/extensions/irc/xul/content/mungers.js,v
Checking in mozilla/extensions/irc/xul/content/mungers.js;
initial revision: 1.1

Checking in mozilla/extensions/irc/xul/content/output-base.css;
new revision: 1.14; previous revision: 1.13
Checking in mozilla/extensions/irc/xul/content/output-window.js;
new revision: 1.18; previous revision: 1.17
Checking in mozilla/extensions/irc/xul/content/scripts.xul;
new revision: 1.13; previous revision: 1.12
Checking in mozilla/extensions/irc/xul/content/static.js;
new revision: 1.230; previous revision: 1.229
Checking in mozilla/extensions/irc/xul/lib/munger.js;
new revision: 1.24; previous revision: 1.23

--> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [cz-0.9.78]
Depends on: 372874
You need to log in before you can comment on or make changes to this bug.