After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 736910 - appDisplay: Hide empty app folders
appDisplay: Hide empty app folders
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 709962 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-09-18 14:37 UTC by Florian Müllner
Modified: 2014-09-25 05:32 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
appDisplay: Hide empty app folders (1.20 KB, patch)
2014-09-18 14:37 UTC, Florian Müllner 		accepted-commit_now Details | Review
appDisplay: Hide empty app folders (951 bytes, patch)
2014-09-18 17:07 UTC, Florian Müllner 		committed Details | Review

Description Florian Müllner 2014-09-18 14:37:28 UTC 
See patch.
Comment 1 Florian Müllner 2014-09-18 14:37:32 UTC 
Created attachment 286504 [details] [review]
appDisplay: Hide empty app folders

Trying to open an empty folder currently leaves the parent view in a
rather confused state. While we should look into fixing this in the
future, empty folders are not useful at all to begin with, so hide them.
Comment 2 Carlos Soriano 2014-09-18 14:47:15 UTC 
Review of attachment 286504 [details] [review]:

The code looks a little confusing(why hide the actor? Why actor.show for each app?), but I don't have a better proposal. So LGTM.
Comment 3 Florian Müllner 2014-09-18 14:56:35 UTC 
(In reply to comment #2)
> The code looks a little confusing(why hide the actor? Why actor.show for each
> app?), but I don't have a better proposal.

Well, an alternative would be to do something like

   this.actor.visible = this.view.getAllItems().length > 0;

at the end of _redisplay() ...
Comment 4 Carlos Soriano 2014-09-18 16:21:34 UTC 
(In reply to comment #3)
> (In reply to comment #2)
> > The code looks a little confusing(why hide the actor? Why actor.show for each
> > app?), but I don't have a better proposal.
> 
> Well, an alternative would be to do something like
> 
>    this.actor.visible = this.view.getAllItems().length > 0;
> 
> at the end of _redisplay() ...

That could be better, but do we need to also make sure at least one app pass the set of ifs:
if (excludedApps.indexOf(appId) >= 0)
                return;

            let app = appSys.lookup_app(appId);
            if (!app)
                return;

            if (!app.get_app_info().should_show())
                return;

So maybe the code looks worse in this case.
If I'm wrong in this, then your new proposal looks indeed better.
Comment 5 Florian Müllner 2014-09-18 17:07:53 UTC 
Created attachment 286527 [details] [review]
appDisplay: Hide empty app folders

(In reply to comment #4)
> (In reply to comment #3)
> > Well, an alternative would be to do something like
> > 
> >    this.actor.visible = this.view.getAllItems().length > 0;
> > 
> > at the end of _redisplay() ...
> 
> That could be better, but do we need to also make sure at least one app pass
> the set of ifs:
> if (excludedApps.indexOf(appId) >= 0)
>                 return;


apps that don's pass any of those tests are not added in the first place, so the attached patch handles this just fine ...
Comment 6 Carlos Soriano 2014-09-18 17:18:59 UTC 
Review of attachment 286527 [details] [review]:

Right, I was reading it as a function called after this function scope. But it's not.
LBetterTM now.
Comment 7 Florian Müllner 2014-09-19 13:13:56 UTC 
Attachment 286527 [details] pushed as 3ae45bd - appDisplay: Hide empty app folders
Comment 8 Florian Müllner 2014-09-25 05:32:48 UTC 
*** Bug 709962 has been marked as a duplicate of this bug. ***