search

awesomeWM / awesome

awesome window manager
https://awesomewm.org/
GNU General Public License v2.0
6.31k stars 596 forks source link

Make client.icon_name writable #3476

Open 3hhh opened 2 years ago

3hhh commented 2 years ago

Output of awesome --version:

4.3

How to reproduce the issue:

E.g.

client.connect_signal("manage", function (c, startup)
    c.icon_name = 'foobar'
    io.stderr:write(string.format('icon_name: %s\n', c.icon_name))
end)

Actual result: Displays the WM_ICON_NAME property in .xsession-errors / the variable stays as it was before.

Expected result:

Changes to foobar at least temporarily.

Notes:

In contrast the c.name variable can be modified, which is useful to change window titles etc. by connecting to the property::name signal.

Unfortunately c.icon_name is used for minimized task list widgets here though, which makes modifying c.name less useful unless one accepts to lose control of the task label when the window becomes minimized.

Aire-One commented 2 years ago

AFAIK, client icon_name property is readonly in our API. (As identified by the issue https://github.com/awesomeWM/awesome/issues/3457, we indeed need to explicitly show this flag in the documentation)

Unfortunately c.icon_name is used for minimized task list widgets here though, which makes modifying c.name less useful unless one accepts to lose control of the task label when the window becomes minimized.

I don't understand (remember?) the rationale behind this check on c.icon_name on minimized task... Maybe your patch can be proposed as a PR?

An alternative fix, that works on the stable version, would be to use the widget_template parameter of the tasklist widget to build your own "task widget" with a custom behavior. You can check the documentation to learn more about the widget_template parameter: https://awesomewm.org/doc/api/classes/awful.widget.tasklist.html

actionless commented 2 years ago

actually client name neither need to be writable if i recall x desktop spec corrrectly (and ~2 years old convo under similar ticket)

3hhh commented 2 years ago

AFAIK, client icon_name property is readonly in our API. (As identified by the issue #3457, we indeed need to explicitly show this flag in the documentation)

Yes, having it documented would definitely help!

Unfortunately c.icon_name is used for minimized task list widgets here though, which makes modifying c.name less useful unless one accepts to lose control of the task label when the window becomes minimized.

I don't understand (remember?) the rationale behind this check on c.icon_name on minimized task... Maybe your patch can be proposed as a PR?

Possibly the hope is that c.icon_name is shorter and thus using less space when minimzed? I don't know...

I can easily send a PR, if it makes sense for other users as well.

An alternative fix, that works on the stable version, would be to use the widget_template parameter of the tasklist widget to build your own "task widget" with a custom behavior. You can check the documentation to learn more about the widget_template parameter: https://awesomewm.org/doc/api/classes/awful.widget.tasklist.html

Yes, I noticed that. However listening on property::name events seemed more robust for Qubes OS from my point of view as any other piece of code attempting to overwrite the added prefix in c.name will cause an infinite loop which will undoubtedly be noticed as bug by the user.

Also it's less code and works with title bars, the tasklist menu and whatever else a user might build from c.name as well.

actionless commented 2 years ago

https://tronche.com/gui/x/icccm/sec-4.html#WM_ICON_NAME

actionless commented 2 years ago

@Aire-One

AFAIK, client icon_name property is readonly in our API.

actually after reading the spec for both WM_NAME and WM_ICON_NAME i dont see a reason why one of them is readonly and other is not - i think we should do either both readonly or both r/w

actionless commented 2 years ago

i think the only reason icon_name is readonly is because there is not setter defined for it in client.c

(you could use luaA_client_set_name(lua_State *L, client_t *c) as an example)

Aire-One commented 2 years ago

actually after reading the spec for both WM_NAME and WM_ICON_NAME i dont see a reason why one of them is readonly and other is not - i think we should do either both readonly or both r/w

I'm honestly not sure why too... This code was written long before I even heard about awesome, and I'm not familiar enough with X11 spec to question these implementations. 🤷‍♂️

i think the only reason icon_name is readonly is because there is not setter defined for it in client.c

This is how I also came to the conclusion this property is read only.

actionless commented 2 years ago

ok, then i leaving this as a feature request

Elv13 commented 2 years ago

There's some news related to this. At least for now, there is a [read only] label in the doc https://awesomewm.org/apidoc/core_components/client.html

bijanbina commented 2 years ago

There's some news related to this. At least for now, there is a [read only] label in the doc https://awesomewm.org/apidoc/core_components/client.html

@Elv13 whats the difference between apidoc and doc? Because readonly tag doesn't shown up in there. Sorry to ask amateur question.

Aire-One commented 2 years ago

There's some news related to this. At least for now, there is a [read only] label in the doc https://awesomewm.org/apidoc/core_components/client.html

@Elv13 whats the difference between apidoc and doc? Because readonly tag doesn't shown up in there. Sorry to ask amateur question.

The doc at https://awesomewm.org/doc/api/index.html targets the latest release of Awesome, aka the version 4.3.

The doc at https://awesomewm.org/apidoc/index.html is automatically generated from this GitHub repository at every new commit on master (including merge commits from PRs), so it's a moving target always up to date with the newest developments.

sclu1034 commented 2 years ago

actually after reading the spec for both WM_NAME and WM_ICON_NAME i dont see a reason why one of them is readonly and other is not - i think we should do either both readonly or both r/w

I'm honestly not sure why too... This code was written long before I even heard about awesome, and I'm not familiar enough with X11 spec to question these implementations. 🤷‍♂️

i think the only reason icon_name is readonly is because there is not setter defined for it in client.c

This is how I also came to the conclusion this property is read only.

On the topic of spec compliance:

The window manager will not change properties written by the client.

The fact that WM_NAME is writable is the one that violates the spec here.

3hhh commented 2 years ago

On 7/4/22 14:41, Lucas Schwiderski wrote:

On the topic of spec compliance:

The window manager will not change properties written by the client.

The fact that WM_NAME is writable is the one that violates the spec here.

Well, I guess that could be fixed in a backwards compatible way by providing the original value written by the client back to the client in case it should ever read it and use the other one inside the window manager. That's beyond my C skills at the moment though.

I'd guess that clients usually never forget their window titles though and therefore it didn't have any impact so far. Plus whoever uses it likely doesn't care too much. What bad could exactly happen?

sclu1034 commented 2 years ago

Well, I guess that could be fixed in a backwards compatible way by providing the original value written by the client back to the client in case it should ever read it and use the other one inside the window manager.

That'd just be "don't call the function that updates the X property". But then you'd be in the rabbit hole of who gets priority when the client changes the value again at runtime. And for that, there is no single correct choice Awesome could make. It's much easier for people to just set a new field (e.g. c.my_custom_name) for the clients they need it for, and replace any usage of c.name with c.my_custom_name or c.name, because then the user is in full control over synchronizing values. And that approach is possible already.

I'd guess that clients usually never forget their window titles though

On the contrary, why would they have to retain an extra copy of that value internally if the X11 spec allows them to assume full control over the X property?

3hhh commented 2 years ago

That'd just be "don't call the function that updates the X property". But then you'd be in the rabbit hole of who gets priority when the client changes the value again at runtime. And for that, there is no single correct choice Awesome could make.

The client would win. Anyone in the WM can listen to changes via signals and change it again as needed until it's stable. I do it that way.

It's much easier for people to just set a new field (e.g. c.my_custom_name) for the clients they need it for, and replace any usage of c.name with c.my_custom_name or c.name, because then the user is in full control over synchronizing values. And that approach is possible already.

Yes, but it would require a lot of patches to awesome wherever awesome uses c.name internally. I'm attempting to get rid of downstream patches, not add more. Or did I get this wrong?

On the contrary, why would they have to retain an extra copy of that value internally if the X11 spec allows them to assume full control over the X property?

Compile-time constants from my experience. Some clients may add some dynamic suffixes (time, link speeds, ...).

By the way another viable alternative might be to just document that whoever writes to client.name should be aware that s/he is stepping outside of the X11 spec and thus clients may misbehave ("use at your own risk").

actionless commented 1 year ago 
    https://x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#client_properties

The fact that WM_NAME is writable is the one that violates the spec here.

could you make an exact quote which would say so? i found only "window manager shouldn't hide WM_NAME until user EXPLICITLY did so"

sclu1034 commented 1 year ago 
    https://x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#client_properties

The fact that WM_NAME is writable is the one that violates the spec here.

could you make an exact quote which would say so? i found only "window manager shouldn't hide WM_NAME until user EXPLICITLY did so"

That's already in the original message. 3hhh left out part of my statement.

actionless commented 1 year ago

i don't understand that you mean, just copy the quote which would be Ctrl+F -able in the spec

sclu1034 commented 1 year ago

As I said, that quote is right there in my message from 4 months ago, the one you quoted only partially: https://github.com/awesomeWM/awesome/issues/3476#issuecomment-1173775145

The window manager will not change properties written by the client.

actionless commented 1 year ago

then it's a problem not with just client.icon_name property - but more general - if property was already set by the client, WM shouldn't attempt to change them.

whilst current logic doesn't seem to take that into account at all

also in case when client didn't set icon_name hint itself - it should be writeable by wm, and we don't have a setter for that, as was mentioned above