-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wlr 0.17 #167
Wlr 0.17 #167
Conversation
@flacjacket if you have the time can you approve CI on this |
This is great, thank you @jwijenbergh and @m-col! I'd love to see pywlroots 0.17.0. |
0b54ba7
to
01201f8
Compare
Hi @flacjacket A polite nudge on this. Would be great if we can move onto 0.17 support soon. Thanks! |
Thank you for this! I missed that this was a PR and not an issue noting what is needed to get to 0.17. I'll take a look at this this weekend, if it is possible to update the conflicts we can work on getting this merged in! |
Thanks! Will work on the conflicts and CI later today |
OutputState used property setters, whereas Output uses set_. In this codebase, it seems like there is a preference for explicit set_ functions as there is little use of these setters
layout changes. If the output is already in the layout, it will become | ||
auto configured. If the position of the output is set such as with | ||
`wlr_output_layout_move()`, the output will become manually configured. | ||
def add_auto(self, output: Output) -> OutputLayoutOutput | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I like the return value here. Would be better if pywlroots raises an exception if it runs out of memory. Other functions do this as well in this rare case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm I am not sure I like the other way around, in my mind I prefer to have everything closer to wlroots API so it's easier to upgrade to a new wlroots. There are still a lot of APIs that do Class | None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of memory should be a very rare error. However, if the return value can also be None
, we impose the handling of this rare error on everyone using pywlroots.
It is true that we have functions whose return value can also be None, but this is usually not an indicator of an error, but rather that a value was not set.
I'm not a fan of exceptions, in fact I've already made a few suggestions about replacing them with appropriate return values, but I think an exception makes sense here.
@@ -75,16 +75,16 @@ def output_at(self, x: float, y: float) -> Output | None: | |||
return None | |||
return Output(output_ptr) | |||
|
|||
def add(self, output: Output, lx: int, ly: int) -> None: | |||
def add(self, output: Output, lx: int, ly: int) -> OutputLayoutOutput | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment regarding add_auto
Thanks for pushing to make this happen! |
supersedes #129, testing CI here. totally not done yet, currently I just added the fractional scale API. Need to see what is missing here