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

> 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
>
The convention is indeed not in the CODING file though the prefix "on" is in majority in the code base, not only in C++ as you shown already but also in QML:

$ grep -r --include="*.qml" \ on[A-Z] . | wc -l
168

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

That's the best option indeed.

>
>
> > 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.

I agree. I misread your comment in the code and completely ignored that important case. The timer is therefore not optional.