Created attachment 239703 [details]
Screenshots

Yep, needs more padding between the bar and the bottom of the OSD.

Is anybody looking at this?

As a beginner of Gnome development I would like to start with this bug. Related to that I have been looking into osdWindow.js.

My suggestion would be just to move icon, label and level bar up or to change the size of icon and move only the label and level bar, in case that both of them are showing up.

(In reply to comment #4)
> My suggestion would be just to move icon, label and level bar up or to change
> the size of icon and move only the label and level bar, in case that both of
> them are showing up.

The position of icon, label and level bar is not set explicitly, but determined from the corresponding styles in data/theme/gnome-shell.css (look for .osd-window). What is set explicitly are the icon-size and the size of the entire popup (in _monitorsChanged). This approach works OK as long as the calculated popup height is taller than the size needed by the content - extra space will be added as additional padding to the icon (if we are lucky, that is still the case with all elements visible, if we are unlucky, it already happens with either label or level bar).

So the possible fixes are:
 (1) set padding/spacing dynamically from code
     (and possibly font-size and .level-bar height
      as well if necessary), so that everything fits
     the popup
 (2) use a smaller icon size if necessary
 (3) allow the popup to grow larger if it doesn't fit

(3) is probably the easiest approach - using min-width/min-height instead of an explicit size should be mostly enough ...

Created attachment 242616 [details]
The size of an icon changes to smaller one in case that OSD has both a label and a level bar

Level bar is not too close to the border anymore and it stays at the same position as in the case when there is no label.

Created attachment 242617 [details] [review]
The size of an icon changes to smaller one in case that OSD has both a label and a level bar

Level bar is not too close to the border anymore and it stays at the same
position as in the case when there is no label.

Review of attachment 242617 [details] [review]:

::: js/ui/osdWindow.js
@@ +176,3 @@
 
+	if (this._label.text)
+	        this._icon.icon_size = size / 2.3;

No, this condition is unsuitable. First, having *only* a label shouldn't require any more space than having *only* a progress bar. But more importantly, whether the icon needs further scaling or not does not just depend on the number of visible elements, but also on the screen size. If I use my laptop's native resolution of 1440x900, OSDs with both label and level bar are actually fine, only if I lower the resolution to 1024x768 or lower can I reproduce the problem.

So all the different factor does is make it less likely to hit the problem. We should fix it instead.

(I still think that using min-width/height instead of a fixed size as of comment #5 is a far easier approach)

(In reply to comment #8)
> Review of attachment 242617 [details] [review]:
> 
> ::: js/ui/osdWindow.js
> @@ +176,3 @@
> 
> +    if (this._label.text)
> +            this._icon.icon_size = size / 2.3;
> 
> No, this condition is unsuitable. First, having *only* a label shouldn't
> require any more space than having *only* a progress bar. But more importantly,
> whether the icon needs further scaling or not does not just depend on the
> number of visible elements, but also on the screen size. If I use my laptop's
> native resolution of 1440x900, OSDs with both label and level bar are actually
> fine, only if I lower the resolution to 1024x768 or lower can I reproduce the
> problem.
> 
> So all the different factor does is make it less likely to hit the problem. We
> should fix it instead.
> 
> (I still think that using min-width/height instead of a fixed size as of
> comment #5 is a far easier approach)

Yeah, of course, how blind I was. Definitely not good approach. Ok, let's try with min-width/height.

Created attachment 242643 [details] [review]
Set the min-width/height in css file so that OSD gets bigger dynamically

I set the min-width/height to 150px because with smaller number OSD's width and height are not the same. So with the min 150px we get the same with and height and they grow dynamically if both level-bar and label are shown in the popoup.

Review of attachment 242643 [details] [review]:

::: js/ui/osdWindow.js
@@ -172,3 @@
         let size = 110 * Math.max(1, scale);
 
-        this._box.set_size(size, size);

I would still use a popup size relative to the monitor, by setting min-width/height from code rather than the CSS. The goal of the patch should be to not change the UI except for the case this bug is about.

(In reply to comment #11)
> Review of attachment 242643 [details] [review]:
> 
> ::: js/ui/osdWindow.js
> @@ -172,3 @@
>          let size = 110 * Math.max(1, scale);
> 
> -        this._box.set_size(size, size);
> 
> I would still use a popup size relative to the monitor, by setting
> min-width/height from code rather than the CSS. The goal of the patch should be
> to not change the UI except for the case this bug is about.

Which is the best way to determine min-width/height from code? Should I write a function to calculate it depending on elements which are included or there is some function that returns those values?

Looking through the code I have noticed often use of _getPreferredWidth/height which is used by containers, so I guess not something that we need.

(In reply to comment #12)
> Which is the best way to determine min-width/height from code? Should I write a
> function to calculate it depending on elements which are included or there is
> some function that returns those values?

I'm suggesting to base it on the existing size calculation, e.g. unless the popup needs growing, it will have the exact same size as now.
If you use the existing size as min-width/min-height though, you'll notice that the popup size does change - the reason being that padding and border don't contribute to the min-size (see https://git.gnome.org/browse/gnome-shell/tree/src/st/st-theme-node.c#n3522). So to get the desired result, just subtract padding/border from the size to get the min-size.

(In reply to comment #13)
> (In reply to comment #12)
> > Which is the best way to determine min-width/height from code? Should I write a
> > function to calculate it depending on elements which are included or there is
> > some function that returns those values?
> 
> I'm suggesting to base it on the existing size calculation, e.g. unless the
> popup needs growing, it will have the exact same size as now.
> If you use the existing size as min-width/min-height though, you'll notice that
> the popup size does change - the reason being that padding and border don't
> contribute to the min-size (see
> https://git.gnome.org/browse/gnome-shell/tree/src/st/st-theme-node.c#n3522). So
> to get the desired result, just subtract padding/border from the size to get
> the min-size.

So basically adding only the following lines should be enough?

let min_size = size - this._padding - this._border;
this._box.set_size(min_size, min_size);

(In reply to comment #14)
> So basically adding only the following lines should be enough?
> 
> let min_size = size - this._padding - this._border;
> this._box.set_size(min_size, min_size);

No. set_size() is what we do now, it doesn't set the minimum size. There is no direct way to set it, but StWidgets have a "style" property which you can use to add CSS from code.

(In reply to comment #15)
> (In reply to comment #14)
> > So basically adding only the following lines should be enough?
> > 
> > let min_size = size - this._padding - this._border;
> > this._box.set_size(min_size, min_size);
> 
> No. set_size() is what we do now, it doesn't set the minimum size. There is no
> direct way to set it, but StWidgets have a "style" property which you can use
> to add CSS from code.

So my conclusion would be to not set the explicit size in function _monitorsChanged, but to base it on the existing size calculation with substrasted padding/border and add it as min-width/height in the style of StWidget:

let monitor = Main.layoutManager.primaryMonitor;       
let scalew = monitor.width / 640.0;        
let scaleh = monitor.height / 480.0;       
let scale = Math.min(scalew, scaleh);       
let size = 110 * Math.max(1, scale);
let min_size = size - this._padding - this._border;

this.actor = new St.Widget({ x_expand: true,
                             y_expand: true,
                             x_align: Clutter.ActorAlign.CENTER,
                             y_align: Clutter.ActorAlign.CENTER
                             style: "min-width: $min_size; min-height: $min_size;"});

(In reply to comment #16)
> So my conclusion would be to not set the explicit size in function
> _monitorsChanged, but to base it on the existing size calculation with
> substrasted padding/border and add it as min-width/height

Yup.


> let monitor = Main.layoutManager.primaryMonitor;       
> let scalew = monitor.width / 640.0;        
> let scaleh = monitor.height / 480.0;       
> let scale = Math.min(scalew, scaleh);       
> let size = 110 * Math.max(1, scale);
> let min_size = size - this._padding - this._border;

this._padding and this._border are both undefined. If they were defined, both padding and border are per side, so assuming equal padding and border, you'd need to multiply each by 2. However you shouldn't make this assumption, so min-width would be size - leftPadding - rightPadding - leftBorderWidth - rightBorderWidth and min-height  size - topPadding - bottomPadding - topBorderWidth - bottomBorderWidth.
You can get those lengths from the actor's theme node (get_theme_node()); for padding, you can use get_horizontal_padding() and get_vertical_padding() for a little shortcut.


> this.actor = new St.Widget({ x_expand: true,
>                              y_expand: true,
>                              x_align: Clutter.ActorAlign.CENTER,
>                              y_align: Clutter.ActorAlign.CENTER
>                              style: "min-width: $min_size; min-height:
> $min_size;"});

No.

First: you are replacing the existing actor with all its children with a new actor without any children. "style" is not a construct-only property, so you can just set it without recreating the while hierarchy:

this.actor.style = ...

Second, the style you are using is not valid CSS - you are passing a string ("$min_size") instead of a length (there's no fancy shell-like substitution, sorry). And if it would work, you'd still be omitting a unit (px, pt, em, ...)

Created attachment 242712 [details] [review]
Set min-width/height dynamically from code

minWidth/height calculation is based on the existing size calculation with
substracted padding/border. min-width/height are set in the "style" property.

Review of attachment 242712 [details] [review]:

::: js/ui/osdWindow.js
@@ +77,3 @@
                                      y_align: Clutter.ActorAlign.CENTER });
+
+	Main.uiGroup.add_actor(this.actor);

Mmh, I see what you are doing here - then canonical way of making sure that you can access the theme node safely is to connect to the widgets 'style-changed' signal and only use the theme node there (this also allows you to pick up changes, for instance when properties change on hover). Note that in this case it's a bit tricky, as you are modifying the style itself (which will itself trigger a 'style-changed' signal), but doable (caching for the rescue!).

@@ +177,3 @@
+	let horizontalPadding = this._box.get_theme_node().get_horizontal_padding();
+	let verticalPadding = this._box.get_theme_node().get_vertical_padding();
+	let topBorder = this._box.get_theme_node().get_border_width(0);

Don't use magic numbers, use St.Side.TOP

@@ +185,3 @@
+	let minHeight = size - horizontalPadding - topBorder - bottomBorder;
+
+	this._box.style = 'min-width: ' + minWidth + 'px' + ';' + 'min-height: ' + minHeight + 'px' + ';';

Only a personal preference (read: feel free to ignore if you disagree), but I like
  this._box.style = 'min-width: %dpx; min-height: %dpx;'.format(minWidth, minHeight);
better..

Created attachment 243198 [details] [review]
Set min-width/height dynamically from code

Small changes from the previous patch.

Created attachment 243203 [details] [review]
Change the size dynamically from code 

Small changes from the previous patch.

Review of attachment 243203 [details] [review]:

This is getting closer.

Apart from the comments below, I think we want the popup to be always square. This was the case when setting width/height directly (with the known problem that the content would overflow), but isn't anymore with min-width/min-height.

::: js/ui/osdWindow.js
@@ +78,3 @@
                                      y_align: Clutter.ActorAlign.CENTER });
+
+        this.actor.connect('style-changed', Lang.bind(this, this._onStyleChanged));

This is not quite correct - you are interested in this._box' theme node, so you should connect to that widget's signal.

@@ +173,3 @@
         let scaleh = monitor.height / 480.0;
         let scale = Math.min(scalew, scaleh);
+        this._totalSize = 110 * Math.max(1, scale);

I know I spontaneously made up that name on IRC, but it's really not that good - maybe this._popupSize?

@@ +177,3 @@
         this._box.translation_y = monitor.height / 4;
 
+        this._icon.icon_size = this._totalSize / 2;

You still need to update the popup size if possible! To test: open Settings -> Displays and change the resolution. The popup size is expected to adapt to the change.

@@ +191,3 @@
+        let minHeight = this._totalSize - horizontalPadding - topBorder - bottomBorder;
+
+        this._box.style = 'min-width: %dpx; min-height: %dpx;'.format(minWidth, minHeight);

Ha, I was assuming that you'd need to cache the properties to avoid a loop (::style-changed => _onStyleChanged() => style = ... => ::style-changed => ...), but StWidget already takes care of this \o/

(In reply to comment #22)
> Review of attachment 243203 [details] [review]:
> 
> This is getting closer.
> 
> Apart from the comments below, I think we want the popup to be always square.
> This was the case when setting width/height directly (with the known problem
> that the content would overflow), but isn't anymore with min-width/min-height.

Ok... so that means that we can't actually use min-width/min-height. Then what I was thinking is to change the size of the popup deepening on the elements which will appear. Inside the function _onStyleChanged I would do the following:

let labelSize = (this._label.visible) ? this._label.get_theme_node().get_theme_node().get_height() : 0;  

let levelSize = (this._level.actor.visible) ? this._level.actor.get_theme_node().get_theme_node().get_height() : 0;  

... and then calculate the size:
let size = this._totalSize + labelSize + levelSize - verticalPadding;
this._box.style = 'width: %dpx; height: %dpx;'.format(size, size);

> @@ +177,3 @@
>          this._box.translation_y = monitor.height / 4;
> 
> +        this._icon.icon_size = this._totalSize / 2;
> 
> You still need to update the popup size if possible! To test: open Settings ->
> Displays and change the resolution. The popup size is expected to adapt to the
> change.

Yeah.. so if I just use this._box.style = ... it will trigger the signal for 'style-changed' and it will be updated.

(In reply to comment #23)
> Ok... so that means that we can't actually use min-width/min-height.

Heh, I didn't say that :-)

We can specify the minimum size, and when the widget gets its *actual* size enforce that width and height are the same (the maximum of the two, though it's safe to assume that width <= height).
You can implement this by either adding a new actor to the hierarchy which uses a custom layout manager to implement this behavior, or more pragmatically (e.g. less elegantly, but simpler and working just fine) by explicitly setting

  this._box.width = this._box.height;

whenever the height changes (a hint though: if you do that, you'll want to grep for "Meta.later_add" :-)



> let labelSize = (this._label.visible) ?
> this._label.get_theme_node().get_theme_node().get_height() : 0;

No, that won't work (even ignoring the obvious error that St.ThemeNode has no get_theme_node() method) - get_height() will return the value of the "height" CSS property, which is not necessarily the allocated height of the widget. In this case, we don't specify an explicit height in the CSS, so the method will return 0.


> let levelSize = (this._level.actor.visible) ?
> this._level.actor.get_theme_node().get_theme_node().get_height() : 0;

Same here. 


> ... and then calculate the size:
> let size = this._totalSize + labelSize + levelSize - verticalPadding;

this._totalSize / 2 actually, and you'd also need to consider the 'spacing' property (which is the space between children). If you are actually doing all the calculations manually, it'll be easier to make _box a Shell.GenericContainer / custom StWidget instead though.


> this._box.style = 'width: %dpx; height: %dpx;'.format(size, size);

You don't need to set height/width via CSS, use this._box.set_size(size, size) (e.g. "what we have now, but with better values for size")


> > @@ +177,3 @@
> > You still need to update the popup size if possible! To test: open Settings ->
> > Displays and change the resolution. The popup size is expected to adapt to the
> > change.
> 
> Yeah.. so if I just use this._box.style = ... it will trigger the signal for
> 'style-changed' and it will be updated.

Yes, that should work. It's an ugly hack though, so don't do it. You can either use

  this._box.style_changed();

or

  if (this._box.get_stage())
      this._onStyleChanged();

Created attachment 243354 [details] [review]
Popup size changes dinamically

Changes from the previous patch: popup size is always squared, function is connected to this._box and not to the widget's signal, popup size is updated on the signal 'monitors-changed'.

Review of attachment 243354 [details] [review]:

::: js/ui/osdWindow.js
@@ +196,3 @@
+        this._box.style = 'min-width: %dpx; min-height: %dpx;'.format(minWidth, minHeight);
+
+        Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this,

This is wrong - at his point, the style change hasn't affected the allocation yet, and you need to always enforce squareness when the popup's size changes, not only on style changes.

To test: change the resolution to the lowest possible choice, then restart the shell and run from a terminal:

(1) gdbus call --session --dest org.gnome.Shell --object-path /org/gnome/Shell --method org.gnome.Shell.ShowOSD '{"icon": <"media-eject-symbolic">}'

(2) gdbus call --session --dest org.gnome.Shell --object-path /org/gnome/Shell --method org.gnome.Shell.ShowOSD '{"icon": <"media-eject-symbolic">, "label":<"Foo Bar">, "level":<50>}'

Chances are that the 2nd command now overflows the popup ...

(In reply to comment #26)
> Review of attachment 243354 [details] [review]:
> 
> ::: js/ui/osdWindow.js
> @@ +196,3 @@
> +        this._box.style = 'min-width: %dpx; min-height:
> %dpx;'.format(minWidth, minHeight);
> +
> +        Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this,
> 
> This is wrong - at his point, the style change hasn't affected the allocation
> yet, and you need to always enforce squareness when the popup's size changes,
> not only on style changes.
> 
> To test: change the resolution to the lowest possible choice, then restart the
> shell and run from a terminal:
> 
> (1) gdbus call --session --dest org.gnome.Shell --object-path /org/gnome/Shell
> --method org.gnome.Shell.ShowOSD '{"icon": <"media-eject-symbolic">}'
> 
> (2) gdbus call --session --dest org.gnome.Shell --object-path /org/gnome/Shell
> --method org.gnome.Shell.ShowOSD '{"icon": <"media-eject-symbolic">,
> "label":<"Foo Bar">, "level":<50>}'
> 
> Chances are that the 2nd command now overflows the popup ...

Yeah, this is better way to test it and now I can see that it still overflows. 
As far as I can see there is no other signals in St.Widget that I can huck up to and enforce squareness. Can the other way around be to use Shell.GenericContainer()?

(In reply to comment #27)
> As far as I can see there is no other signals in St.Widget that I can huck up
> to and enforce squareness.

There is ClutterActor::allocation-changed - note however that if you want to enforce squareness by using Math.max(width, height) as size, you cannot use this approach, as setting an explicit height prevents the popup from growing dynamically. So you either need to rely on height >= width (which you can, as the icon is square and additional elements are stacked vertically) and enforce the width to be the same as the height (allowing the latter to exceed the min-height) or use an intermediate actor with a custom layout manager. (In the former case you can use the notify::height signal instead of allocation-changed.)


> Can the other way around be to use Shell.GenericContainer()?

Yes, but note that Shell.GenericContainer was introduced to overcome limitations in gjs which prevented us from implementing custom containers in JS; this is no longer the case, so using Shell.GenericContainer in newly written code is discouraged. You can achieve the same by subclassing St.Widget or implementing a layout manager.

Created attachment 243514 [details] [review]
Popup size changes dinamically

Always enforce squareness; height >= width (the "allocation-changed" signal)

Review of attachment 243514 [details] [review]:

Yes! Just one small nitpick left on the code side. Other than that, it's time to write a proper commit message (what's "hello"? ;-)) - see http://lists.cairographics.org/archives/cairo/2008-September/015092.html for some guidance.

::: js/ui/osdWindow.js
@@ +85,3 @@
 
+        this._box.connect('style-changed', Lang.bind(this, this._onStyleChanged));
+        this._box.connect('allocation-changed', Lang.bind(this, this._allocate));

"_allocate" is a misleading name - _onAllocationChanged is better, though I'd connect to notify::height and use _onHeightChanged to make it clear that we are only interested in *height* changes.
Also, as the function is basically a one-liner, you can consider moving it inline:

this._box.connect('notify::height', Lang.bind(this,
    function() {
        Meta.later_add(Meta.LaterType.BEFORE_REDRAW, Lang.bind(this,
            function() {
                this._box.width = this._box.height;
            }));
    }));

Created attachment 243528 [details] [review]
The popup will grow larger to prevent that OSD contents is misaligned vertically

To prevent that OSD contents is misaligned vertically its size will change dynamically. Minimum size is specified and when the widget gets the right size it is enforced that width = height (with an assumption that height >= width).

Created attachment 243532 [details] [review]
osdWindow: Allow popup to grow if necessary

The popup currently has a fixed size based on monitor size. As a result,
the popup's content may overflow if its minimum size is larger than the
popup size. To prevent this, use min-width/min-height for the popup size
so that the popup can grow if necessary.


Thanks, looks good! I'm attaching a tweaked patch, that
 (1) uses a more detailed commit message
 (2) has the following code change:

-        this._box.style = 'min-width: %dpx; min-height: %dpx;'.format(minWidth, minHeight);
+        this._box.style = 'min-height: %dpx;'.format(Math.max(minWidth, minHeight));

As we enforce the width to equal the height, the min-width is irrelevant, so don't set it. For the same reason, if minWidth is bigger than minHeight, we need to use that for min-height.

Attachment 243532 [details] pushed as 8e270dc - osdWindow: Allow popup to grow if necessary

Aaaaand - you are a GNOME contributor!

(In reply to comment #33)
> Attachment 243532 [details] pushed as 8e270dc - osdWindow: Allow popup to grow if
> necessary
> 
> Aaaaand - you are a GNOME contributor!

THANK YOU very much for help Florian. I've learned a lot while working on this small bug. Now it's easier to understand how things work, so I'll continue learning... :D