Created attachment 243341 [details] [review]
appDisplay: Hide frequent view when app monitoring is disabled

Currently we stop monitoring application usage when disabling the
'enable-app-monitoring' setting, but we still expose previously
gathered data in the app picker's frequent view. This is not what
users should expect, so hide the view in that case.

Review of attachment 243341 [details] [review]:

::: js/ui/appDisplay.js
@@ +427,3 @@
+        this._controls.visible = visibleViews > 1;
+        if (!enabled && this._views[Views.FREQUENT].view.actor.visible)
+            this._showView(Views.ALL);

Ideally, we'd make showView fail early if the view wasn't available. Would make the API easier for consumers.

Created attachment 243345 [details] [review]
Replace 'enable-app-monitoring' setting

The org.gnome.desktop.privacy schema gained a 'remember-app-usage'
key, use that instead of our own preference.


Patch depends on bug 699715. See bug 699716 for a related privacy panel patch.

Review of attachment 243345 [details] [review]:

OK.

(In reply to comment #2)
> Review of attachment 243341 [details] [review]:
> 
> ::: js/ui/appDisplay.js
> @@ +427,3 @@
> +        this._controls.visible = visibleViews > 1;
> +        if (!enabled && this._views[Views.FREQUENT].view.actor.visible)
> +            this._showView(Views.ALL);
> 
> Ideally, we'd make showView fail early if the view wasn't available. Would make
> the API easier for consumers.

I don't see how that's related here? It certainly won't help with the code above (except that it assumes that Views.ALL will always be available). The code does basically:

  if (!enabled)
      this._showView(Views.ALL);

The additional condition is for the case that extensions have added additional views - if one of those has focus, there's no need to switch to the ALL view after hiding the frequent view.

Comment on attachment 243341 [details] [review]
appDisplay: Hide frequent view when app monitoring is disabled

It's only half-generic though, the controls are either hidden or shown as a whole - should take the case into account where it remains visible after hiding the frequent view.

(In reply to comment #5)
> (In reply to comment #2)
> > Review of attachment 243341 [details] [review] [details]:
> > 
> > ::: js/ui/appDisplay.js
> > @@ +427,3 @@
> > +        this._controls.visible = visibleViews > 1;
> > +        if (!enabled && this._views[Views.FREQUENT].view.actor.visible)
> > +            this._showView(Views.ALL);
> > 
> > Ideally, we'd make showView fail early if the view wasn't available. Would make
> > the API easier for consumers.
> 
> I don't see how that's related here? It certainly won't help with the code
> above (except that it assumes that Views.ALL will always be available). The
> code does basically:

What I mean is that changing this._showView(Views.FREQUENT); to instead show the ALL view would solve that case.

(In reply to comment #7)
> (In reply to comment #5)
> > I don't see how that's related here? It certainly won't help with the code
> > above (except that it assumes that Views.ALL will always be available). The
> > code does basically:
> 
> What I mean is that changing this._showView(Views.FREQUENT); to instead show
> the ALL view would solve that case.

How? We don't ever call this._showView(Views.FREQUENT); in this method, nor do I think we should.

Created attachment 243348 [details] [review]
appDisplay: Hide frequent view when app monitoring is disabled

Fixed problem noted in comment #6.

Review of attachment 243348 [details] [review]:

OK.

Attachment 243345 [details] pushed as 731e8bf - Replace 'enable-app-monitoring' setting
Attachment 243348 [details] pushed as f88d9c0 - appDisplay: Hide frequent view when app monitoring is disabled

Getting this off the list ...

I refrained from commenting, hoping to find a silver bullet, but that didn't happen. I don't think using "fake" data for the period of time when we don't yet have frequency is good (favorites). But this label gives off an "unfinished" vibe for a default state. 

There is something wrong about seeing a toggle to enable a view that's empty and telling me it's empty as the initial experience. 

Out of all the options, I like exposing all pass by default, without the view toggle at all. Only after you gather enough frequency data (5?), you present this option. 1 might be a more logical behavior, but presenting just a single item also has its downsides. I'd love to hear Jon chime in.