Inconsistencies with setters and getters

[jwijenbergh]

Some properties use set_ whereas other use @theproperty.setter. What should we decide on? Same holds for getters, some use @property, some don't.

cc @heuer

let's fix this in the wlr 0.17 branch

good example: set_mode or mode.setter in output.py

[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]

Ah, sorry, haven't seen it.

[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.

[heuer]

I'll come back to this problem again.

It still feels a bit unfortunate to me.

We're talking about a Python API that could be superior to a C API at best.

Why do we accept implicit costs in getters by initializing Python objects, away from native objects like integers and booleans etc., while we have to explicitly specify the costs when setting the value?

It doesn't feel very natural, at least to me, to be able to do an
obj.enabled but not obj.enabled = True, but having to resort to obj.set_enabled(True).