Closed Bug 46134 Opened 24 years ago Closed 24 years ago

allow >1 value per attribute in XUL template substitution

Categories

(Core :: XUL, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: waterson, Assigned: waterson)

References

Details

(Keywords: memory-footprint, perf, Whiteboard: [nsbeta3-][rtm++]FIXED ON TRUNK)

Attachments

(7 files)

XUL templates need to support >1 "substitution property"; for example, using 
the simple template syntax:

  <treeitem
    class="rdf:http://home.netscape.com/NC-rdf#isMessage,
           rdf:http://home.netscape.com/NC-rdf#isRead" />

Or in the extended syntax:

  <treeitem class="$isMessage $isRead" />

Besides being nifty, this enables two key optimizations for large templates 
(like mail/news):

  1. It requires fewer attributes to be created on XUL elements, this
     accounts for the bulk of the expense in time when loading a
     mailnews folder.

  2. It allows the style rules to be written based on class, rather than
     on attribute. The style system hashes rules by class, so this makes
     style resolution O(1) rather than O(n).

We need to be a bit careful about how we do this to avoid excessive string 
munging and memory allocation. Ideally, supporting this will not degrade 
performance of the simple, single-substitution case.
Status: NEW → ASSIGNED
Keywords: footprint, nsbeta3, perf
Target Milestone: --- → M18
I was looking at this last night, and there's one thing that threw up a red flag
for me..

in mail, we have a few extra "static" classes for all treecells, currently in
the class attribute as class="treecell-indent tree-cell-threadpane-icon" and so
forth.

This means that we'll need to have support for both static text and
rdf-substituted text within the same attribute, so we should be sure to handle
the case where a token in the attribute does not contain rdf:
Whiteboard: [nsbeta3+]
Attached patch first cutSplinter Review
cc'ing scc for string fu.
Notes to self after re-reading patch...

- the case where we run off the end of the string is handled wrong.
- need to clarify & think about what might separate a variable name from other
content in the string
(I like nsPromiseString)  r=rjc (on 2nd cut)
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Hey Chris. I have a simple test case (stolen from pref-themes.xul) that shows
me that this is working. However, the substituted value for a multi-sustitution
is the concatenated string from the matches. In other words:

<treecell 
 value="rdf:http://ww...rg/rdf/chrome#name  rdf:http://ww..rg/rdf/chrome#display
Name"/>

gets the result "Classicclassic/1.0" instead of "Classic classic/1.0".

That, of course, doesn't give you the right thing, should you want to
use these matches to set css class="class-one class-two class-three".

I'll attach my testcase (and hopefully it doesn't have some bone-headed 
mistake).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Believe it or not, this was actually what I intended to do. The reasoning goes
something like this: the "end" of the variable is indicated using a space; that
space is consumed during processing; if you want to have a space in the
resulting template, you need to put *two* spaces in.

In retrospect, I think it was a dumb idea! I'll attach a patch shortly that will
fix this; if we ever do need the ability to jam something "right up next to" a
variable, then we'll add another special character that means "the variable ends
here".
Status: REOPENED → ASSIGNED
What the hell. The above patch gets it right (I think), and fixes another match
error to boot.

If you really want to concatenate, with no spaces in between, you can terminate
your variable name with a caret ("^") character (which, per RFC2396, must be
%-encoded if it appears in a URI for real). Otherwise, a variable simply ends
when a space is encountered.

The math error was a misunderstanding of how nsPromiseSubstring<*> worked: I'd
assumed the ctor took a start and end position; it actually takes a start
position and a length.
Sorry about that; I was following the old syntax from |Substring|, I think.  Is 
this not intuitive?  My bad.  In the future, the API will probably change to take 
a pair of iterators, as with |nsSlidingSubstring| now.  A special variation for 
flat strings will take a pair of character pointers.  This combination will be 
more efficient in every case, and will not encourage confusion since, when using 
iterators, points and distances have different data types; this is not so when 
using indexes.
Blessing with a r=rjc.
fix checked in.
no, really!
Status: ASSIGNED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
verified fixed. A space between two RDF URL remains a space, '^' can be used 
as a catenation operator, and a non-rdf string is not altered as an attribute
value, so 

 class="someClass  rdf:http...#prop1  rdf:http:...#prop2^rdf:http:...#prop3"
   appears as 
 class="someClass value1 value2value3"

This also works using the advanced syntax for templates. Sweet. 

(The "^" operator is not consumed when it trails a static value:
    |foobar^rdf:http://...#prop1|    appears as   |foobar^value1|             
although, it is consumed in this case
    |rdf:http://...#prop1^foobar|    appears as   |value1foobar|             
)
Status: RESOLVED → VERIFIED
In the last case, you can put two carets in; e.g.,

  |rdf:http://...#prop1^^foobar|

to produce:

  |value1^foobar|

Thanks for the extensive testing! I'll need to go update the template docs at
some point...
Actually, it would be nice to have the ^ operator work in the case of 
  
   staticname^rdf:..#prop1   producing   staticnameValue1

So that a datasource which, for example, has a property with two values, can
be convinced to produce a reasonably legible classname:

     message-status-^rdf:..#prop1   producing   message-status-unread
good lord, that would kick ass. I was just starting to think about that. this 
will allow us to speed up mail just by fixing XUL, not our datasources.
Once again, nice job chris!
jgrm: can't you just do "message-status-rdf:..#prop1" to get
"message-status-unread". The delimiter is really only needed on the back-end of
the variable name so I can know when to stop parsing the RDF property...
Indeed you can (I thought the ' rdf:' was the delimiter on the front end. 
So you produce classnames like:

    twostate-rdf:...#p1^-rdf...#p2    ->   twostate-true-false
I screwed this up, which I discovered when I was working on bug 53627 :-(. The
case that I missed was AddSimpleRuleBindings(), which computes the bindings for
the "simple" syntax. Attaching a fix that works, and also cleans up a bunch of
copy-n-paste of complicated code.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Blocks: 53627
Removing [nsbeta3+], but we'll need to reconsider if we decide to include the
fix for bug 53627...
Whiteboard: [nsbeta3+] → [nsbeta3-]
Status: REOPENED → ASSIGNED
Had to re-work the patch because egcs-1.1.2 can't stomach operator->*() and the
string template foo at the same time. So I made the callbacks be static members
of nsXULTemplateBuilder, and explicitly pass a "this" parameter to them. Gonna
verify the fancy C++ on gcc-2.7.2.3.

Oh, also fixed an ommission where I should've been looking for "..." as well as
"rdf:*".
Fix checked in on the trunk. Nominating for RTM because of 53627 dependency.
Keywords: rtm
Whiteboard: [nsbeta3-] → [nsbeta3-][rtm+]
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm+]FIXED ON TRUNK
Marking rtm++.  Please check this in as soon as possible.
Whiteboard: [nsbeta3-][rtm+]FIXED ON TRUNK → [nsbeta3-][rtm++]FIXED ON TRUNK
For posterity's sake (and in case we need to back it out), the last patch is the
patch that I am going to check in on the branch: it includes some gcc-2.7.2.3
cleanup, as well as mkaply's OS/2 cleanup.
Fixed on branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
Verified fixed as far as my pea-brain tells me (at least until waterson finds 
the next thing to fix :-]). marking vtrunk.
Keywords: vtrunk
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.