Code review comment for lp://qastaging/~osomon/unity/dash-custom-home-screen

Thanks for the review Gordon!

> I have some concerns over adding extra translation in a non standard
> format, there are good well established formats for .desktop files and
> in source code, but nothing for a custom json file. We should really
> either be extending the desktop files to include an even more generic
> name that is translated such as:
> X-Unity-Generic-Action=Browse the web
>
> or creating new desktop files and installing them somewhere like
> /usr/share/unity/dash/home - using those to store the required
> information. I'd like a compelling reason to be using json over desktop
> files really.

Right, installing desktop files in a custom location to override attributes such as (translated) name, icon, etc. sounds like a good option.
The reason I allowed overriding those attributes directly in the JSON file was to minimize the work of whoever wants to customize the dash, especially in the context of OEM customizations: in most cases that’s only one additional file to ship.

> I'm not a fan of having the old code and this new code sit side-by-side,
> we should stick to one method and lose the other. Which means that we
> should be including a default json file in unity that gets installed to
> somewhere sane like /usr/share/unity/dash/.

That sounds like a very good idea, thanks for the suggestion.

> There is also an issue in that non of this is tested, thus it can't be
> approved until tests exist.

Right. At this point it’s unclear to me whether we actually want this in the trunk, since the (presumably) only target is Oneiric. The point of the exercise of submitting a merge request against the trunk was to have a formal review by someone competent. I’ll clarify this with involved parties.
Obviously tests would be very nice to have, but it may be beyond the scope of the patch at this point.

> small thing, but style wise, we shouldn't be using #define's, (the
> define that exists is from quite old code before we started using this
> style)

Thanks for pointing this out, I fixed it.