Merge lp://qastaging/~gordallott/unity/fix-dash-icon-aspect-ratio into lp://qastaging/unity

Proposed by Gord Allott
Status: Merged
Merged at revision: 1640
Proposed branch: lp://qastaging/~gordallott/unity/fix-dash-icon-aspect-ratio
Merge into: lp://qastaging/unity
Diff against target: 96 lines (+55/-8)
1 file modified
plugins/unityshell/src/ResultRendererTile.cpp (+55/-8)
To merge this branch: bzr merge lp://qastaging/~gordallott/unity/fix-dash-icon-aspect-ratio
Reviewer Review Type Date Requested Status
Andrea Cimitan (community) Approve
Review via email: mp+76977@code.qastaging.launchpad.net

Description of the change

fixes the aspect ratio of the icons in the dash being a bit off. for +1 i want to change this again so that the icons are larger when the aspect ratio is corrected, but it needs a bit more code shakeup for that. so in the interest of fixing bugs and not making new ones, kept it simple for now :)

To post a comment you must log in.
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
Revision history for this message
Andrea Cimitan (cimi) wrote :

looks good, thx gord!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.