Bug 50006

(In reply to comment #0)
> Created an attachment (id=26077) [details]
> Patch to only dump kerning table elements requested by the user
> 
> ttf2svg dumps the entirety of the kerning table instead of the subset in the
> low/high range given by the user.

Sounds like a good idea - I'm an adept of optimizations. :-)


(from the patch)
> -                    ps.println(getKerningPairAsSVG(kst.getKerningPair(i), post));
> +                	int left = kst.getKerningPair(i).getLeft();
> +                	int right = kst.getKerningPair(i).getRight();
> +                	boolean left_in_range = (first <= left && left <= last);
> +                	boolean right_in_range = (first <= right && right <= last);
> +                	if (left_in_range && right_in_range) {
> +                    	ps.println(getKerningPairAsSVG(kst.getKerningPair(i), post));
> +                    }


Should we optimize things a bit, maybe combining the comparison verbosity/additional variable creation into a comment? Something like:

+                	int left = kst.getKerningPair(i).getLeft();
+                	int right = kst.getKerningPair(i).getRight();
+                       // check if left and right are both in range
+                	if ((first <= left && left <= last) && (first <= right && right <= last)) {


Also, the patch is using tabs for indentation. Please also replace with spaces to match Batik's code standards. Other than that, looks good. ;-)


I've adjusted the bug's properties to reflect a patch being available, as well as other minor stuff.


Thomas, what do you thing?

Created attachment 26082 [details]
Updated patch incorporating requested changes

Created attachment 26087 [details]
Fixed glyph ID confusion

Hrm, looks like getLeft()/getRight() return a glyph ID(?) instead of the Unicode codepoint as I thought.

There's probably a better way to map the glyph ID back to the corresponding codepoint but I was unable to find it.

(In reply to comment #3)
> Hrm, looks like getLeft()/getRight() return a glyph ID(?) instead of the
> Unicode codepoint as I thought.

Nice catch. :-)


> There's probably a better way to map the glyph ID back to the corresponding
> codepoint but I was unable to find it.

The code sounds fine, I'm just thinking a bit about performance. I've noticed you using TreeSet which, according to the documentation, isn't bad:

"This implementation provides guaranteed log(n) time cost for the basic operations (add, remove and contains)." [1]

I'm thinking a bit about this in the sense that Unicode fonts may have a very large set of characters, where then performance can be a bit noticeable. I couldn't easily find a better suited replacement. Maybe Thomas/Cameron/DVHolten can shed some light in here?


Apart from that potential improvement (regarding set management), the code looks hood and ready to land IMO. ;-)


[1] http://download.oracle.com/javase/6/docs/api/java/util/TreeSet.html

Created attachment 26103 [details]
Performance improved

(In reply to comment #4)
> > There's probably a better way to map the glyph ID back to the corresponding
> > codepoint but I was unable to find it.
> 
> The code sounds fine, I'm just thinking a bit about performance.

I had "forgotten" about hash set [1], which seems a better fit [1] [2]:

"This class offers constant time performance for the basic operations (add, remove, contains and size)[...]" [1]

I've also reworked the patch so it includes the full source path - otherwise, one needs to already know where the patch target is. ;-)

If there's nothing against, I'll make some tests (I don't have a test environment handy right now) and commit as-is within the next couple of days. :-)

[1] http://download.oracle.com/javase/6/docs/api/java/util/HashSet.html
[2] http://www.jusfortechies.com/core-java/inside-set.html

(In reply to comment #5)
> If there's nothing against, I'll make some tests (I don't have a test
> environment handy right now) and commit as-is within the next couple of days.
> :-)

I've tested with DejaVuSans-Oblique (see bug 48182) and everything (expected behavior and performance) seems fine. :-)

I've made a minor variable cleanup to the latest proposed patch and the whole was committed in revision 1003750.

Thanks, Charles! ;-)