Skip to content
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

Replace Option<T> in Window function return types with T #794

Closed
Osspial opened this issue Feb 13, 2019 · 2 comments · Fixed by #913
Closed

Replace Option<T> in Window function return types with T #794

Osspial opened this issue Feb 13, 2019 · 2 comments · Fixed by #913
Labels
C - needs discussion Direction must be ironed out D - easy Likely easier than most tasks here H - good first issue Ideal for new contributors S - api Design and usability
Milestone

Comments

@Osspial
Copy link
Contributor

Osspial commented Feb 13, 2019

In #434, we decided that an active Window struct ideally should only represent an open window - the only way of closing the window is to drop it. This stance is contrary to several of the Window function APIs that return Option<T> (specifically, get_position, get_inner_position, get_inner_size, and get_outer_size). The docs for those functions say that the functions will return None if the backing window no longer exists, but the API says that the backing window must exist if the Window struct exists.

As such, this issue tracks the removal of Option from those APIs. It also opens the question of what exactly we should do in the unlikely case that the OS does choose to close the window without our consent. As far as I can tell, there are three possible options:

  • Panic
  • Return the last known value
  • Return a stub value

As I'm writing this I don't feel all that strongly about which option we take, but if nobody weighs in I'd be fine with making a choice. Since the Event Loop 2.0 release seems to be our break absolutely everything release I'd support rolling these changes into that update.

@Osspial Osspial added S - api Design and usability C - needs discussion Direction must be ironed out D - easy Likely easier than most tasks here labels Feb 13, 2019
@Osspial Osspial added this to the EventLoop 2.0 milestone Feb 13, 2019
@elinorbgr
Copy link
Contributor

In that case I'd like to raise the question of the platforms where the methods cannot return anything meaningful other than None, typically wayland, where the get_position methods will likely never work (by design of the platform).

Also cc #793 about get_current_monitor whose return type does not really match the behavior we can expect from wayland...

@Osspial
Copy link
Contributor Author

Osspial commented Feb 13, 2019

For functions that the backing platforms potentially don't support, should we add some sort of NotSupportedError type? That way we can also communicate a failure for functions that don't return a meaningful value, like set_position, by having them return Result<(), NotSupportedError>.

Regardless, I'd be fine with having get_position return None or Err on platforms where it's not supported, as long as the documentation reflects that - right now, it's pretty misleading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out D - easy Likely easier than most tasks here H - good first issue Ideal for new contributors S - api Design and usability
Development

Successfully merging a pull request may close this issue.

3 participants