Bug 47359 - [PATCH] Nested baseline-shift are not additive
Summary: [PATCH] Nested baseline-shift are not additive
Status: NEW
Alias: None
Product: Batik - Now in Jira
Classification: Unclassified
Component: GVT Text (show other bugs)
Version: 1.8
Hardware: All All
: P3 normal
Target Milestone: ---
Assignee: Batik Developer's Mailing list
URL: http://www.nabble.com/nesting-baselin...
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2009-06-12 00:41 UTC by Nicolas Socheleau
Modified: 2009-06-16 13:28 UTC (History)
1 user (show)



Attachments
testcase for baseline-shift property (1.42 KB, image/svg+xml)
2009-06-12 00:41 UTC, Nicolas Socheleau
Details
better formatted testcase (3.63 KB, image/svg+xml)
2009-06-12 01:59 UTC, Nicolas Socheleau
Details
fix attempt (6.24 KB, patch)
2009-06-12 02:02 UTC, Nicolas Socheleau
Details | Diff
fix attempt 2 (6.32 KB, patch)
2009-06-16 08:59 UTC, Nicolas Socheleau
Details | Diff
animation of baseline-script property (2.14 KB, image/svg+xml)
2009-06-16 09:12 UTC, Nicolas Socheleau
Details
scripting of the baseline-shift property (1.85 KB, image/svg+xml)
2009-06-16 09:13 UTC, Nicolas Socheleau
Details
minor formatting fix (6.36 KB, patch)
2009-06-16 13:24 UTC, Nicolas Socheleau
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nicolas Socheleau 2009-06-12 00:41:52 UTC
Created attachment 23803 [details]
testcase for baseline-shift property

According to the spec (http://www.w3.org/TR/SVG11/text.html#BaselineShiftProperty),
baseline-shift properties can nest and are additive but not inheritable.

the following example is expected to render text on the same line though the tspan is back on the baseline:

<text baseline-shift="10pt">the quick <tspan fill="brown">brown</tspan> fox</text>

the following example is expected to render the tspan within another tspan higher up compared to the baseline:

<text> using <tspan baseline-shift="super">super <tspan baseline-shift="5pt">super!</tspan> script</tspan></text>

Attached are testcases for the baseline-shift property
Comment 1 Nicolas Socheleau 2009-06-12 01:59:49 UTC
Created attachment 23804 [details]
better formatted testcase

testcase formatted for the batik test suite
Comment 2 Nicolas Socheleau 2009-06-12 02:02:36 UTC
Created attachment 23805 [details]
fix attempt

this is a proposed patch, but I'm rusty and I haven't followed much lately any development on Batik, so I'm not sure it is valuable. Still, the rendering of baseline-shift seems correct with this patch.
Comment 3 Helder Magalhães 2009-06-13 01:07:25 UTC
First of all, thanks for the report and suggested fix, Nicolas! ;-)

Few bug properties meta changes:
 * Version moved to 1.8 as the issue was confirmed in trunk (revision 784345);
 * Component changed to GVT Text as, although Bridge also seems to be involved, conceptually this concerns text handling (I'm not very confident about this although IMO it sounds better);
 * Added [PATCH] prefix and related keyword in order to reflect the fact that there's a fix proposal available;
 * Added the mailing list discussion prior to issue creation as the report's URL (a bit of background for the curious). :-)

I'll try to follow-up with a set of comments regarding each of the attachments.
Comment 4 Helder Magalhães 2009-06-13 02:02:23 UTC
(In reply to comment #1)
> Created an attachment (id=23804) [details]
> better formatted testcase
> 
> testcase formatted for the batik test suite

This should apparently be placed in "samples/tests/spec/text" (quick tip to save time). ;-)

As the test is basically a small extend of "textLayout2.svg", I'd propose merging the new sub-tests ('<text> baseline-shift="-5pt" / baseline-shift="+/-10pt"' and 'nested <tspan>') into it ("textLayout2.svg"), maybe by using two or more columns such as in "textLayout.svg", and replace this attachment for a patch against that test ("textLayout2.svg"). Note that, for coherency with "textLayout2.svg", I'd propose dropping the guide lines used (or adding them consistently in "textLayout2.svg" also).
Comment 5 Helder Magalhães 2009-06-13 02:33:56 UTC
(In reply to comment #2)
> Created an attachment (id=23805) [details]
> fix attempt
> 
> this is a proposed patch, but I'm rusty and I haven't followed much lately any
> development on Batik, so I'm not sure it is valuable. Still, the rendering of
> baseline-shift seems correct with this patch.

Intuitively sounds good, although I haven't tested the patch myself yet. Few comments first:


Batik uses the space character for indenting. Please update your patch (which uses a mix of tabs and spaces) with this in mind. ;-)


Missing space before "asb" in:

+        fillAttributedStringBuffer(ctx, element, true, null, null, null, null,asb);


Missing space before "elementAttributes" in:

+	parentAttributes = handleNestedBaselineShift(parentAttributes,elementAttributes);


Is it safe to assume "elementAttributes" is always non-null? That is, should we be making the same null check made for the "parentAttributes" nearby?

+        if ( elementAttributes.containsKey(BASELINE_SHIFT) )


I'd say the space before the semi-colon could be dropped in:
+                    for(int k = 0 ; k < values.size(); k++)


Although this portion was adapted from previous code, I'd propose that spaces were inserted between "==" and also the "+= -" was changed to "-=" in:

+                            if (baselineValue==TextAttribute.SUPERSCRIPT_SUPER) {
+                                baselineAdjust += baselineAscent*0.5f;
+                            } else if (baselineValue==TextAttribute.SUPERSCRIPT_SUB) {
+                                baselineAdjust += -baselineAscent*0.5f;


With these small changes addressed, I'd say the patch is ready for review. ;-)


Apart from the changes, I'd just like to leave a couple of thoughts which I haven't been able to dig down properly (at least, not for now):
 * Is this code ready for on-the-fly modification of the baseline-shift property? By scripting or by placing a SMIL animation on the property, either in the child or in the parent for a nesting sample, does the proposal keep up with?
 * Can the introduced memory allocations be introducing any memory leaks?
Comment 6 Helder Magalhães 2009-06-13 02:48:00 UTC
(In reply to comment #4)
> As the test is basically a small extend of "textLayout2.svg", I'd propose
> merging the new sub-tests [...] into it ("textLayout2.svg") [...]

Oops, forgot to mention that, if one decides to keep the two tests separately, the newly introduced should be also added to the test list ("test-resources/org/apache/batik/test/samplesRendering.xml") so regard [1] can see it. A reference image needs to be generated also.

If the couple of introduced sub-tests are merged into the existing one, only the reference image needs to be updated. ;-)

[1] http://xmlgraphics.apache.org/batik/dev/test.html#regard
Comment 7 Nicolas Socheleau 2009-06-16 08:59:48 UTC
Created attachment 23811 [details]
fix attempt 2

I updated the patch to take into account Helder Magalhães commments
Comment 8 Nicolas Socheleau 2009-06-16 09:04:59 UTC
> Batik uses the space character for indenting. Please update your patch (which
> uses a mix of tabs and spaces) with this in mind. ;-)
> 
> 
> Missing space before "asb" in:
> 
> +        fillAttributedStringBuffer(ctx, element, true, null, null, null,
> null,asb);
> 
> 
> Missing space before "elementAttributes" in:
> 
> +    parentAttributes =
> handleNestedBaselineShift(parentAttributes,elementAttributes);
> 
> 

I've addressed those issues in the 2nd submitted patch.

> Is it safe to assume "elementAttributes" is always non-null? That is, should we
> be making the same null check made for the "parentAttributes" nearby?
> 
> +        if ( elementAttributes.containsKey(BASELINE_SHIFT) )
> 

handleNestedBaselineShift( Map, Map ) is called from one location only and a few lines before the call, elementsAttributes is set to new HashMap(). I assume it is safe not to check elementAttributes within that method.

> 
> I'd say the space before the semi-colon could be dropped in:
> +                    for(int k = 0 ; k < values.size(); k++)
> 
> 
> Although this portion was adapted from previous code, I'd propose that spaces
> were inserted between "==" and also the "+= -" was changed to "-=" in:
> 
> +                            if
> (baselineValue==TextAttribute.SUPERSCRIPT_SUPER) {
> +                                baselineAdjust += baselineAscent*0.5f;
> +                            } else if
> (baselineValue==TextAttribute.SUPERSCRIPT_SUB) {
> +                                baselineAdjust += -baselineAscent*0.5f;
> 
> 

adjusted in the latest patch

> With these small changes addressed, I'd say the patch is ready for review. ;-)
> 
> 
> Apart from the changes, I'd just like to leave a couple of thoughts which I
> haven't been able to dig down properly (at least, not for now):
>  * Is this code ready for on-the-fly modification of the baseline-shift
> property? By scripting or by placing a SMIL animation on the property, either
> in the child or in the parent for a nesting sample, does the proposal keep up
> with?

I've attached test file for animation and scripting as well for the review.
Comment 9 Nicolas Socheleau 2009-06-16 09:12:42 UTC
Created attachment 23814 [details]
animation of baseline-script property

testcase with <animate/> and <set/> to modify the baseline-shift property
Comment 10 Nicolas Socheleau 2009-06-16 09:13:33 UTC
Created attachment 23815 [details]
scripting of the baseline-shift property

testcase with onload ecmascript to modify the baseline-shift property
Comment 11 Helder Magalhães 2009-06-16 11:20:13 UTC
(In reply to comment #8)
> > Missing space before "asb" in: [code snippet]
> > Missing space before "elementAttributes" in: [code snippet]
> I've addressed those issues in the 2nd submitted patch.

Apparently not those two items. ;-)


> +                    for(int k = 0; k < values.size(); k++)
> +                    {
> +                        Object baselineValue = values.get(k);

Could/should the "Object" variable be moved to outside the cycle? Although the nesting will probably never be more than one level...


> +         //for the child of the element

Seems that a superfluous (indenting) space slipped into that line. ;-)


> handleNestedBaselineShift( Map, Map ) is called from one location only and a
> few lines before the call, elementsAttributes is set to new HashMap().
> I assume it is safe not to check elementAttributes within that method.

Sounds reasonable. :-)


> I've attached test file for animation and scripting as well for the review.

Cool, I noticed the attachments but didn't have the chance to test them yet. Nevertheless, you didn't state whether the patch properly handled those situations (although you working suggests so).


I'd say that, even without the minor formatting stuff integrated, the patch is ready for a deeper review from someone with a deeper knowledge on Batik's internals (assuming that the animation cases are being properly handled, as guessed in the previous paragraph). ;-)
Comment 12 Nicolas Socheleau 2009-06-16 13:24:15 UTC
Created attachment 23820 [details]
minor formatting fix

- fix the 2 missing space between the comma and the parameter name (this time;)
- remove the extra space
- move the local variable outside of the loop
Comment 13 Nicolas Socheleau 2009-06-16 13:28:13 UTC
> 
> Apparently not those two items. ;-)

I hope I got it right this time :)

> > +                    for(int k = 0; k < values.size(); k++)
> > +                    {
> > +                        Object baselineValue = values.get(k);
> 
> Could/should the "Object" variable be moved to outside the cycle? Although the
> nesting will probably never be more than one level...

I moved it outside of the loop

> > +         //for the child of the element
> 
> Seems that a superfluous (indenting) space slipped into that line. ;-)
> 
you got me!

> > I've attached test file for animation and scripting as well for the review.
> 
> Cool, I noticed the attachments but didn't have the chance to test them yet.
> Nevertheless, you didn't state whether the patch properly handled those
> situations (although you working suggests so).
> 

the scripting and animation test files are working with the patch. Though, I'm still working on some other animations where I don't get what I expect. 
I'm starting to fix it is a different issue. I'll keep digging...