arminfriedl / xcb-wm

Rust implementation of xcb-wm - icccm and ewmh extensions for xcb
MIT License
5 stars 3 forks source link

Support for _NET_WM_CM_Sn #4

Open vikigenius opened 2 years ago

vikigenius commented 2 years ago

I took a look at the code and it's very well written and easy to understand, so thanks for this.

I looked at the Atoms and you have 2 TODOs there that are interesting

https://github.com/arminfriedl/xcb-wm/blob/f61f0b4b2fd4e0a7fae19e283bba3ceff2151c23/src/ewmh/atoms.rs#L3

and

https://github.com/arminfriedl/xcb-wm/blob/f61f0b4b2fd4e0a7fae19e283bba3ceff2151c23/src/ewmh/atoms.rs#L97

So these two seem a bit conflicting I was wondering if there was a way to do both?

I say they are conflicting because the xcb::atoms_struct is a macro and would consider every field as a single atom. But to use _NET_WM_CM_Sn you would need an array of atoms, so this would not be easily included in the macro struct.

I was wondering if you have any thoughts on this?

arminfriedl commented 2 years ago

Thank you, that's very kind! Some parts could still use quite some love unfortunately (particularly the macros :cold_sweat:).

The reason I wanted to explore the xcb::atom_struct!{} is because it basically does what I'm doing (quite manually) in the atoms.rs. I think there's a chance we could remove quite some code there and make it more readable by just using the atom_struct!{}.

Regarding the _NET_WM_CM_Sn: This is indeed the most complicated atom because it is dynamic (depends on the number of screens). However, be aware that at the end of the day - for X11 - this is not an array. The atoms that need to be interned are e.g. _NET_WM_CM_S0, _NET_WM_CM_S1, _NET_WM_CM_S2, etc.

I feel like we will need to handle _NET_WM_CM_Sn in a special way. That's also what lbxcb-wm (the C implementation of xcb-wm) ended up doing. I'm open for better solutions, but I think adding the static atoms via xcb::atom_struct!{} (if possible) and then dynamically adding the _NET_WM_CM_Sn afterwards would be a good way to go here.