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 745649 - dash icon cut off
dash icon cut off
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: overview
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 746481 748519 748818 750263 752192 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-03-05 00:56 UTC by Matthias Clasen
Modified: 2016-03-23 18:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot (61.33 KB, image/png)
2015-03-05 00:56 UTC, Matthias Clasen
  Details
Sometimes the dash changes its size for no apparent reason (1.49 MB, video/webm)
2015-04-11 11:52 UTC, Luis Henrique Mello
  Details
screenshot from 3.16.2 (25.37 KB, image/jpeg)
2015-07-03 15:14 UTC, Bastien Nocera
  Details
dash: Revert mislead cleanup (2.52 KB, patch)
2015-10-05 13:38 UTC, Florian Müllner
committed Details | Review
dash: Ensure style for icon size computation (1.19 KB, patch)
2015-10-13 15:45 UTC, Florian Müllner
none Details | Review
dash: Ensure style for icon size computation (1.37 KB, patch)
2015-10-13 17:57 UTC, Florian Müllner
committed Details | Review

Description Matthias Clasen 2015-03-05 00:56:11 UTC
Created attachment 298592 [details]
screenshot

observe
Comment 1 Florian Müllner 2015-03-19 20:56:42 UTC
*** Bug 746481 has been marked as a duplicate of this bug. ***
Comment 2 Yosef Or Boczko 2015-03-27 00:12:31 UTC
Can approve this (I see it for a few weeks).
Comment 3 Luis Henrique Mello 2015-04-11 11:52:45 UTC
Created attachment 301359 [details]
Sometimes the dash changes its size for no apparent reason

Sometimes the dash increases in size for no apparent reason and does not display everything. If you launch an application, it returns to the normal size.
Comment 4 Florian Müllner 2015-04-27 13:56:57 UTC
*** Bug 748519 has been marked as a duplicate of this bug. ***
Comment 5 Britt Yazel 2015-04-28 22:21:32 UTC
I additionally have this issue. It seems to be a new bug affecting 3.16.x only. I believe the dashTodock extension has created a local work around for this within the extension.
Comment 6 Florian Müllner 2015-05-03 06:01:29 UTC
*** Bug 748818 has been marked as a duplicate of this bug. ***
Comment 7 Florian Müllner 2015-06-02 07:05:37 UTC
*** Bug 750263 has been marked as a duplicate of this bug. ***
Comment 8 Bastien Nocera 2015-07-03 15:14:39 UTC
Created attachment 306723 [details]
screenshot from 3.16.2

A few data points:
- 2560x1440 resolution
- gnome-shell-3.16.2-1.fc22.x86_64
- hi-dpi mode is turned on automatically
Comment 9 Марко Костић (Marko Kostić) 2015-07-09 15:53:41 UTC
I also experience this issue. If I start a new program or if I remove/add a program to the dash, it resizes itself properly to fit all icons. Very estetically annoying bug.
Comment 10 Isaque Galdino 2015-07-09 20:14:58 UTC
I'm experience this problem too, using a 1366x768 resolution, no hi-dpi.
Fedora 22, GNOME Shell 3.16.2.
Thanks.
Comment 11 Florian Müllner 2015-07-09 21:28:22 UTC
*** Bug 752192 has been marked as a duplicate of this bug. ***
Comment 12 Diogo Campos 2015-07-13 05:53:12 UTC
Same to me:

Fedora 22
GNOME Shell 3.16.3
Intel Driver 2.99.917
Xorg 1.17.2
Mesa 10.6.1
Kernel 4.0.5
External monitor at 1280x720 (through VGA port).
Built-in screen disabled (with "video=LVDS-1:d" kernel boot option).
Comment 13 Britt Yazel 2015-08-21 04:47:15 UTC
Has this been fixed in the 3.17 branch?
Comment 14 Bastien Nocera 2015-08-24 09:16:47 UTC
(In reply to Britt Yazel from comment #13)
> Has this been fixed in the 3.17 branch?

No, otherwise it would certainly have been marked as fixed.
Comment 15 Flo 2015-09-18 15:11:47 UTC
Same here:
Fedora 22
Gnome Shell 3.16.2 (or if I ask "gnome-session --version": 3.16.3)
Resolution: 1280 x 800
Intel: xorg-x11-drv-intel-2.99.917-15.20150729.fc22.x86_64
X.Org X Server: 1.17.2
Kernel: 4.1.6-201.fc22.x86_64
Comment 16 Cédric Bellegarde 2015-10-01 13:31:57 UTC
Same here:
ArchLinux
Radeon drivers
Gnome shell 3.18.0

Seems to only happen on my computer with dual screen.
Comment 17 Victor Porton 2015-10-01 14:48:03 UTC
> Seems to only happen on my computer with dual screen.

For me it happens despite I have single screen and never used dual screen.
Comment 18 Britt Yazel 2015-10-01 16:06:23 UTC
Yes I have it both on my laptop and on my dual-screen desktop. I have not tested with 3.18 yet to see if it still an issue.
Comment 19 Florian Müllner 2015-10-05 13:38:03 UTC
Created attachment 312678 [details] [review]
dash: Revert mislead cleanup

When adjusting dash icon sizes, we compute the icon padding by subtracting
the configured icon size from the first icon actor's preferred size. To
make sure that the preferred size correctly corresponds to the current
dash icon size even while the icon is animating, we enforce the size
before the size request. For that we used to temporarily manipulate
the icon texture size directly, but commit e92d204d425 cleaned this
up to use the setIconSize() method instead.
This does not work however, as the icon actor's iconSize property will
always match the dash iconSize property, making the method a noop. So
go back to the original approach of enforcing the texture size to make
sure we always base our computations on correct values.
Comment 20 Florian Müllner 2015-10-05 13:40:47 UTC
(In reply to Florian Müllner from comment #19)
> Created attachment 312678 [details] [review] [review]
> dash: Revert mislead cleanup

So here's one issue I found. However there must be more to it, as we've been shipping with the offending commit since 3.12, and reports for this issue have only been piling up since 3.16.
(Though it is possible that the message indicator we removed in 3.16 somehow triggered icon size recomputations that covered up the bug before)
Comment 21 Rui Matos 2015-10-11 14:59:26 UTC
Review of attachment 312678 [details] [review]:

I think I got to the root of this while reviewing this.

You're right that setIconSize() here is a nop but the real problem is that when we call _adjustIconSize() while the dash is hidden and change the icon size we hit this code in st_icon_update():

  theme_node = st_widget_peek_theme_node (ST_WIDGET (icon));
  if (theme_node == NULL)
    return;

which means that then if _adjustIconSize() is called again, the StIcon is 0x0 and so get_preferred_height(-1) doesn't take it into account makes us compute a new icon size that is larger than it should be and will only get fixed *if* _adjustIconSize() is called yet again while the dash is visible.

I think a proper fix here could be something like

+        this.actor.connect('style-changed', Lang.bind(this, this._queueRedisplay));

in Dash._init() which means that we'll always _adjustIconSize() at least once when the dash becomes visible again and thus can get proper values fixing any previous miscalculations while it was hidden. In fact, perhaps _redisplay() could even return early while the dash isn't visible?
Comment 22 Florian Müllner 2015-10-13 15:44:42 UTC
(In reply to Rui Matos from comment #21)
> You're right that setIconSize() here is a nop but the real problem is that
> when we call _adjustIconSize() while the dash is hidden and change the icon
> size we hit this code in st_icon_update():

I wouldn't say "the real problem", but "another problem" - the attached patch doesn't help when the first icon doesn't have a theme node, but the proposed solution doesn't fix the described problem we can hit when calling _adjustIconSize() while the icon is animating. So I think we need to fix both issues, style changes while the dash is hidden and calls to adjustIconSize() while the dash is animating.


> I think a proper fix here could be something like
> 
> +        this.actor.connect('style-changed', Lang.bind(this,
> this._queueRedisplay));

We don't unset the theme-node when a widget is hidden, only when the widget's style changes. So if the first icon's style changes while hidden, but the dash's style remains unchanged, we end up with the same issue again. (Not sure how theoretical that is - pretty sure we don't touch the dash style at all, so we'd only call this once when entering the overview for the first time; I'm less sure with the icon, at least its parent can change style on unmap as it's doing hover-tracking).


> In fact, perhaps _redisplay() could even return early while the dash isn't visible?

No, we'd end up animating additions/removals that happened while the dash was hidden - think of closing an app, then see its icon disappear when you enter the overview 20 minutes later.
Comment 23 Florian Müllner 2015-10-13 15:45:21 UTC
Created attachment 313202 [details] [review]
dash: Ensure style for icon size computation

When a widget's style changes while the widget is hidden, its theme node
is unset, but computation of the new node is deferred to when the widget
is mapped - make sure this doesn't throw off the size request we use to
compute the dash icon size by ensuring the icon's style first.
Comment 24 Rui Matos 2015-10-13 16:45:07 UTC
Review of attachment 312678 [details] [review]:

(In reply to Florian Müllner from comment #22)
> I wouldn't say "the real problem", but "another problem" - the attached
> patch doesn't help when the first icon doesn't have a theme node, but the
> proposed solution doesn't fix the described problem we can hit when calling
> _adjustIconSize() while the icon is animating. So I think we need to fix
> both issues, style changes while the dash is hidden and calls to
> adjustIconSize() while the dash is animating.

Oh right, I didn't think about the animation and indeed the current behavior while animating is buggy so we need this patch.
Comment 25 Rui Matos 2015-10-13 16:46:05 UTC
Review of attachment 313202 [details] [review]:

::: js/ui/dash.js
@@ +649,2 @@
         // Enforce the current icon size during the size request
+        firstIcon.ensure_style();

firstIcon isn't a StWidget, needs to be either firstButton or firstIcon.icon but the latter might be null. In any case, yes, this should work.
Comment 26 Florian Müllner 2015-10-13 17:57:44 UTC
Created attachment 313210 [details] [review]
dash: Ensure style for icon size computation

(In reply to Rui Matos from comment #25)
> firstIcon isn't a StWidget, needs to be either firstButton or firstIcon.icon

On closer look, it needs to be the latter - the call to firstIcon.icon.get_size() already takes care of creating all theme nodes as necessary, the only thing's missing is the ::style-changed signal for StIcon to update its texture. 

StIcon will skip loading the texture when its theme node is unset (which
may happen on style changes while the widget is hidden). While our size
request to compute the dash icon size will create the icon's theme node
if necessary (and of all its parents), a missing texture can still throw
off our computation.
Make sure this doesn't happen by ensuring the icon's style first, so the
texture is updated in response to StWidget::style-changed if necessary.
Comment 27 Britt Yazel 2015-10-13 17:59:46 UTC
Will these patches make it in time for 3.18.1, or do we need to wait for 3.18.2?
Comment 28 Марко Костић (Marko Kostić) 2015-10-13 18:02:40 UTC
Also, will it be backported to the 3.16 branch?
Comment 29 Florian Müllner 2015-10-13 18:05:19 UTC
(In reply to Britt Yazel from comment #27)
> Will these patches make it in time for 3.18.1

It's up to me to roll the 3.18.1 release, so yes.


(In reply to Марко Костић (Marko Kostić) from comment #28)
> Also, will it be backported to the 3.16 branch?

They should apply cleanly, so sure.
Comment 30 Florian Müllner 2015-10-13 18:27:27 UTC
Attachment 312678 [details] pushed as 9720b32 - dash: Revert mislead cleanup
Attachment 313210 [details] pushed as 14c52bb - dash: Ensure style for icon size computation

We've never had a reproducer for this issue, so let's keep this open for a while to see if it goes away with the pushed patches.
Comment 31 Марко Костић (Marko Kostić) 2015-10-13 19:32:49 UTC
> We've never had a reproducer for this issue, so let's keep this open for a while to see if it goes away with the pushed patches.

That's great! I am willing to test out the fixes.

I hope I'm not going over the edge by asking this. Is it possible to get newer gnome-shell package in updates-testing repo, in Fedora 22 (and 23) so I (and somebody else, hopefully) could test these fixes out?

Thanks Florian! You rock. :)
Comment 32 Juraj Fiala 2016-03-23 18:48:44 UTC
Well, it's 5 months now and I haven't encountered the issue at all, and it used to be quite frequent.
Comment 33 Florian Müllner 2016-03-23 18:51:15 UTC
OK, let's assume the pushed patches fixed it then.
(I haven't seen it anymore either, but it was never a frequent problem for me to begin with).
Comment 34 Flo 2016-03-23 18:55:24 UTC
Haven't seen it for quite some time... seems to be fixed.