GNOME Bugzilla – Bug 786120
User's avatar does not get updated when the scaling-factor changes
Last modified: 2017-08-11 10:49:07 UTC
When switching from one resolution to another that implies getting the scaling-factor's value changed (e.g. moving from 1920x1080 to 3200x1800 on my 13" Yoga 900 laptop), the size of the user's avatar does not get updated accordingly. This might not be a terrible issue on the upstream shell since the avatar is not as prominent as on Endless (where it's featured right on the bottom panel), but I think the fix is still relevant, so it would be nice to land it here as well.
Created attachment 357353 [details] [review] Resize the user's avatar when scaling factor changes
Review of attachment 357353 [details] [review]: ::: js/ui/userWidget.js @@ +57,3 @@ this.actor.child = null; this.actor.style = 'background-image: url("%s");'.format(iconFile); + this.actor.set_size(this._iconSize * scaleFactor, this._iconSize * scaleFactor); I think you should move this out of the if-block: The actor has an explicit size (in _init), so we shouldn't rely on it to adjust its size to a resized child in the else-block. @@ +61,3 @@ this.actor.style = null; this.actor.child = new St.Icon({ icon_name: 'avatar-default-symbolic', + icon_size: this._iconSize * scaleFactor }); This looks wrong: the icon-size property inside StIcon is considered unscaled, and the scale is applied when the icon texture is loaded. So I'm pretty sure you are double-scaling here.
Created attachment 357354 [details] [review] Resize the user's avatar when scaling factor changes The previous patch considered the scaling factor when setting the size via StIcon's icon-size, which was not needed (it will be handled internally in st_icon_update())
(In reply to Florian Müllner from comment #2) > @@ +61,3 @@ > this.actor.style = null; > this.actor.child = new St.Icon({ icon_name: > 'avatar-default-symbolic', > + icon_size: this._iconSize * > scaleFactor }); > > This looks wrong: the icon-size property inside StIcon is considered > unscaled, and the scale is applied when the icon texture is loaded. So I'm > pretty sure you are double-scaling here. Yes, I've noticed that after submitting and already proposed a newer patch with that fixed, even before seeing your comment :-) > ::: js/ui/userWidget.js > @@ +57,3 @@ > this.actor.child = null; > this.actor.style = 'background-image: > url("%s");'.format(iconFile); > + this.actor.set_size(this._iconSize * scaleFactor, > this._iconSize * scaleFactor); > > I think you should move this out of the if-block: The actor has an explicit > size (in _init), so we shouldn't rely on it to adjust its size to a resized > child in the else-block. I'm not sure I follow: if I moved this out of the if-block, wouldn't it be apply the scaling factor twice for the "else" side of the branch? Can you clarify? Thanks!
(In reply to Mario Sánchez Prada from comment #4) > I'm not sure I follow: if I moved this out of the if-block, wouldn't it be > apply the scaling factor twice for the "else" side of the branch? Can you > clarify? Thanks! Currently the actor has a fixed size that is applied once during initialization. Your patch changes that to update the size in the if-block, but not in the else part - what I would expect from that is that the actor keeps its original fixed size, and the icon is scaled to fit the parent independent from the content size (that is determined by the icon-size parameter). I did a quick test now (in lookingGlass): b = new St.Bin({ width: 32, height: 32 }); Main.layoutManager.addChrome(b); b.child = new St.Icon({ icon_name: 'start-here', icon_size: 64 }) b.child.width >>>> 32, not 64 I don't think it's different with the scale factor ...
Created attachment 357396 [details] [review] Resize the user's avatar when scaling factor changes (In reply to Florian Müllner from comment #5) > [...] > Currently the actor has a fixed size that is applied once during > initialization. Your patch changes that to update the size in the if-block, > but not in the else part - what I would expect from that is that the actor > keeps its original fixed size, and the icon is scaled to fit the parent > independent from the content size (that is determined by the icon-size > parameter). Ah! Looks like I was mixing the actor with its child, and that's where the confusion came from, sorry about that. I understand what you mean now, thanks for the clarification. See the new patch attached.
Review of attachment 357396 [details] [review]: LGTM
Fixed: https://git.gnome.org/browse/gnome-shell/commit/?id=b8eeac6fcf5505736b94d35dea0ece40caa048c9