Inconsistencies with setters and getters

[jwijenbergh]

Inconsistencies with setters and getters

[heuer]

Ah, sorry, haven't seen it.

[heuer]

Good point.

I'd prefer properties unless we want to imply "costs" through a function call.

Presumably the current approach is that there is a property if there is a wlr_X_get_Y, /wlr_X_set_Y function. For example, see wlr_seat_get_keyboard / wlr_seat_set_keyboard, these functions are represented by the property Seat.keyboard.

We can consider whether it makes sense to deviate from this scheme. For example, the Seat class has a method set_name, but does not provide an API to get the name.

[jwijenbergh]

I actually like the set_ approach as it implies that we are calling a function. In regular python APIs property setters make sense as you basically just use it to set a private variable (e.g. self._var). Class.var = b would really hide we're crossing the C barrier.

A large portion of the getters in this codebase are just for getting individual struct fields using the C pointer. I think that is fine to have a normal property getter as it does not involve a function call.

I was going to say that we should only use @property and @theproperty.setter for methods that do not involve function calls, whereas ones that do should have get_ and set_ defined, but maybe that is awkward. Thoughts?

[heuer]

One question I have is whether it makes sense from a Python developer's perspective to know whether a struct member is being accessed or a function is being called. Ultimately, users know that pywlroots wraps a C-API.

An exception should be functions that are expensive, for example require some calculations. This should not be hidden.

From a Python developer's perspective, it may seem strange to be able to read a property but have to call a function to change the property

current_name = seat.name  # Access struct member
seat.set_name('changed_name')

It seems feel more natural to use

current_name = seat.name
seat.name = 'changed name'

As written, I have a tendency to use properties for both read and write access, but I would also be open to any other solution as long as it is implemented consistently in the API.

[jwijenbergh]

Good point. I think that indeed consistency is important. I am not the biggest fan of properties in general because of the fact that it indeed hides too much. The other part is that if we have a setter that takes multiple arguments, we have to create named tuples for each of them. See e.g. CustomMode in output

pywlroots/wlroots/wlr_types/output.py

Line 327 in f00de20

class CustomMode(NamedTuple):

.

En-fin, maybe I am being too harsh on them and I should just accept that it's standard in python to use properties. You make a good point in the sense that properties are super natural for a python developer. I guess it depends if we want to make pywlroots more high-level/python like or have it more closely mimic wlroots. Do you have an opinion on this @flacjacket.

Btw I am willing to go for the solution anyone of you think is best, as it's not a big deal to convert qtile to any of the proposed APIs

[heuer]

As written, I have a tendency to use properties for both read and write access, but I would also be open to any other solution as long as it is implemented consistently in the API.

Aha! Now I see where you're coming from. OutputState vs Output API. Yes, I agree, there is a difference in the API which should be aligned. The OutputState was recently added and I applied a mindset on it w/o changing the Output API because I don't wanted to introduce too much changes in a minor release.

Anyway, as @jwijenbergh, I am open to any solution and consistency should matter.

[jwijenbergh]

As written, I have a tendency to use properties for both read and write access, but I would also be open to any other solution as long as it is implemented consistently in the API.

Right! That's clear.

Aha! Now I see where you're coming from. OutputState vs Output API. Yes, I agree, there is a difference in the API which should be aligned. The OutputState was recently added and I applied a mindset on it w/o changing the Output API because I don't wanted to introduce too much changes in a minor release.

Anyway, as @jwijenbergh, I am open to any solution and consistency should matter.

Exactly, sorry for not explaining it properly. This was indeed the reason I brought up the point because initially I thought something went wrong with rebasing. I will see what I will do in the wlr 0.17, I have some time again in a few days

[jwijenbergh]

I have reverted back to using explicit setters (set_) when a function is called under the hood. This seems like this was the intention in the codebase as the only setters made by @m-col col seem to be only from direct pointer manipulation (self._ptr.val = val). A bit unrelated but these were @m-col's though for the qtile codebase at least:

it would be best with explicit getters and setters
They communicate to the caller of the method that a function is being called
Whereas a property intentionally tries to deceive the caller
Explicit > implicit and all
You could follow the way window.place works
Could make a Configurable with all fx options and pass that to the one method and then its a super simple api
Only 1 method to remember

If we want to use property setters everywhere I think it's best that we do it everywhere then, but as converting everything to set_ (for ones that call functions) was less work, I went with this route

[heuer]

Maybe we should reactivate the (deprecated) Seat.set_keyboard then? And either remove the @keyboard.setter or at least deprecate it?

C.f. #137 (comment)

Probably it is okay to just remove the @keyboard.setter since the change was made about a month ago and I guess we don't have that many installations yet.

[jwijenbergh]

Maybe we should reactivate the (deprecated) Seat.set_keyboard then? And either remove the @keyboard.setter or at least deprecate it?

Exactly! I did that already in the wlr 0.17 branch

Probably it is okay to just remove the @keyboard.setter since the change was made about a month ago and I guess we don't have that many installations yet.

I have also removed this setter

[heuer]

Maybe Seat shouldn't have a keyboard property at all? Seat.keyboard uses wlr_seat_get_keyboard. Or is that too nitpicky?

[jwijenbergh]

Done! Thanks

What about this one? https://github.com/jwijenbergh/pywlroots/blob/575a8168286d4f4f08b2013d8ceab49c151a10e0/wlroots/wlr_types/keyboard.py#L154-L160. Maybe get_modifier but that is awkward as the API is get_modifiers, get_modifiers is awkward too because we already have a modifiers property

[heuer]

I think we have to leave it as it is now that 0.17 has been released. API changes in a minor release are somewhat unfortunate.

I would have liked to tweak some rough edges, but that will have to wait until 0.18 I guess.