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

Functions in windows::Win32::Graphics::Gdi should return Result #1758

Open
Enyium opened this issue Dec 11, 2023 · 7 comments
Open

Functions in windows::Win32::Graphics::Gdi should return Result #1758

Enyium opened this issue Dec 11, 2023 · 7 comments

Comments

@Enyium
Copy link

Enyium commented Dec 11, 2023

In windows::Win32::Graphics::Gdi, there are many functions like CreateSolidBrush(), CreateBitmap(), CreateBitmapIndirect(), CreateEllipticRgn() etc. that return handles, although the docs at learn.microsoft.com say, they can fail. So, they should return Results like many other fallible functions in the windows crate already do.

@Enyium
Copy link
Author

Enyium commented Dec 11, 2023

BTW: On the page for CreateBitmap(), ERROR_INVALID_BITMAP is listed as a possible "return code" (after saying the function returns NULL on error; so, probably meant as a GetLastError() code). But, what is really strange is that the value for this constant is nowhere to be found - it's also missing in the windows crate. I searched with Google, on every error code page and in my local folder C:\Program Files (x86)\Windows Kits (1.71 GB of size), but didn't find the concrete value!

@riverar
Copy link
Collaborator

riverar commented Dec 11, 2023

The Win32 metadata project is aimed at modeling all the APIs in a language-neutral manner.

Metadata currently represents CreateSolidBrush as:

[DllImport("GDI32.dll", ExactSpelling = true, PreserveSig = false)]
[Documentation("https://learn.microsoft.com/windows/win32/api/wingdi/nf-wingdi-createsolidbrush")]
...
public static extern HBRUSH CreateSolidBrush([In] COLORREF color);

And Rust (windows) generates:

pub unsafe fn CreateSolidBrush<P0>(color: P0) -> HBRUSH
where
    P0: IntoParam<COLORREF>

@kennykerr With [InvalidHandleValue(-1L)] and [InvalidHandleValue(0L)] on HBRUSH, is there an opportunity to do Result transformation here? Or is the cost vs. benefit here too small?

@riverar
Copy link
Collaborator

riverar commented Dec 11, 2023

Upon further thought, it's not clear how Rust would know when to apply such a transformation. It can't assume all functions returning HBRUSH can be transformed. And it's not going to maintain a one-off list.

@tim-weis
Copy link
Contributor

The situation cannot be improved here. None of the functions listed set the calling thread's last error code. The metadata already does the right thing by not applying the SetLastError = true attribute on those API calls.

@Enyium
Copy link
Author

Enyium commented Dec 12, 2023

What about providing Err(windows::core::Error::from(E_FAIL)) on error? Besides the handle, there's the additional layer of success/error, after all, which would ideally get a rusty representation.

EDIT: Or rather E_HANDLE ("Handle that is not valid").

EDIT: This impl is an example of other code that chooses something fitting:

impl std::convert::From<std::string::FromUtf16Error> for Error {
    fn from(_: std::string::FromUtf16Error) -> Self {
        Self { code: HRESULT::from_win32(crate::imp::ERROR_NO_UNICODE_TRANSLATION), info: None }
    }
}

@tim-weis
Copy link
Contributor

What should the win32metadata team change to make that happen?

@Enyium
Copy link
Author

Enyium commented Dec 12, 2023

I don't know your system and what it allows you to implement. I also don't have a clear understanding of the border between this repo and the windows crate. I'm just interested in a more consistent, somewhat easier to use and structurally more predictable windows crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants