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

Refactor errors to be more specific #3067

Closed
wants to merge 11 commits into from
Closed

Refactor errors to be more specific #3067

wants to merge 11 commits into from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Aug 30, 2023

Move errors out of the generic error module, and into the module where they're used.

This should help the user with actually handling the errors (instead of just propagating them), and with seeing what kind of errors a method can return (more work is still needed in this area, but this is a first step towards that).

The public changes are:

  • error::ExternalError::Ignored -> event::RequestIgnored.
  • error::EventLoopError is split into:
    • event_loop::EventLoopCreationError (with less variants).
    • event_loop::EventLoopRunError (with less variants).
  • platform::startup_notify::EventLoopExtStartupNotify::request_activation_token now errors with platform::startup_notify::ActivationTokenNotFound instead of error::NotSupportedError.
  • error::ExternalError -> window::WindowError.
  • error::NotSupportedError -> window::NotSupportedError.

A few documents that guide decision making here:

Checklist:

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
    • Unsure how much information is relevant here.
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Builds upon #3066, DO NOT MERGE YET!

@madsmtm madsmtm added S - api Design and usability C - needs discussion Direction must be ironed out labels Aug 30, 2023
@madsmtm madsmtm marked this pull request as ready for review August 30, 2023 12:01
@madsmtm madsmtm requested review from notgull and maroider and removed request for msiglreith and jackpot51 August 30, 2023 12:01
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also a good idea to extract the "could not find any backends" error in linux/mod.rs into its own variant.

src/event_loop.rs Outdated Show resolved Hide resolved
src/platform/startup_notify.rs Show resolved Hide resolved
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely love this change, it's a great improvement imo!

Only reviewed the cross-platform stuff and Web, I think Web errors in general need an overhaul so I'm gonna leave things as they are.

src/event.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
@madsmtm
Copy link
Member Author

madsmtm commented Sep 2, 2023

It's also a good idea to extract the "could not find any backends" error in linux/mod.rs into its own variant.

Fully agreed, ideally I'd get rid of the generic OsError, but we have to start somewhere.

@@ -1,142 +1,36 @@
use std::panic::Location;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of keeping this module then in the end? The OsError sounds like it should be in the platform module, since it depends on the platform. We could maybe name it differently, like PlatformError.

}
}

impl std::error::Error for RequestIgnored {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think nothing stops from doing a proper import at the top.

Comment on lines +1071 to +1083
/// The request to change the inner size synchronously was ignored.
///
/// See [`InnerSizeWriter::request_inner_size`] for details.
#[derive(Debug)] // Explicitly not other traits, in case we want to extend it in the future
pub struct RequestIgnored {
_priv: (),
}

impl fmt::Display for RequestIgnored {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
f.write_str("the request was ignored")
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason this error is here and not on the window for example?

Comment on lines +466 to +467
/// Application has exit with an error status.
ExitFailure(i32),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely gone with the recent rework.

Comment on lines +37 to +43
/// The error type for when the requested operation is not supported by the backend.
#[derive(Debug)]
pub struct ActivationTokenNotFound {
_marker: (),
}

impl fmt::Display for ActivationTokenNotFound {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it was changed to something that has nothing to do with the activation token. It can't be not found. It's either supported or not supported, which is why it was NotSupportedError.

Comment on lines +1516 to +1527
#[derive(Debug)]
pub enum WindowError {
/// The operation is not supported by the backend.
NotSupported(NotSupportedError),
/// The OS cannot perform the operation.
Os(OsError),
}

impl fmt::Display for WindowError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
match self {
Self::NotSupported(e) => write!(f, "window operation failed: {e}"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just name it a RequestError. I'm also not a fan of TODO items in top level stuff, could just open an issue.

Base automatically changed from add-builders to master January 27, 2024 18:16
Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack'ing the Android changes, good stuff!

Comment on lines 26 to +33
event::{self, InnerSizeWriter, StartCause},
event_loop::{self, ControlFlow, EventLoopWindowTarget as RootELW},
event_loop::{
self, ControlFlow, EventLoopCreationError, EventLoopRunError,
EventLoopWindowTarget as RootELW,
},
platform::pump_events::PumpStatus,
window::{
self, CursorGrabMode, ImePurpose, ResizeDirection, Theme, WindowButtons, WindowLevel,
self, CursorGrabMode, ImePurpose, NotSupportedError, ResizeDirection, Theme, WindowButtons,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just me who thinks these self imports are funky?

@kchibisov
Copy link
Member

ping @madsmtm

what should we do with this now?

@madsmtm
Copy link
Member Author

madsmtm commented Feb 4, 2024

what should we do with this now?

I still believe this is a change that should be done, but it'll need work, so I'll close this PR, and try to open new, smaller scoped ones for these error changes.

@madsmtm madsmtm closed this Feb 4, 2024
@madsmtm madsmtm deleted the specific-errors branch February 4, 2024 15:14
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 S - api Design and usability
Development

Successfully merging this pull request may close these issues.

5 participants