search

graphicore / ufoJS

Javascript API for the Unified Font Object
lib.ufojs.org
GNU General Public License v3.0
52 stars 10 forks source link

issues/144 move readGlyph to an object model so that the error callback is always available. #17

Closed monkeyiq closed 9 years ago

monkeyiq commented 9 years ago

The lines at the end of readGlyph are not exactly what was mentioned on IRC. I had trouble running those lines, whereas the code here runs ok. Suggestions and feedback welcome.

https://github.com/metapolator/metapolator/issues/144

monkeyiq commented 9 years ago

I'll take a look at npm test locally...

monkeyiq commented 9 years ago

@graphicore if the code itself looks OK then I'll do another round of testing and see what these changes broke and fix that up for a merge.

monkeyiq commented 9 years ago

npm test fixed

monkeyiq commented 9 years ago

Note that I haven't been testing these updates against the MP code. Once the ufoJS PR looks like it could be merged then I'll turn to making sure it actually still performs properly.

monkeyiq commented 9 years ago

I assume that the 'this' is still overridable for readGlyph _p._getAttribute(). I had a little bug in there in the initial code for this PR.

graphicore commented 9 years ago

yeah. MP will have to follow these updates

graphicore commented 9 years ago

about _p._getAttribute: there are some methods in ReadGlyph, that are rather helper function than business logic. _getAttribute expects a DOM element as its this value. No particular this is yet expected for _isDOMElement, _toNumber, _getArrayInterface, _listOfChildren, _attributesDict. It's hard to say whether these methods should be part of ReadGlyph or not, if we want to inherit from ReadGlyph one day, it may be useful to still have acess to these methods. Otherwise, I'd say they don't need to be part of if it at the moment, but I think this is a not so important detail. If it would be easier to read the code in one way or the other, feel free to edit that.

In JavaScript you can use the methods call, apply and bind to change the this value of a function call. Also, every method always has a this, it's the global object if no other object applies. The acessors . and [ ] can be seen as syntactic sugar: obj.mymethod = myfunction; obj.mymethod('a') equals myfunction.call(object, 'a'). Although, square brackets allow any string as a name.

monkeyiq commented 9 years ago

Regarding the binding of 'this' see the Array.map at https://github.com/monkeyiq/ufoJS/blob/02b66ee5f9d30851b56e13847968849c0fb66d71/lib/ufoLib/glifLib/readGlyph.js#L701

Previously the _getArrayInterface() was a top level function, so I assume 'this' would be some top level object. But now getArrayInterface() is using 'this' which will be a readGlyph object instead?

graphicore commented 9 years ago

The seccond argument of Array.prototype.map can be used to set the this value of the method beeing mapped.

this._getArrayInterface doesn't use this at all, but it returns a new new method that uses this.

so, in this case, the returned method is called with component as this.

that this (component) is used to call '_getAttribute'

monkeyiq commented 9 years ago

Cool, thanks for the clarification. So it seems all is well with _getAttribute() et al with the new object based setup.

monkeyiq commented 9 years ago

@graphicore so this looks ok to merge?

graphicore commented 9 years ago

I think that's all then. Could you also squash the commits into one please?

monkeyiq commented 9 years ago

The commits have been squashed into a single commit in the new pull request at https://github.com/graphicore/ufoJS/pull/19