MestreLion / ewmh-client

EWMH (Extended Window Manager Hints) Client API
GNU General Public License v3.0
2 stars 0 forks source link

some ideas on set_property() method (perhaps extendable to all other methods) #2

Open Kalmat opened 1 year ago

Kalmat commented 1 year ago

Taking a look to set_property() overload, some questions and ideas assaulted me (I think they can be exportable to other methods):

The main idea is to encapsulate all requests as much as possible, so the user doesn't have to deal with complex issues like "data" content.

Furthermore, it's not straight-thru to know what to use in every case.

For instance, to minimize a window, you should use: window.set_property(WM_CHANGE_STATE, [Xlib.Xutil.IconicState, 0, 0, 0, 0]) whilst to maximize a window (which intutively should be similar), you have to use this: window.set_wm_state(ACTION_SET, STATE_MAX_VERT, STATE_MAX_HORZ)

Another example is ewmh/python-xlib get_attributes() method, by which I get a different struct than using X11 library's XGetWindowAttributes()...

What I mean is to design something that can abstract the user as much as possible from the inherent complexity of xlib... what do you think?

Sorry again if this is too long or too... silly! HAHAHA!

EDIT: After a closer look, I know understand the meaning of data_format (it's not data size as a I erroneously understood). I think this could be, again, encapsulated (self-assumed) for common resquests, and then perhaps leaving this field in a new, more generalistic function suitable to more advanced users or for marginal/specialized requests (not common or inevitably complex, I mean)

Kalmat commented 1 year ago

Taking a closer look to the set_property() implementation, I see some differences compared to my own implementation (I have no idea about which one is better and why, to be honest... something to investigate, I guess). Look:

This is your implementation:

def set_property(
    self,
    name: AtomLike,
    data: PropertyValue,
    property_type: AtomLike,
    data_format: Format = Format.INT,
    mode: Mode = Mode.REPLACE,
    onerror: t.Optional[XErrorHandler[_T]] = None,
    immediate: bool = True,
) -> None:
    if isinstance(data, bytes) and data_format != Format.CHAR:
        raise EwmhError(
            "Format mismatch for bytes data: expected %s, got %s",
            Format.CHAR,
            Format(data_format),
        )

    # FIXME: provisional, for testing
    def error_handler(*args: object, **kwargs: object) -> None:
        raise EwmhError("Error in set_property(%s): %s", params, locals())

    params = locals().copy()
    self.handle.change_property(
        property=self.Atom(name),
        property_type=self.Atom(property_type),
        format=data_format,
        data=data,
        mode=mode,
        onerror=onerror or error_handler,
    )
    if immediate:
        self.display.flush()

This is how I did it (it works but, again, not sure which is better or when should one or the other be used, etc.):

def setProperty(self, prop: str | int, data: list[int]):

    if isinstance(prop, str):
        prop = self.display.get_atom(prop)

    if type(data) is str:
        dataSize = 8
    else:
        data = (data + [0] * (5 - len(data)))[:5]
        dataSize = 32

    ev = Xlib.protocol.event.ClientMessage(window=self.win, client_type=prop, data=(dataSize, data))
    mask = Xlib.X.SubstructureRedirectMask | Xlib.X.SubstructureNotifyMask
    self.display.send_event(destination=self.rid, event=ev, event_mask=mask)
    self.display.flush()

The main difference it's that you are using a drawable() method (change_property()), while I am sending an event to the root window (self.rid)

MestreLion commented 1 year ago

Taking a look to set_property() overload, some questions and ideas assaulted me (I think they can be exportable to other methods):

  • set_property
    • name: Union[Atom, int, str], -> Accept only pre-defined Atoms?

Are you talking about "which atoms should it accept" or "what type should name have"?

About the former, do you mean by "pre-defined" atom?

Some considerations:

Side note: python-xlib does atom caching to avoid redundant X Server requests.


But... if you are talking about "what should an atom argument be typed as", and suggesting it should restrict to Atom instances.. well, tough call:

I'm considering using Atom (my class) everywhere, including in property results such as _NET_SUPPORTED. So instead of:

>>> root.get_supported()
[482, 442, 369, 307, 424, 423, 314, 425, 426, ...]

You would get:

>>> root.get_supported()
[<Atom(482) _NET_WM_FULL_PLACEMENT>, <Atom(442) _NET_SUPPORTED>, <Atom(369) _NET_SUPPORTING_WM_CHECK>, ...]

But... should this extend to the more general, lower-level (and non-EWMH exclusive) root.get_property() too? Should root.get_property("_NET_SUPPORTED") also yield Atom instances? Should it "Atomicize" any results of type Xlib.Xatom.ATOM?

MestreLion commented 1 year ago

The main difference it's that you are using a drawable() method (change_property()), while I am sending an event to the root window (self.rid)

It took me a while to understand this point in the EWMH, as this is nowhere said explicitly: There is no such thing as an universal high-level "set property" concept in EWMH

In EWMH, some abstract properties should be changed directly, as in "using X Protocol's ChangeProperty method" (the case with _NET_WM_NAME), while some should be changed indirectly, by sending a client message event to the root window, as the case with _NET_ACTIVE_WINDOW.

That's why in my implementation, some of mine Window.set_*() call XWindow.send_event() and some others call XWindow.ChangeProperty(). That's in the EWMH spec. Btw, there's no SetProperty in X. Or in EWMH.

MestreLion commented 1 year ago

In X there's only events and window properties. And for X, a window property is just a bunch of well-defined structured data you attach to a window. It has no meaning or semantics. X provides a mechanism for manipulating that data: ChangeProperty, DeleteProperty, and so on. (no Set or Create, both use Change in REPLACE mode). Events are, for X, somewhat unrelated to this chunk of window data we call "property".

For X, a window property has/is:

Now comes ICCCM spec to standardize these chunks of named data. It says Window Managers should use a property named WM_NAME for the string they show in the title bar (title bar or even title is an alien concept for X. Windows have no title in X). ICCCM says _"use the atom WM_NAME for the property name, use the atom STRING as its type, use an 8-bit data format, and interpret each data item as a character"_. Again, X have no semantics for the atom named STRING

And EWMH is an extension of ICCCM, it simply defines more properties. It says: _"Window Managers, please use a property named _NET_ACTIVE_WINDOW to store the X Window ID of the currently active window as a single, 32-bit item in its data array. And let's use an atom named CARDINAL as its type"_. Again, X itself has no notion of active window, or CARDINAL.

And the EWMH spec also says: if anyone wants to change the active window, do not set that property's value! Instead, use X's event mechanism that a WM will be listening to, and let the WM change the property value. As for _NET_WM_NAME, well go ahead and use X to directly change the value with ChangeProperty. WM's will also (somehow) be notified of that and change the displayed title.

MestreLion commented 1 year ago

The main idea is to encapsulate all requests as much as possible, so the user doesn't have to deal with complex issues like "data" content.

We're in the same page here

Furthermore, it's not straight-thru to know what to use in every case.

That's why I'm planning to use a get/set simple but consistent convention:

Window.set_xxx_yyy() means _"set the (abstract) property _NET_XXX_YYY in whatever way the EWMH spec mandates"_

It will call either Window.set_property (a wrapper to the underlying Window.handle.change_property()) or Window.send_message() (a wrapper to Window.handle.send_event())

For instance, to minimize a window, you should use: window.set_property(WM_CHANGE_STATE, [Xlib.Xutil.IconicState, 0, 0, 0, 0]) whilst to maximize a window (which intutively should be similar), you have to use this: window.set_wm_state(ACTION_SET, STATE_MAX_VERT, STATE_MAX_HORZ)

Another example is ewmh/python-xlib get_attributes() method, by which I get a different struct than using X11 library's XGetWindowAttributes()...

What I mean is to design something that can abstract the user as much as possible from the inherent complexity of xlib... what do you think?

I haven't arrived in implementing those methods yet. Possibly the distinction between "an abstract property" and "an action for the WM" will become blurry, so I'm not sure what would be the best approaches for those.

EDIT: After a closer look, I know understand the meaning of data_format (it's not data size as a I erroneously understood). I think this could be, again, encapsulated (self-assumed) for common resquests, and then perhaps leaving this field in a new, more generalistic function suitable to more advanced users or for marginal/specialized requests (not common or inevitably complex, I mean)

All Window.set_xxx_xxx() methods already encapsulate the format (and the type too). They're the high-level methods. When I'm done implementing all of them, one should not need to use Window.set/get_property() (or their _text_ counterparts) or send_message to be able to access all of EWMH's features

MestreLion commented 1 year ago
  • data: Union[bytes, Sequence[int]], -> Wrap (set them internally, I mean) these values for common props/atoms/requests when these values are fixed or do not add any value?
  • property_type: Union[Atom, int, str], -> Wrap in a different function for common types (e.g. WINDOW_STATE, WINDOW_TYPE, HINTS, WM_PROTOCOLS?)
    • data_format: Format = Format.INT, -> Normally 8 (if str) / 32 (otherwise)... Can this also be set internally?

Could you elaborate on those? I mean, what would my Window.set_property() signature be like, what what enforcements/heiritcs would it make?

What I know so far is:

This all depends on whether to keep set_property() as a general utility method or not. Remember it's meant to be low-level. For high-level, there is (or will be) a set_xxx method for each abstract property in EWMH.

  • mode: Mode = Mode.REPLACE, -> Is this suitable for all properties? (if not, maybe it could be used in some separate functions, when it applies. Perhaps I'm wrong, but I only used them in WM_WINDOW_TYPE requests)

This is an X feature, user can choose if they want to append or prepend a value to a property instead of fully replacing it. EWMH does not dictate this either. My own set_xxx() property wrappers will always use REPLACE, so you must pass the full data. Should the low-level set_property() require this too?

  * onerror: Optional[Any] = None,

It's temporary until I figure out exacly how the low level works. Error handling in X is tricky, and Xlib is funky, it almost never raises exceptions, I'm not sure how I should deal with it.

  * immediate: bool = True) -> None       -> flush() is usually fast, is a non-immediate call necessary? Is there any difference with sync() we shoul take into account?

Not sure what sync() is, but yes, I'll prolly drop immediate and always issue flush() unconditionally.

Furthermore some actions need some time for the WM to be refreshed, not sure if we should take this into account as well.In PywinCtl I introduced the param "wait" for that.

For the high-level properties that the EWMH says we should directly change the underlying low-level property there's no need of wait, a get_property will be updated after flush. For those requiring send event it's up for how responsive the WM is in changing the corresponding low-level property

EDIT: After a closer look, I know understand the meaning of data_format (it's not data size as a I erroneously understood). I think this could be, again, encapsulated (self-assumed) for common resquests, and then perhaps leaving this field in a new, more generalistic function suitable to more advanced users or for marginal/specialized requests (not common or inevitably complex, I mean)

Maybe I'll split set_property in 2: a really general, low level one, with all features allowed by X/Xlib, and a somewhat limited (but still low-level) one for the sole purpose of EWMH properties. But... where to draw the line? Should this "mid" tier also be able to accomodate for ICCCM's properties?

Kalmat commented 1 year ago

About the former, do you mean by "pre-defined" atom?

  • Just the ones in Xlib.Xatom: formally those are the "pre-defined" ones, as the X Standard guarantees their value-name pair in any Display Server. It's a small, basic list, meant for constants, EWMH uses a few but it does not contain any of the atoms defined by it (_NET_*), or by ICCCM for that matter, so no WM_NAME either.
  • All defined as property names by the EWMC. Should it include non-EWMH, ICCCM properties too? so ewmh.get_property() can be used to retrieve ICCCM's WM_NAME?
  • All used by the EWMH spec, regardless if defined in EWMH or ICCCM. Should include non-property names too? (currently I don't have neither of these sets enumerated. but I'm considering to create one)
  • All pre-existing atoms in the X Display Server: basically only refusing to create new atoms. That's the most inclusive notion of "pre-defined" one can have, and that's the one I'm using. Notice I'm using display.get_atom(..., create=False) by default, so it will raise an exception for new atoms.

ewmh separates change_properties(), change_attributes and set_wm_state(). What I mean is to separate those properties categories, having a set of pre-defined values, and a separate function for custom ones. For instance (if it's not clear, just let me know, please), I have these states values:

WM_CHANGE_STATE = 'WM_CHANGE_STATE'
WM_STATE = '_NET_WM_STATE'
STATE_MODAL = '_NET_WM_STATE_MODAL'
STATE_STICKY = '_NET_WM_STATE_STICKY'
STATE_MAX_VERT = '_NET_WM_STATE_MAXIMIZED_VERT'
STATE_MAX_HORZ = '_NET_WM_STATE_MAXIMIZED_HORZ'
STATE_SHADED = '_NET_WM_STATE_SHADED'
STATE_SKIP_TASKBAR = '_NET_WM_STATE_SKIP_TASKBAR'
STATE_SKIP_PAGER = '_NET_WM_STATE_SKIP_PAGER'
STATE_HIDDEN = '_NET_WM_STATE_HIDDEN'
STATE_FULLSCREEN = '_NET_WM_STATE_FULLSCREEN'
STATE_ABOVE = '_NET_WM_STATE_ABOVE'
STATE_BELOW = '_NET_WM_STATE_BELOW'
STATE_ATTENTION = '_NET_WM_STATE_DEMANDS_ATTENTION'
STATE_FOCUSED = '_NET_WM_STATE_FOCUSED'
STATE_NULL = 0

The values are known for the WM, but not for the dev, they are long to write, and you can easily write them wrong. So we could have a constants submodule (like Xlib.X, but not all together without any organization as it is know), so I could write this (and only this:

window.set_wm_state(state=ewmh-client.STATES.Modal)

or

ewm-client.wm_state(window, state=ewmh-client.STATES.Modal)

Notice I didn't use an EWMH() class, just a "general" function, passing the window handle.

The available values would be automatically presented by any IDE when I write ewmh-client.STATES. Furthermore, "WM_CHANGE_STATE" prop is irrelevant, since we could set it internally because set_wm_state is intended only for that.

We could also add as many similar pre-defined functions (like set_wm_window_type() which is not present in ewmh, don't know why, hints, normal_hints, protocols... everything we can "standardize").

In these functions, I would only accept these values, not str and/or int. So I could define:

ewmh-client.STATES.FullScreen = 5

and then

constants = {
    ...
    5: "_NET_WM_STATE_FULLSCREEN"
    ...
}

Then internally, the actual atom would be calculated as:

display.get_intern_atom(constants[state], True/False)

So this all remains managed internally, and the user can see/needs only: ewmh-client.STATES.FullScreen

In case one of the properties/attributes/types/values in general is not allowed (or doesn't exist), we could return the corresponding return code or raise an exception (this is a separate discussion, I know). For other properties, as you so rightly point), we could use a totally-free, separate function (for those who know what they are doing), something like:

set_custom_property(property, property_type, format, data... and other required/complex params)

Notice that in this last case we are not adding much "value", since the user must introduce almost the same data than if he was using xlib directly (we can, nevertheless, abstract from ctypes complexity as well managing pointers, structures and similar things, and now not talking about set_property() but some other and more complex functions, like XGetWindowAttributes()).

Kalmat commented 1 year ago

Not sure what sync() is, but yes, I'll prolly drop immediate and always issue flush() unconditionally.

Furthermore some actions need some time for the WM to be refreshed, not sure if we should take this into account as well.In PywinCtl I introduced the param "wait" for that.

For the high-level properties that the EWMH says we should directly change the underlying low-level property there's no need of wait, a get_property will be updated after flush. For those requiring send event it's up for how responsive the WM is in changing the corresponding low-level property

EDIT: After a closer look, I know understand the meaning of data_format (it's not data size as a I erroneously understood). I think this could be, again, encapsulated (self-assumed) for common resquests, and then perhaps leaving this field in a new, more generalistic function suitable to more advanced users or for marginal/specialized requests (not common or inevitably complex, I mean)

In my experience, it's not so immediate for the WM to update everything. Perhaps it's because I'm using a Virtual Machine... quite possible. As far as I could read, sync() send the order to apply change and waits until changes have been made, whilst flush() sends the order to apply changes, but doesn't wait (I might be wrong here... well, everywhere, hahahaha!).