GNOME Bugzilla – Bug 694710
Frequent apps is empty by default
Last modified: 2013-09-02 19:30:39 UTC
Out of the box, the frequent apps well is empty. So if a user does 1) go to overview 2) click on the app grid icon then they're presented with no icons unless they click All We should probably do one of - select All by default if Frequent is empty - Hide the tabs completely unless there are apps that are frequently used - prepopulate Frequent with apps a typical gnome user would frequently use. The first there seems like the most straight forward and least astonishing to me, but anyone of them would be better than the current situation (i'm up for potentially other ideas too)
(In reply to comment #0) > Out of the box, the frequent apps well is empty. So if a user does > > 1) go to overview > 2) click on the app grid icon > > then they're presented with no icons unless they click All > > We should probably do one of > > - select All by default if Frequent is empty > - Hide the tabs completely unless there are apps that are frequently used > - prepopulate Frequent with apps a typical gnome user would frequently use. Even though it is kind of redundant why not prepoulate it with the favorites?
*** Bug 695068 has been marked as a duplicate of this bug. ***
Or we can make the 'All' view the sensible default again. Or we can do away with the frequent view, because that's what the dash is for.
The dash doesn't show frequently used apps.
Not automatically, but what do people use the dash for? I'd contend that "Favorite" apps are largely the same as "Frequent" apps.
Created attachment 242658 [details] [review] appDisplay: Only show frequent apps if it is not empty. https://bugzilla.gnome.org/show_bug.cgi?id=694710
I aded a patch for testing. Now, all works as expected, but I added a hardcoded boolean to test the behaviour at line 352. If true(we have some frequent apps to show), we show frequent apps view is shown as default and also we create buttons to change between views, if false(we haven't got some frequent apps to show), no buttons shown and all apps view shown by default. this patch will be more useful with this: https://bugzilla.gnome.org/show_bug.cgi?id=694946
(In reply to comment #1) > Even though it is kind of redundant why not prepoulate it with the favorites? This was the first thing Jon said when we discussed this a while ago - let's get some design review here.
(In reply to comment #6) > appDisplay: Only show frequent apps if it is not empty. I've thought about this a little, with the conclusion that personally I don't like this approach. My reasoning is as follows: there have been some requests to allow disabling the frequent view entirely, and I can actually see how this makes sense in the context of the new privacy panel. In fact, application monitoring has been optional from the get-go, see the enable-app-monitoring setting in org.gnome.shell - however the key currently only disables monitoring itself, it doesn't affect the UI (except for the existing frequent apps no longer being updated of course). But I think it does make sense to actually hide the frequent view when monitoring is disabled - at which point I'd consider a difference between "monitoring turned off" and "not enough data yet" helpful (the toggle-off case could be made to work by pruning the existing data, but only updating UI a while after toggling a setting is a bit confusing). So while awaiting some actual designer feedback, my suggestion would be to first implement binding the frequent view's visibility to the app-monitoring setting (I'd expect this to move to gsettings-desktop-schemas if we want it exposed in g-c-c, but that's trivially changed later). If we do decide that hiding is the correct solution for the empty case as well, all that will be needed is an additional condition to show/hide the frequent view. Carlos, does that sound reasonable? Can you adapt the patch accordingly? Note that in any case, your current approach of determining the visibility once at startup and never updating it later is not great, we shouldn't require users to sign out to see changes. While it is possible to create/destroy UI accordingly, actually hiding the frequent view (and the view switcher) is likely easier.
I'll just say here again: I think the "Frequent" design results from our lack of application installation/uninstallation, so distributions end up shoveling lots of stuff into the default install, and we try to work around it by filtering it...
I also think a big help would be to have the UI say "no frequent applications yet" would be much better than blank space, which just looks broken.
Hi, (In reply to comment #9) > (In reply to comment #6) > > appDisplay: Only show frequent apps if it is not empty. > > I've thought about this a little, with the conclusion that personally I don't > like this approach. My reasoning is as follows: there have been some requests > to allow disabling the frequent view entirely, and I can actually see how this > makes sense in the context of the new privacy panel. In fact, application > monitoring has been optional from the get-go, see the enable-app-monitoring > setting in org.gnome.shell - however the key currently only disables monitoring > itself, it doesn't affect the UI (except for the existing frequent apps no > longer being updated of course). But I think it does make sense to actually > hide the frequent view when monitoring is disabled - at which point I'd > consider a difference between "monitoring turned off" and "not enough data yet" > helpful (the toggle-off case could be made to work by pruning the existing > data, but only updating UI a while after toggling a setting is a bit > confusing). > > So while awaiting some actual designer feedback, my suggestion would be to > first implement binding the frequent view's visibility to the app-monitoring > setting (I'd expect this to move to gsettings-desktop-schemas if we want it > exposed in g-c-c, but that's trivially changed later). If we do decide that > hiding is the correct solution for the empty case as well, all that will be > needed is an additional condition to show/hide the frequent view. > > Carlos, does that sound reasonable? Can you adapt the patch accordingly? Note > that in any case, your current approach of determining the visibility once at > startup and never updating it later is not great, we shouldn't require users to > sign out to see changes. While it is possible to create/destroy UI accordingly, > actually hiding the frequent view (and the view switcher) is likely easier. Seems reasonable. I can addapt de patch. Seems not too hard to find the required setting to implement that. And hide instead of create the views seems more reasonable than the current approach. Also, I implemented the patch as it is becasue I don't like the idea of open the app view, see nothing(because there's not enough data) and in the same session, after you open some apps a "frequent apps view" appears. So, I can see people filing bugs saying "frequent app view is not sometimes shown". What do you think about? If not, we can toggle a signal when there's enough data to show "frequent app view" as you said.
(In reply to comment #11) > I also think a big help would be to have the UI say "no frequent applications > yet" would be much better than blank space, which just looks broken. I agree, it will not confuse the user.
(In reply to comment #13) > (In reply to comment #11) > > I also think a big help would be to have the UI say "no frequent applications > > yet" would be much better than blank space, which just looks broken. > > I agree, it will not confuse the user. Yes, blank space is broken. Either a message as suggested or pre-filling look like valid options to me.
Created attachment 243212 [details] [review] appDisplay: Only show frequent apps if it is not empty. https://bugzilla.gnome.org/show_bug.cgi?id=694710
Created attachment 243214 [details] [review] AppDisplay: Show label when frequent app view is empty. Currently we show a empty view, that seems broken, so we add a label showing that there's not enough frequent applications to show.
(In reply to comment #15) > Created an attachment (id=243212) [details] [review] > appDisplay: Only show frequent apps if it is not empty. > https://bugzilla.gnome.org/show_bug.cgi?id=694710 Sorry, this attachment is not correct.
In my last patch I added a label like Jasper said, and I think it is a good approach. But, if we you in the code... I had to do some hacks that I think are wrong... Basically, to show the label at center I had to set the align of the parent widget (this.actor) to CENTER, and, to show the frequent app view I had to set the align of the parent widget to FILL (like it was before). So..there's a better way to do this..? thanks!
(In reply to comment #18) > .there's a better way to do this..? FrequentView:actor doesn't respect a child's x-align/y-align properties; it will once you add an appropriate layout manager.
Created attachment 243282 [details] [review] AppDisplay: Show label when frequent app view is empty. Currently we show a empty view, that seems broken, so we add a label showing that there's not enough frequent applications to show.
Thanks Florian, here it is the fixed patch. But...I only was be able to fix it with a hack(after some frustration because it is not working as expected) needed for ClutterBinLayout which I found in other part of the code. Do we really need to do this or is there some better manner?
(In reply to comment #21) > But...I only was be able to fix it with a hack(after some frustration because > it is not working as expected) needed for ClutterBinLayout which I found in > other part of the code. Not sure what you mean - it works just fine here if I remove those lines (also note that x_expand is already set in the parent class, so no need to do it again).
Review of attachment 243282 [details] [review]: Generally looks good, thanks! ::: data/theme/gnome-shell.css @@ +908,3 @@ +.no-frequent-applications-label { + font-family: cantarell, sans-serif; Why? ::: js/ui/appDisplay.js @@ +306,3 @@ + // Standard hack for ClutterBinLayout + this._grid.actor.x_expand = true; + this._grid.actor.y_expand = true; What exactly does this fix? @@ +313,3 @@ + y_expand: true }); + + this._noFrequentAppsLabel = new St.Label({ text: _("No frequent applications yet"), Note to self: exact message text needs design approval before committing. @@ +333,3 @@ loadApps: function() { let mostUsed = this._usage.get_most_used (""); + if(mostUsed.length >= 1) I'm not sure 1 is the right threshold; I'd probably do something like this._noFrequentAppsLabel.visible = mostUsed.length >= MIN_FREQUENT_APPS_COUNT; if (!this._noFrequentAppsLabel.visible) return; and define MIN_FREQUENT_APPS_COUNT to ... 3 maybe?
(In reply to comment #23) > Review of attachment 243282 [details] [review]: > > Generally looks good, thanks! > > ::: data/theme/gnome-shell.css > @@ +908,3 @@ > > +.no-frequent-applications-label { > + font-family: cantarell, sans-serif; > > Why? Ups, I guess it is not necessary. Done. > > ::: js/ui/appDisplay.js > @@ +306,3 @@ > + // Standard hack for ClutterBinLayout > + this._grid.actor.x_expand = true; > + this._grid.actor.y_expand = true; > > What exactly does this fix? > I asked to Jasper(who previously used the hack) and tell me that ClutterBinLayout doesn't take actor align flags into account unless the actor is expanding. If the actor isn't expanding, it uses the child's align flags. So that's why the frequent View is going to the bottom rigth. > @@ +313,3 @@ > + y_expand: true }); > + > + this._noFrequentAppsLabel = new St.Label({ text: _("No frequent > applications yet"), > > Note to self: exact message text needs design approval before committing. > Ok, I also noted to self =) > @@ +333,3 @@ > loadApps: function() { > let mostUsed = this._usage.get_most_used (""); > + if(mostUsed.length >= 1) > > I'm not sure 1 is the right threshold; I'd probably do something like > > this._noFrequentAppsLabel.visible = mostUsed.length >= MIN_FREQUENT_APPS_COUNT; > if (!this._noFrequentAppsLabel.visible) > return; > > and define MIN_FREQUENT_APPS_COUNT to ... 3 maybe? Done.
Created attachment 243334 [details] Actor not respecting alignments (In reply to comment #22) > (In reply to comment #21) > > But...I only was be able to fix it with a hack(after some frustration because > > it is not working as expected) needed for ClutterBinLayout which I found in > > other part of the code. > > Not sure what you mean - it works just fine here if I remove those lines (also > note that x_expand is already set in the parent class, so no need to do it > again). I don't know why it is not working for me so. I attach a image showing what's happening if I deleted those lines.
Oh, looks like a local patch was to blame ... OK then, are you gonna attach an updated patch?
Created attachment 243337 [details] [review] AppDisplay: Show label when frequent app view is empty. Currently we show a empty view, that seems broken, so we add a label showing that there's not enough frequent applications to show.
(In reply to comment #26) > Oh, looks like a local patch was to blame ... OK then, are you gonna attach an > updated patch? of course! thanks for the help Florian.
the last patch I attached is the good one.
Review of attachment 243337 [details] [review]: Still needs design review, otherwise good except for some minor nitpicks: ::: js/ui/appDisplay.js @@ +337,3 @@ + if(!this._noFrequentAppsLabel.visible) + { + this._noFrequentAppsLabel.hide(); show()/hide() just sets the actor's visibility, so it's pointless to do both. You won't need the else{} part then, so I'd suggest an early return instead of having most of the function in an if{} block (note that if you don't follow that, you need to fix the braces, which go on the same line as if/else)
Created attachment 243838 [details] [review] AppDisplay: Show label when frequent app view is empty. Currently we show a empty view, that seems broken, so we add a label showing that there's not enough frequent applications to show.
By the way, > otherwise good except for some minor nitpicks Done in last patch Thanks very much Florian for the patience and explanatory review. Now waiting for design feedback.
Created attachment 244967 [details] Screenshot with patch applied
What happens when you set "do not track history" in the privacy settings?
(In reply to comment #34) > What happens when you set "do not track history" in the privacy settings? Nothing, "history" is about files; I filed bug 699716 for "usage", e.g. apps. The frequent view is completely hidden in that case (bug 699714)
*** Bug 700876 has been marked as a duplicate of this bug. ***
*** Bug 702889 has been marked as a duplicate of this bug. ***
I thought we made more progress on this for 3.10. There was some discussion on IRC and I can't remember the details, but I would imagine that we'd be looking for a heuristic like X number of apps launched Y times before showing the frequent section. X == 4, Y == 3 ?
Created attachment 253646 [details] [review] AppDisplay: Show label when frequent app view is empty. Currently we show a empty view, that seems broken, so we add a label showing that there's not enough frequent applications to show.
Created attachment 253647 [details] [review] appDisplay: Default to All view when not enough usage data is available The frequent view is not useful when it doesn't contain any applications yet. While the previously added label makes this state appear less like an error (OMG, my apps are gone!), it doesn't address the issue of usefulness - default to the more helpful All view in this case.
The first patch is Carlos' patch rebased on top of bug 706081 with an updated string (suggested by Allan on IRC). Allan also suggested to not default to the frequent view when the label is visible, which is what the second patch implements.
Review of attachment 253646 [details] [review]: ::: js/ui/appDisplay.js @@ +693,3 @@ let mostUsed = this._usage.get_most_used (""); + this._noFrequentAppsLabel.visible = mostUsed.length < MIN_FREQUENT_APPS_COUNT; + if(this._noFrequentAppsLabel.visible) missing space, and I'd prefer this to be a separate condition let haveApps = mostUsed.length >= MIN_FREQUENT_APPS_COUNT; this._noFrequentAppsLabel.visible = !haveApps; if (!haveApps) return;
Review of attachment 253647 [details] [review]: Ah, this is where the split out condition is. I'd prefer it to be squashed in the previous patch, but OK.
Comment on attachment 253647 [details] [review] appDisplay: Default to All view when not enough usage data is available Attachment 253647 [details] pushed as 938628a - appDisplay: Default to All view when not enough usage data is available
Pushed with suggested code change and after r-t approval.