<div dir="ltr">On Thu, Sep 14, 2017 at 12:37 PM, Behdad Esfahbod <span dir="ltr"><<a href="mailto:behdad.esfahbod@gmail.com" target="_blank">behdad.esfahbod@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><div><div><div><div>Thanks Matthias.  I took a look at the patch.  Looks good!  Minor comments:<br><br></div>- Moving the format towards CSS by default (no comma, no equal sign; just spaces!) while accepting both is what I do in HarfBuzz. Maybe advertise the same here,  UPDATE: I was wrong.  CSS uses comma as well.  It's the equal sign that they don't use, they use space.  We should accept both.<br></div></div></div></div></div></div></blockquote><div><br></div><div>I've made the parser a bit more flexible. It now accepts variations like<br><br></div><div>wdth=200,wght=300<br></div><div>wdth 200, wght=300<br></div><div>wdth 200 , wght 300<br><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><div><div>- Maybe make the variations member be "const char *" instead of "char *".<br></div></div></div></div></div></blockquote><div><br></div><div>But I'm strdup'ing and freeing it. The const will get in the way there.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><div><div></div>- In +cairo_font_options_set_<wbr>variations(), typically in these kinds of functions I like copying then freeing the old value, such that the input can be the return value of get_variations() on the same object.<br></div></div></div></div></blockquote><div><br></div><div>Done<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><div>- (((uint8_t)p[0])<<24)|(((<wbr>uint8_t)p[1])<<16)|(((uint8_t)<wbr>p[2])<<8)|((uint8_t)p[3]); </div><div>FreeType has a macro for that: FT_MAKE_TAG()</div></div></div></div></blockquote><div><br></div><div>Thanks for the reminder. Done.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">(Generally I find it easier pushing a tree on github and reviewing there; also because I can just fix stuff and push my own tree.)<br></div><br></blockquote><div><br></div><div>You can find the new version here:<br><br><a href="https://github.com/matthiasclasen/cairo/commits/wip/matthiasc/font-variations">https://github.com/matthiasclasen/cairo/commits/wip/matthiasc/font-variations</a> <br></div></div></div></div>