Introduce Font and FontVariationDescription #105

Merged
merged 21 commits into from Apr 8, 2013

Conversation

Projects
None yet
2 participants
@bramstein
Member

bramstein commented Mar 8, 2013

Ready to deploy. This pull request is an internal rewrite of how font (family) names and font variation descriptions are passed around. Currently this is done using a pair of font family names and an object containing font family names and descriptions. This pull request introduces a Font class that contains FontVariationDescription instances. This will reduce the number of parameters passed around, makes type annotations easier to read and creates a central place to manipulate fonts and font variations.

src/core/fontvariationdescription.js
- }
- }
- }
+ FontVariationDescription.prototype.parseFvd = function (fvd) {

This comment has been minimized.

@seanami

seanami Mar 11, 2013

Contributor

Shouldn't this and parseCss be class methods that you call like FontVariationDescription.parseCSS(...) and that return a new FontVariationDescription instance? I feel like instances of this class should probably be immutable once they're instantiated, to clarify that they're just a representation of a particular weight and style with convenience methods attached.

@seanami

seanami Mar 11, 2013

Contributor

Shouldn't this and parseCss be class methods that you call like FontVariationDescription.parseCSS(...) and that return a new FontVariationDescription instance? I feel like instances of this class should probably be immutable once they're instantiated, to clarify that they're just a representation of a particular weight and style with convenience methods attached.

This comment has been minimized.

@bramstein

bramstein Mar 12, 2013

Member

Aren't they immutable? Both those methods are marked as private, so they can't be used to update existing variations. Or?

@bramstein

bramstein Mar 12, 2013

Member

Aren't they immutable? Both those methods are marked as private, so they can't be used to update existing variations. Or?

This comment has been minimized.

@seanami

seanami Mar 12, 2013

Contributor

Ah right, I guess that would at least make it externally immutable. And I see that you use these to determine whether a string is CSS or an FVD when instantiating one.

Personally, I feel like it would be clearer to have two class methods for parseFvd and parseCss that return new instances. The constructor could just take standardized inputs that were already parsed (like 400, "normal"). That way, in other code, you'd consistently call the one that you meant to call without having to use the "a string of length two must be an FVD" trick. That would work unless there's a time when we don't know whether a given piece of input is an FVD or CSS. Is that a case that we need to handle, where there's user input or another string that could be either an FVD or CSS?

@seanami

seanami Mar 12, 2013

Contributor

Ah right, I guess that would at least make it externally immutable. And I see that you use these to determine whether a string is CSS or an FVD when instantiating one.

Personally, I feel like it would be clearer to have two class methods for parseFvd and parseCss that return new instances. The constructor could just take standardized inputs that were already parsed (like 400, "normal"). That way, in other code, you'd consistently call the one that you meant to call without having to use the "a string of length two must be an FVD" trick. That would work unless there's a time when we don't know whether a given piece of input is an FVD or CSS. Is that a case that we need to handle, where there's user input or another string that could be either an FVD or CSS?

src/core/fontfamily.js
+ * @param {string} name The font family name
+ * @param {webfont.FontVariationDescription=} opt_variation A font variation..
+ */
+webfont.FontFamily = function (name, opt_variation) {

This comment has been minimized.

@seanami

seanami Mar 11, 2013

Contributor

This seems incorrectly named to me. A family should be something that can contain more than one weight and style, but this is something that can only contain a single weight/style. This seems more like an individual font than a family...

@seanami

seanami Mar 11, 2013

Contributor

This seems incorrectly named to me. A family should be something that can contain more than one weight and style, but this is something that can only contain a single weight/style. This seems more like an individual font than a family...

This comment has been minimized.

@bramstein

bramstein Mar 12, 2013

Member

Good point. It used to be a FontFamily, but not it is just a Font.

@bramstein

bramstein Mar 12, 2013

Member

Good point. It used to be a FontFamily, but not it is just a Font.

src/core/font.js
- * @param {Window} mainWindow The main application window containing
- * webfontloader.js.
- * @param {webfont.FontModuleLoader} fontModuleLoader A loader instance to use.
- * @param {webfont.UserAgent} userAgent The detected user agent to load for.
* @constructor

This comment has been minimized.

@seanami

seanami Mar 22, 2013

Contributor

Can you add a comment about what this class is for? I guess it's pretty obvious from the code, but "font" is such an overloaded term that we should probably describe exactly what this represents.

@seanami

seanami Mar 22, 2013

Contributor

Can you add a comment about what this class is for? I guess it's pretty obvious from the code, but "font" is such an overloaded term that we should probably describe exactly what this represents.

src/core/eventdispatcher.js
if (this.callbacks_[event]) {
- this.callbacks_[event](opt_arg1, opt_arg2);
+ this.callbacks_[event](opt_arg1);

This comment has been minimized.

@seanami

seanami Mar 22, 2013

Contributor

We can't change the arguments that we pass to external callbacks. These callbacks are defined by end-users that are using webfont.js (or derivatives like Typekit's kit JS). We should split this back out at this point into two strings. One reason is that we don't want (and can't expect) all users of this JS to change their callback formats. Another reason is that, at that point, we'd have to have all of the Font class methods externalized (or at least the public ones) since it'd be interacting with uncompiled code.

@seanami

seanami Mar 22, 2013

Contributor

We can't change the arguments that we pass to external callbacks. These callbacks are defined by end-users that are using webfont.js (or derivatives like Typekit's kit JS). We should split this back out at this point into two strings. One reason is that we don't want (and can't expect) all users of this JS to change their callback formats. Another reason is that, at that point, we'd have to have all of the Font class methods externalized (or at least the public ones) since it'd be interacting with uncompiled code.

This comment has been minimized.

@bramstein

bramstein Mar 23, 2013

Member

Good catch! I was trying to get it in-line with the fontloader interface, but we shouldn't change this important API. I'll split it out again when the events are sent to the user and keep the object internally.

@bramstein

bramstein Mar 23, 2013

Member

Good catch! I was trying to get it in-line with the fontloader interface, but we shouldn't change this important API. I'll split it out again when the events are sent to the user and keep the object internally.

src/fontdeck/fontdeck_script.js
+ } else if (/[1-9]00/.test(weight[1])) {
+ weight = parseInt(m[1].substr(0, 1), 10);
+ }
+ }

This comment has been minimized.

@seanami

seanami Mar 22, 2013

Contributor

Now that you've pulled this code out of the old FontVariationDescription class, should we still have some unit tests that cover the behavior of it? It seems dangerous to add it here without adding related tests for FontdeckScript. But on the other hand, the FontdeckScript tests don't really seem like the right place to unit test this type of code in a detailed way. Maybe this should be encapsulated as a class-method on webfont.Font?

@seanami

seanami Mar 22, 2013

Contributor

Now that you've pulled this code out of the old FontVariationDescription class, should we still have some unit tests that cover the behavior of it? It seems dangerous to add it here without adding related tests for FontdeckScript. But on the other hand, the FontdeckScript tests don't really seem like the right place to unit test this type of code in a detailed way. Maybe this should be encapsulated as a class-method on webfont.Font?

This comment has been minimized.

@bramstein

bramstein Mar 23, 2013

Member

We should definitely have tests for this. I forgot to add them again. I'm still not sure where the best place is to put this functionality. The Font interface is the most likely candidate although this is the only place it is used.

@bramstein

bramstein Mar 23, 2013

Member

We should definitely have tests for this. I forgot to add them again. I'm still not sure where the best place is to put this functionality. The Font interface is the most likely candidate although this is the only place it is used.

spec/core/eventdispatcher_spec.js
});
it('should call the correct callback', function () {
- expect(callbacks.fontloading).toHaveBeenCalledWith('My Family', 'n4');
+ expect(callbacks.fontloading).toHaveBeenCalledWith(font);

This comment has been minimized.

@seanami

seanami Mar 22, 2013

Contributor

Like I said in the class, we can't change the parameters that user-supplied callbacks are called with. (Or at least, we shouldn't right now.)

@seanami

seanami Mar 22, 2013

Contributor

Like I said in the class, we can't change the parameters that user-supplied callbacks are called with. (Or at least, we shouldn't right now.)

@@ -34,7 +35,7 @@ describe('MonotypeScript', function () {
monotype.load(load);
global[MonotypeScript.HOOK + configuration.projectId] = function () {
- return ['aachen bold', 'kid print regular'];
+ return [{fontfamily: 'aachen bold'}, {fontfamily: 'kid print regular'}];

This comment has been minimized.

@seanami

seanami Mar 22, 2013

Contributor

Is this changing something internal to the webfontloader and the modules, or is it changing a dependency in Monotype's JS code? This stuff is confusing to trace through, with the communication between the module and the non-webfontloader code, but I just wanted to make sure that none of these changes will require any kind of associated changes in non-webfontloader code.

@seanami

seanami Mar 22, 2013

Contributor

Is this changing something internal to the webfontloader and the modules, or is it changing a dependency in Monotype's JS code? This stuff is confusing to trace through, with the communication between the module and the non-webfontloader code, but I just wanted to make sure that none of these changes will require any kind of associated changes in non-webfontloader code.

This comment has been minimized.

@bramstein

bramstein Mar 23, 2013

Member

This (and the change below) are because the original test was broken. It did not test that fonts were parsed and loaded correctly. I noticed this while I was making the font/variation changes and though I might as well fix it. I verified these changes with the Monotype API.

@bramstein

bramstein Mar 23, 2013

Member

This (and the change below) are because the original test was broken. It did not test that fonts were parsed and loaded correctly. I noticed this while I was making the font/variation changes and though I might as well fix it. I verified these changes with the Monotype API.

@@ -44,6 +45,6 @@ describe('MonotypeScript', function () {
expect(support).toHaveBeenCalled();
expect(fakeDomHelper.createElement).toHaveBeenCalledWith('script');
expect(script.src).toEqual('http://fast.fonts.com/jsapidev/01e2ff27-25bf-4801-a23e-73d328e6c7cc.js');
- expect(load).toHaveBeenCalledWith([undefined, undefined], {});
+ expect(load).toHaveBeenCalledWith([new Font('aachen bold'), new Font('kid print regular')]);

This comment has been minimized.

@seanami

seanami Mar 22, 2013

Contributor

Why did this change? Was this just broken before? undefined deoesn't seem like the same kind of return value at all...

@seanami

seanami Mar 22, 2013

Contributor

Why did this change? Was this just broken before? undefined deoesn't seem like the same kind of return value at all...

@seanami

This comment has been minimized.

Show comment
Hide comment
@seanami

seanami Mar 25, 2013

Contributor

Nice fixes. I think this looks good now.

Contributor

seanami commented Mar 25, 2013

Nice fixes. I think this looks good now.

@seanami

This comment has been minimized.

Show comment
Hide comment
@seanami

seanami Apr 2, 2013

Contributor

@bramstein Ah crap, I just realized something when looking at the Typekit side of these changes. I believe this branch changes the arguments that are passed back and forth between third-party code (like Typekit's own kit JS) and the core webfont.js library (in the init method that's set up in order to communicate things).

This doesn't seem like an OK change to make, as it will cause older third-party JS (like older Typekit kits) not to work with the newer versions of webfont.js (and vice-versa), since the different versions will be expecting different argument numbers and types.

Also, passing anything back and forth between third-party codebases (like Typekit kit JS) and webfont.js other than simple JS types (like boolean, number, string, array, object used as key/value hash) isn't a good idea. Passing instances of classes back and forth gets complicated, since the method names of that class that are shared between them must be ensured to match and never change (through exports) so that the code can interoperate. Basic JS types are much more future proof, and we also don't have to worry about compilation renaming throwing a wrench in the works.

I think you should make a change so that the arguments that are passed back and forth between the non-webfontloader code and the webfontloader module code doesn't change at all. That will ensure that we don't have backward-incompatibility problems.

Contributor

seanami commented Apr 2, 2013

@bramstein Ah crap, I just realized something when looking at the Typekit side of these changes. I believe this branch changes the arguments that are passed back and forth between third-party code (like Typekit's own kit JS) and the core webfont.js library (in the init method that's set up in order to communicate things).

This doesn't seem like an OK change to make, as it will cause older third-party JS (like older Typekit kits) not to work with the newer versions of webfont.js (and vice-versa), since the different versions will be expecting different argument numbers and types.

Also, passing anything back and forth between third-party codebases (like Typekit kit JS) and webfont.js other than simple JS types (like boolean, number, string, array, object used as key/value hash) isn't a good idea. Passing instances of classes back and forth gets complicated, since the method names of that class that are shared between them must be ensured to match and never change (through exports) so that the code can interoperate. Basic JS types are much more future proof, and we also don't have to worry about compilation renaming throwing a wrench in the works.

I think you should make a change so that the arguments that are passed back and forth between the non-webfontloader code and the webfontloader module code doesn't change at all. That will ensure that we don't have backward-incompatibility problems.

spec/google/fontapiparser_spec.js
@@ -12,54 +13,20 @@ describe('FontApiParser', function () {
it('should parse families correctly', function () {
var fontFamilies = parser.getFontFamilies();

This comment has been minimized.

@seanami

seanami Apr 3, 2013

Contributor

Some of these methods could probably stand some renaming now, since this method returns fonts and not families.

@seanami

seanami Apr 3, 2013

Contributor

Some of these methods could probably stand some renaming now, since this method returns fonts and not families.

src/fontdeck/fontdeck_script.js
- // Add the FVDs
- self.fontFamilies_.push(font['name']);
- self.fontVariations_[font['name']] = [self.fvd_.compact("font-weight:" + font['weight'] + ";font-style:" + font['style'])];
+ self.fontFamilies_.push(new Font(font['name'], Font.parseCssVariation('font-weight:' + font['weight'] + ';font-style:' + font['style'])));

This comment has been minimized.

@seanami

seanami Apr 3, 2013

Contributor

Same naming note here. This is really an array of fonts now, not families. We should update the names so that it's consistent and easy to read the code and deduce what something is.

@seanami

seanami Apr 3, 2013

Contributor

Same naming note here. This is really an array of fonts now, not families. We should update the names so that it's consistent and easy to read the code and deduce what something is.

src/google/googlefontapi.js
@@ -59,8 +59,7 @@ goog.scope(function () {
domHelper.insertInto('head', domHelper.createCssLink(
fontApiUrlBuilder.build()));
- onReady(fontApiParser.getFontFamilies(), fontApiParser.getVariations(),
- fontApiParser.getFontTestStrings());
+ onReady(fontApiParser.getFontFamilies(), fontApiParser.getFontTestStrings());

This comment has been minimized.

@seanami

seanami Apr 3, 2013

Contributor

I noticed that the Google module passes test strings to the onReady method as well, but it looks like the other side of the onready method at

module.load(function (fontFamilies) {
doesn't support receiving them. Is this just an oversight? A bug from longer ago? Am I missing something?

@seanami

seanami Apr 3, 2013

Contributor

I noticed that the Google module passes test strings to the onReady method as well, but it looks like the other side of the onready method at

module.load(function (fontFamilies) {
doesn't support receiving them. Is this just an oversight? A bug from longer ago? Am I missing something?

This comment has been minimized.

@bramstein

bramstein Apr 4, 2013

Member

I noticed this as well. It has been in there for a long time. My plan was to give that whole code an overhaul as well once we expose custom test strings in the API.

@bramstein

bramstein Apr 4, 2013

Member

I noticed this as well. It has been in there for a long time. My plan was to give that whole code an overhaul as well once we expose custom test strings in the API.

src/monotype/monotype_script.js
@@ -17,7 +19,6 @@ webfont.MonotypeScript = function (userAgent, domHelper, configuration) {
this.domHelper_ = domHelper;
this.configuration_ = configuration;
this.fontFamilies_ = [];

This comment has been minimized.

@seanami

seanami Apr 3, 2013

Contributor

Same naming note here: these are just fonts now, not font families.

@seanami

seanami Apr 3, 2013

Contributor

Same naming note here: these are just fonts now, not font families.

+ * @param {Array.<string>} providedFamilies
+ * @return {Array.<webfont.Font>}
+ */
+ AscenderScript.prototype.parseFamiliesAndVariations = function (providedFamilies) {

This comment has been minimized.

@seanami

seanami Apr 3, 2013

Contributor

The tests for the ascender module aren't very complete, and I'm not confident that they check that we're actually watching the same things as a result of these changes. Should we add a test to ensure that the same things are coming out of multiple families being parsed, and that the onReady still receives the correct response for a given configuration?

@seanami

seanami Apr 3, 2013

Contributor

The tests for the ascender module aren't very complete, and I'm not confident that they check that we're actually watching the same things as a result of these changes. Should we add a test to ensure that the same things are coming out of multiple families being parsed, and that the onReady still receives the correct response for a given configuration?

This comment has been minimized.

@bramstein

bramstein Apr 4, 2013

Member

I added three new tests, all are passing.

@bramstein

bramstein Apr 4, 2013

Member

I added three new tests, all are passing.

@seanami

This comment has been minimized.

Show comment
Hide comment
@seanami

seanami Apr 3, 2013

Contributor

@bramstein Sorry for the previous false alarm. After looking at this more closely, I realize that it's only changing what webfont.js is passing around internally between the modules and the core code, but it doesn't change any data that 3rd-party JS would be passing back and forth with webfont.js.

I added some notes about naming, one potential issue with the Google module, and a lack of tests for some of the functionality of the Ascender module. Otherwise, this looks good.

For the naming, I just think we should switch to using "fonts" wherever we have an array of font objects (in all of the module code and the core code) instead of continuing to call the variables "fontFamilies" or similar when the items within don't actually represent families. It will just make the code easier to read in the future.

Contributor

seanami commented Apr 3, 2013

@bramstein Sorry for the previous false alarm. After looking at this more closely, I realize that it's only changing what webfont.js is passing around internally between the modules and the core code, but it doesn't change any data that 3rd-party JS would be passing back and forth with webfont.js.

I added some notes about naming, one potential issue with the Google module, and a lack of tests for some of the functionality of the Ascender module. Otherwise, this looks good.

For the naming, I just think we should switch to using "fonts" wherever we have an array of font objects (in all of the module code and the core code) instead of continuing to call the variables "fontFamilies" or similar when the items within don't actually represent families. It will just make the code easier to read in the future.

@bramstein

This comment has been minimized.

Show comment
Hide comment
@bramstein

bramstein Apr 4, 2013

Member

I adressed the naming issues and added some new tests to the ascender module. I prefer to leave the Google module bug for another pull request.

Member

bramstein commented Apr 4, 2013

I adressed the naming issues and added some new tests to the ascender module. I prefer to leave the Google module bug for another pull request.

src/ascender/ascender_script.js
}
- return { families:families, variations:variations };
+ return families;

This comment has been minimized.

@seanami

seanami Apr 4, 2013

Contributor

Nitpick: Here's one more spot where we could switch families to fonts for semantic accuracy.

@seanami

seanami Apr 4, 2013

Contributor

Nitpick: Here's one more spot where we could switch families to fonts for semantic accuracy.

src/custom/customcss.js
@@ -33,21 +36,22 @@ goog.scope(function () {
}
var families = [];

This comment has been minimized.

@seanami

seanami Apr 4, 2013

Contributor

Nitpick: Here's another place to rename families to fonts.

@seanami

seanami Apr 4, 2013

Contributor

Nitpick: Here's another place to rename families to fonts.

src/typekit/typekit_script.js
} else {
support(true);
}
};
TypekitScript.prototype.load = function(onReady) {
- onReady(this.fontFamilies_, this.fontVariations_);
+ onReady(this.fontFamilies_);

This comment has been minimized.

@seanami

seanami Apr 4, 2013

Contributor

Nitpick: One last place to change families to fonts.

@seanami

seanami Apr 4, 2013

Contributor

Nitpick: One last place to change families to fonts.

@seanami

This comment has been minimized.

Show comment
Hide comment
@seanami

seanami Apr 4, 2013

Contributor

Other than 3 more naming nitpicks, this looks good! Hooray for reorganization and code improvement. :)

🚢

Contributor

seanami commented Apr 4, 2013

Other than 3 more naming nitpicks, this looks good! Hooray for reorganization and code improvement. :)

🚢

@bramstein

This comment has been minimized.

Show comment
Hide comment
@bramstein

bramstein Apr 5, 2013

Member

Thanks, I fixed those. Should have grepped myself, sorry.

Member

bramstein commented Apr 5, 2013

Thanks, I fixed those. Should have grepped myself, sorry.

bramstein added a commit that referenced this pull request Apr 8, 2013

Merge pull request #105 from typekit/bs-fontfamily
Introduce Font and FontVariationDescription

@bramstein bramstein merged commit ddc3dcc into master Apr 8, 2013

@bramstein bramstein deleted the bs-fontfamily branch Apr 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment