Code review comment for lp:~unity-team/unity/unity.coverflow

Revision history for this message
Tim Penhey (thumper) wrote :

On 15/02/12 03:44, David Barth wrote:
> Review: Disapprove
>
> Jason, I appreciate this is a nice feature and was done on your spare time. Nonetheless, this cannot land, the reasons being:
>
> - there are no tests; no exceptions...

This is the prime blocker at this stage.

> - this is a feature for which performance *is* a quality requirement: the dash can't become slow because that code is not performant; the same way there were performance issues last cycle with other views, I want facts to prove that this widget / view will perform well: how does it work with 100 entries? what's the startup time impact? is icon loading performed on the fly, or in one batch at the start?
> See https://wiki.canonical.com/ProductStrategyTeam/Checklist/WhatNeedsTesting for a reference.

This isn't an issue as it is using all the same loading code as the grid.

>
> - the list of required design changes is quite long: which means that the costs of accepting that feature is significant: impact on design, on platform, on unity-2d: i'd rather not add more to the feature development overhead and instead focus on getting higher quality for the LTS

Obviously there are some that are needed prior to landing, and others
that are "ideal". It is up to design to work out what is what there.

> - the feature was unplanned, not tracked anywhere: remember that we're targeting an LTS with this version of Unity

I don't think that this is a valid reason at all.

A key part of open source is being able to scratch your own itch.
Obviously for a group of talented hackers, coverflow was certainly an
itch that they wanted to scratch.

Since this was done during their own time, and design did reviews in
their own time, as long as the work meets the quality needs, I see
absolutely no reason why the feature couldn't land just because it
wasn't on the "planned deliverables for the shell team".

So, things blocking this landing are:
  - sufficient testing
  - design sign-off

Ideally we'd like to land this in a feature complete mode, not one where
we need to do a lot of extra work. I'm very excited to see this work
and I'd like to see it land, but I also want to make sure the quality of
what we ship stays high.

Thanks,
Tim

« Back to merge proposal