Merge lp://qastaging/~mterry/unity-greeter/background-file into lp://qastaging/~robert-ancell/unity-greeter/trunk

Proposed by Michael Terry
Status: Merged
Approved by: Robert Ancell
Approved revision: 239
Merged at revision: 239
Proposed branch: lp://qastaging/~mterry/unity-greeter/background-file
Merge into: lp://qastaging/~robert-ancell/unity-greeter/trunk
Diff against target: 194 lines (+32/-17)
2 files modified
src/unity-greeter.vala (+7/-8)
src/user-list.vala (+25/-9)
To merge this branch: bzr merge lp://qastaging/~mterry/unity-greeter/background-file
Reviewer Review Type Date Requested Status
Robert Ancell Approve
Review via email: mp+84804@code.qastaging.launchpad.net

Description of the change

Use background info from liblightdm-gobject if available.

To post a comment you must log in.
Revision history for this message
Robert Ancell (robert-ancell) wrote :

Looks good, I'd just change:

unowned string background = default_background;
to
var background = default_background;
(we're not gaining anything by making it unowned)

and the spacing should be four spaces here:
11 + if (user.background != null)
12 + background = user.background;

Feel free to commit with those changes

review: Approve
Revision history for this message
Michael Terry (mterry) wrote :

ACK on spacing.

But for the unowned, isn't the difference whether the C code ends up doing a strdup or not? I was trying to avoid an unnecessary malloc.

Revision history for this message
Robert Ancell (robert-ancell) wrote :

Yeah, but you'd be removing one malloc out of thousands that already occur (without significant penalty).

"We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%. A good programmer will not be lulled into complacency by such reasoning, he will be wise to look carefully at the critical code; but only after that code has been identified"[5] — Donald Knuth

Revision history for this message
Michael Terry (mterry) wrote :

Granted, but this was new code, so I did it right. It's not like I'm proposing a branch that changes all existing code. If this were C and I had added an unnecessary g_strdup, you probably would have asked me to drop it.

I'm assuming your unspoken premise is that "var" is more readable than "unowned string" and worth the tradeoff. I can make the change when I push tomorrow.

Revision history for this message
Robert Ancell (robert-ancell) wrote :

Yeah I love var :)

240. By Michael Terry

fix review nits

Revision history for this message
Michael Terry (mterry) wrote :

Hmm, I can't push to unity-greeter trunk. So you'll have to merge this for me. I've updated it to include your review comments.

241. By Michael Terry

handle errors when loading wallpaper by falling back to default_background; keep track of default_background in only one place

Revision history for this message
Michael Terry (mterry) wrote :

I've also updated the branch to handle errors a bit better, by falling back to default_background. I've also moved the default_background variable inside UserList so we only have to keep track of it in one place (instead of unity-greeter.vala as well).

Revision history for this message
Robert Ancell (robert-ancell) wrote :

I've changed the branch for unity-greeter and created a team. You can push to it now.

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.

Subscribers

People subscribed via source and target branches

to all changes: