Code review comment for lp:~osomon/unity-2d/webfavs

Revision history for this message
Olivier Tilloy (osomon) wrote :

I agree on most of your points, I’ll update my code accordingly. Please see my comments below.

> signal handlers (slot methods) should not be prefixed 'slot';
> 'on' is the convention for prefixing signal handlers though
> it's much better to name the method by what it actually does
> when possible.

This convention is not part of the coding guidelines. A quick analysis of the header files shows that the two prefixes currently happily coexist in the trunk:

$ grep -r --include="*.h" "void\ on" * | wc -l
32
$ grep -r --include="*.h" "void\ slot" * | wc -l
19

I agree that it would be better to name the methods according to what they actually do, so I’ll do that.

> OPTIONALLY remember the desktop files of the applications that
> were favorited by the user and monitor the parent folder of the
> desktop file for their creation and re-add them to the launcher
> as favorites them when appropriate (that will also avoid implementing
> the grace period timer for vi & co at the expense of having a tiny
> visual glitch where the icon disappears and then reappears
> immediately in the launcher)

This is by no means a "tiny" visual glitch, it’s rather a major issue that will make the launcher look broken. The grace period is fine, it just delays the removal by one second, which I think is perfectly acceptable. Note that it’s not just "vi & co", it’s also the more frequent use case of an application being upgraded: the new desktop file is copied over the old one, effectively equivalent to the file being removed and then written again.
On top of that, monitoring several directories that may contain several hundreds of desktop files has a cost. And if an application is really removed for good, keeping on monitoring the parent folder of its desktop file is useless.

« Back to merge proposal