Code review comment for lp://qastaging/~gordallott/unity/fix-dash-icon-aspect-ratio

Revision history for this message
Andrea Cimitan (cimi) wrote :

Few notes:
- I think "cairo_scale (cr, 1.0f, 1.0f);" does nothing, unless you added it to highlight you're not scaling the context, but in this case it might be better to add a simple comment and removes redundant calls.
- if you use "CAIRO_OPERATOR_CLEAR" then the call "cairo_set_source_rgba(cr, 1.0, 1.0, 1.0, 0.0);" is useless, you can remove it (or use it with "CAIRO_OPERATOR_SOURCE" instead).
- you calculate translation with "(width - (pixbuf_width * scale)) * 0.5, (height - (pixbuf_height * scale)) * 0.5)", but this can handle in float numbers, while I think we want integers to avoid misalignment to the cairo grid: simply cast them to be integers "(int)((width - (pixbuf_width * scale)) * 0.5), (int)((height - (pixbuf_height * scale)) * 0.5))", and then check again if it's correctly aligned.

A part from the quick overview on the cairo code, I didn't test the real math to see if works correctly, but I trust gord as it seems pretty simple math ;)

Good practice (not to apply to this code, as I think context are deleted after the functions close):
When modifying either the cairo matrix (calls to "cairo_scale" or "cairo_translate") or operators I'd use "cairo_save(cr);" then "cairo_restore(cr);" afterwords, because if the cairo context lives after your drawing and other drawing methods use it, they might continue from the previous state you're modified, leading to weird behaviors.

review: Needs Fixing

« Back to merge proposal