Open jwijenbergh opened 4 months ago
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.
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?
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.
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 https://github.com/flacjacket/pywlroots/blob/f00de20e060d747b41353bf2e211879af2f140a8/wlroots/wlr_types/output.py#L327.
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
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.
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
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
Maybe we should reactivate the (deprecated) Seat.set_keyboard
then? And either remove the @keyboard.setter
or at least deprecate it?
C.f. https://github.com/flacjacket/pywlroots/issues/137#issuecomment-1978176129
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.
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
Ah, sorry, haven't seen it.
Maybe Seat shouldn't have a keyboard
property at all? Seat.keyboard uses wlr_seat_get_keyboard
. Or is that too nitpicky?
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
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.
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
ormode.setter
inoutput.py