Merge lp://qastaging/~osomon/unity/dash-custom-home-screen into lp://qastaging/unity

I am not sure how this relates to bug #885738...

Yeah, I know this is in apparent contradiction with the description of bug #885738 and with what was discussed at UDS.
But OEM needs this functionality for a project running Oneiric, and they want the patch reviewed and approved by Unity core developers, hence this merge request. Whether it should actually be merged into trunk is beyond me, but a review and functional tests would be nice. Thanks!

We need an SRU for oneiric that includes this patch.

As per bug #885738, this won't be a problem for precise and beyond.

On 23/11/11 15:31, Olivier Tilloy wrote:
> Olivier Tilloy has proposed merging lp:~osomon/unity/dash-custom-home-screen into lp:unity.
>
> Requested reviews:
> Unity Team (unity-team): code functional
> Related bugs:
> Bug #785840 in unity: "Home dash icons need to be customizable"
> https://bugs.launchpad.net/unity/+bug/785840
>
> For more details, see:
> https://code.launchpad.net/~osomon/unity/dash-custom-home-screen/+merge/83175
>
> So it seems that the patch I wrote some time ago to make the home screen of the dash customizable just like in unity-2d is being considered for inclusion in Oneiric…
>
> I refreshed it to apply against the current trunk, functional testing and comments are welcome. Please bear with me as I’m not really familiar with unity’s code base, coding style and conventions.

So I have a few questions and a few notes;

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.

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

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

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)

namespace
{
  std::string custom_shortcuts_file =
"/unity/HomeShortcuts/Customized.json";
}

is a preferred style.

Apart from that the code looks good

 review needs-info

--
Gordon Allott
Canonical Ltd.
27 Floor, Millbank Tower
London SW1P 4QP
www.canonical.com

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.

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

So if we aren't targeting trunk you need to re-submit this against lp:unity/4.0 not lp:unity - small thing, but the auto lander will put this in trunk if we leave it against lp:unity

Rejecting as this functionality should target oneiric (4.0), not trunk.

Superseded by https://code.launchpad.net/~osomon/unity/4.0-dash-custom-home-screen/+merge/85852.