Created attachment 165330 [details] [review]
[PopupImageMenuItem] Support setting the image after construction

This patch allows the icon in PopupImageMenuItems to be changed at
any time, while preserving the item and all other properties (style,
signals) attached and without rebuilding the whole menu. This is useful
for images reflecting a dynamic status (e.g. cellular strength
indicator or battery level)

Comment on attachment 165330 [details] [review]
[PopupImageMenuItem] Support setting the image after construction

>-        box.add(new St.Label({ text: text }), { expand: true });
>+        this.label = new St.Label({ text: text });
>+        box.add(this.label, { expand: true });

what is this change for? If we also want to be able to update the label, then there should be a setter for that, like with setIcon. And if we don't, then the change is unnecessary?

(In reply to comment #2)
> (From update of attachment 165330 [details] [review])
> >-        box.add(new St.Label({ text: text }), { expand: true });
> >+        this.label = new St.Label({ text: text });
> >+        box.add(this.label, { expand: true });
> 
> what is this change for? If we also want to be able to update the label, then
> there should be a setter for that, like with setIcon. And if we don't, then the
> change is unnecessary?

The icon cannot be a normal property, as the actor needs to be recreated every time, so you need a setter, either explicit or implicit (and implicit setters conflict with GJS style guide), while the label can be exposed directly (this is from discussion of bug 621356)

Any progress of this? Should I prepare an updated patch that does not make .label public?

i guess it's ok as is