Update app list search box menu when Users changes

Update app list search box menu when Users changes

BUG=320409

[stevenjb, 2013-11-21 19:43:34]

There are two patchsets here. The first one is a tiny change that should fix the
Windows bug.

The second, I think, is more robust, but adds additional complexity to the code.

Let me know what you think.

[xiyuan, 2013-11-21 20:50:20]

My preference is to move AppListMenu and user etc back into chrome side and
expose them via a MenuModel from AppListViewDelegate.

If that is not going to happen, I am okay with the proposal here in general.
Just maybe wrap Users into a proper class and having the observer interface on
that class instead.

[koz (OOO until 15th September), 2013-11-21 23:39:35]

On 2013/11/21 20:50:20, xiyuan wrote:
> My preference is to move AppListMenu and user etc back into chrome side and
> expose them via a MenuModel from AppListViewDelegate.
> 
> If that is not going to happen, I am okay with the proposal here in general.
> Just maybe wrap Users into a proper class and having the observer interface on
> that class instead.

I agree with Xiyuan, Users should be its own class and have its own observer
interface.

[tfarina, 2013-11-21 23:41:42]

On Thu, Nov 21, 2013 at 9:39 PM,  <koz@chromium.org> wrote:
> I agree with Xiyuan, Users should be its own class and have its own observer
> interface.
>
And the observer in its own header file (please ;)).

> https://codereview.chromium.org/79773005/

-- 
Thiago

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tapted, 2013-11-22 00:03:19]

Still grokking.. we might be able to make `Users` a
ui::ListModel<AppListViewDelegate::User>

But, that might be overkill, since we typically just rebuild the entire thing
each time.

https://codereview.chromium.org/79773005/diff/20001/ui/app_list/app_list_view...
File ui/app_list/app_list_view_delegate.h (right):

https://codereview.chromium.org/79773005/diff/20001/ui/app_list/app_list_view...
ui/app_list/app_list_view_delegate.h:58: AppListViewDelegate() {}
heads-up clang will probably complain about this and the destructor not having a
separate definition in the .cc (but maybe moot anyway)

[benwells, 2013-11-22 03:42:18]

On 2013/11/21 23:41:42, tfarina wrote:
> On Thu, Nov 21, 2013 at 9:39 PM,  <mailto:koz@chromium.org> wrote:
> > I agree with Xiyuan, Users should be its own class and have its own observer
> > interface.
> >
> And the observer in its own header file (please ;)).

I quite like nested observers - is there a reason why we should keep them
separate? Is that a chromium style thing I'm unaware of?

> 
> > https://codereview.chromium.org/79773005/
> 
> -- 
> Thiago
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2013-11-22 15:28:24]

On Fri, Nov 22, 2013 at 1:42 AM,  <benwells@chromium.org> wrote:
> On 2013/11/21 23:41:42, tfarina wrote:
>
>> On Thu, Nov 21, 2013 at 9:39 PM,  <mailto:koz@chromium.org> wrote:
>> > I agree with Xiyuan, Users should be its own class and have its own
>> > observer
>> > interface.
>> >
>> And the observer in its own header file (please ;)).
>
>
> I quite like nested observers - is there a reason why we should keep them
> separate?
Yes, there is. I already repeated that many times. Could ask John
Abd-El-Malek for more details? You should note that is the way we
prefer/do in ui/app_list, and Xiyuan already said he agrees with this.
Just see how many *_observer.h headers we have there (and also note
that is the way done by content API).

Thanks!

-- 
Thiago

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2013-11-22 17:56:28]

I started separating Users into an AppListUserModel class and decided that I
really didn't like it:
* It's not entirely clear where ownership of the class would belong
* Behavior between Views and Mac is significantly different and a bunch of tests
would have to be re-factored.

I'm not saying that it might not be cleaner eventually, I just don't think it's
worth it.

For now, I think we are better off just rebuilding Users whenever we show the
list in Views.

(Also, FWIW: I agree with Ben that there is nothing wrong with an embedded
Observer class, especially when it's small and purely virtual, and there is
nothing in the style guide that forbids it. That said, I also respect others
preferences and try to be consistent with existing code. In the app_list code
we've been using separate headers and should stick with that.)

[xiyuan, 2013-11-22 18:07:59]

LGTM

On 2013/11/22 17:56:28, stevenjb wrote:
> I started separating Users into an AppListUserModel class and decided that I
> really didn't like it:
> * It's not entirely clear where ownership of the class would belong
> * Behavior between Views and Mac is significantly different and a bunch of
tests
> would have to be re-factored.
> 
> I'm not saying that it might not be cleaner eventually, I just don't think
it's
> worth it.
> 
> For now, I think we are better off just rebuilding Users whenever we show the
> list in Views.

Agree.

> 
> (Also, FWIW: I agree with Ben that there is nothing wrong with an embedded
> Observer class, especially when it's small and purely virtual, and there is
> nothing in the style guide that forbids it. That said, I also respect others
> preferences and try to be consistent with existing code. In the app_list code
> we've been using separate headers and should stick with that.)

It's not mandatory, just a preferred style.

Quoted from chromium coding style:

"Prefer putting delegate classes in their own header files. Implementors of the
delegate interface will often be included elsewhere, which will often cause more
coupling with the header of the main class."

[benwells, 2013-11-25 00:45:31]

On 2013/11/22 18:07:59, xiyuan wrote:
> LGTM
> 
> On 2013/11/22 17:56:28, stevenjb wrote:
> > I started separating Users into an AppListUserModel class and decided that I
> > really didn't like it:
> > * It's not entirely clear where ownership of the class would belong
> > * Behavior between Views and Mac is significantly different and a bunch of
> tests
> > would have to be re-factored.
> > 
> > I'm not saying that it might not be cleaner eventually, I just don't think
> it's
> > worth it.
> > 
> > For now, I think we are better off just rebuilding Users whenever we show
the
> > list in Views.
> 
> Agree.
> 
> > 
> > (Also, FWIW: I agree with Ben that there is nothing wrong with an embedded
> > Observer class, especially when it's small and purely virtual, and there is
> > nothing in the style guide that forbids it. That said, I also respect others
> > preferences and try to be consistent with existing code. In the app_list
code
> > we've been using separate headers and should stick with that.)
> 
> It's not mandatory, just a preferred style.
> 
> Quoted from chromium coding style:
> 
> "Prefer putting delegate classes in their own header files. Implementors of
the
> delegate interface will often be included elsewhere, which will often cause
more
> coupling with the header of the main class."

xiyuan: thanks for the quote, that makes some sense.

tfarina: apologies if I've asked you that before, I can't recall doing so but my
memory isn't perfect.

[tapted, 2013-11-26 00:40:07]

https://codereview.chromium.org/79773005/diff/250001/ui/app_list/views/search...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/79773005/diff/250001/ui/app_list/views/search...
ui/app_list/views/search_box_view.cc:190: // Always regenerate the menu in case
view_delegate_->GetUsers() has changed.
So, mac isn't affected by the checkmark-not-moving bug, but it is affected by
the "menu not updated when profile added/removed" bug.

Unfortunately Mac's menu_controller works a bit different -- an `NSMenu` is
populated and attached to the button itself, so I don't think there's a nice way
to intercept the 'click' the way you can for context menus.

But there is already an observer interface -- OnAppListModelSigninStatusChanged
-- which triggers a menu refresh. Hacky fix: just remove the `signed_in_ ==
signed_in` check from

void AppListModel::SetSignedIn(bool signed_in) {
  if (signed_in_ == signed_in)
    return;

/* .. */
}

I think this will fix Mac. Certainly, deleting a profile, then switching the app
launcher to a profile that isn't signed in and back will refresh the menu.

So, to make the fix less hacky, WDYT about just changing
`OnAppListModelSigninStatusChanged` to
`AppListModelObserver::OnAppListModelProfileChanged` ?

It looks like void AppListView::OnSigninStatusChanged() does
`app_list_main_view_->search_box_view()->InvalidateMenu();` so other platforms
should be covered too.