Merge lp://qastaging/~kamstrup/unity/home-lenses into lp://qastaging/unity

Meh. Sorry for the i18n noise. No idea how that got here. Will try to remove.

Sigh. bzr insists that I don't have any diff with trunk wrt the translations. Fun and games.

Found it. Amended in r1875.

Just a note: all the lens branches are now merged in respective trunks.

Is PlacesHomeView.cpp/h still needed?

1519 ~Lens();

I believe this should be virtual now.

1538 template<class RowAdaptor>
1539 Model<RowAdaptor>::Model()

The missing model.SetGetterFunction(...) could be a very nasty surprise in the future.

One of weird things I noticed is that when using keynav in the home view (specifically the down array) sometimes the "See xx more results" doesn't get highlighted and can't be expanded by pressing enter.

> One of weird things I noticed is that when using keynav in the home view
> (specifically the down array) sometimes the "See xx more results" doesn't get
> highlighted and can't be expanded by pressing enter.

Fixed here https://code.launchpad.net/~andyrock/unity/key-nav

> Is PlacesHomeView.cpp/h still needed

Strictly, no. I didn't delete it because technically the home-lens concept is on a trial run. If design ditches it we'll have to revert to the old view and I didn't want to dig it out of the rev history. Although, I probably should delete it, to have a better looking diffstat :-)

> 1519 ~Lens();
>
> I believe this should be virtual now.

Right. I wonder why the compiler didn't complain. It usually does in these cases. Fixed.

> 1538 template<class RowAdaptor>
> 1539 Model<RowAdaptor>::Model()
>
> The missing model.SetGetterFunction(...) could be a very nasty surprise in the
> future.

Ah, good catch. I created a new Init() method that gets called from the constructors so they can share the setup code. (my favorite hate-pattern. why oh why can't we chain constructors in C++?!?!?!11). Anyway; fixed.

WTF. The diffs seems to be showing that I removed po/unity.pot which I swear I didn't! Let me look into it.

Ok, the diff should be clean in r1881. Please re-review

One thing that's still missing is resetting the search entry when dash is hidden (or not-resetting the search sent to lenses as https://bugs.launchpad.net/ayatana-design/+bug/914759 requires).

Either would be fine, but right now the search entry isn't reset, while the search is...

Implemented search state retention as requested by design. Please re-review.

John acked the design review on IRC

Sorry, but Esc now no longer clears the search entry... I don't think that's desired.

> Sorry, but Esc now no longer clears the search entry... I don't think that's
> desired.

1866 - else
1867 - search_bar_->search_string = "";

> > Sorry, but Esc now no longer clears the search entry... I don't think that's
> > desired.
>
> 1866 - else
> 1867 - search_bar_->search_string = "";

I am not sure if you mean to back Michal up, or refute his claim Andrea? :-)

In any case, I think I have the right behavior here... When clicking Esc I see:

a) if there is text in the entry the text is cleared, and the dash shows results for the empty search
b) if there is no search, the dash is hidden

As a Corollary, c) hitting Esc twice clears and hides the dash.

Unfortunately, that's not the behaviour I see (particularly a) doesn't work). Afaict the code Andrea pasted is what handled the "esc and search entry not empty" case, and you removed it...

Hmmm, I can see you are correct looking at the code... I wonder how it could work on my box. I must have been running some almost, but not entirely, identical to what I pushed. Nonetheless; pushed a fix.

Yep, it's fine now. Approved from me, but I suppose more people want to go through this...

237 (and other places): Instead of handling the gchar* yourself, use glib::String from UnityCore/GLibWrapper.h, it autofree's it when it's popped from the stack, so you don't need to worry about g_free in multiple places.

330: Evil, evil kamstrup.

Apart from that, excellent work :)

> 237 (and other places): Instead of handling the gchar* yourself, use
> glib::String from UnityCore/GLibWrapper.h, it autofree's it when it's popped
> from the stack, so you don't need to worry about g_free in multiple places.

Ok, now using glib::String. It required that I add automatic std::string conversion to glib::String in order not to complicate the code - I was wondering if there was a specific reason it wasn't in glib::String already. Seems like a strange omission?

Neil ACKed this as well, so approving...