Add initial support for OpenHarmony #293
Conversation
|
This is looking great. I tested the Android build locally and it compiled. I didn't test it in an emulator. I suppose this is just blocked on a new raw-window-handle release now. |
Ah, actually raw-window-handle 0.6.2. is already released, I just forgot to update this PR after the release. |
| @@ -0,0 +1,87 @@ | |||
| // surfman/surfman/src/platform/egl/android_surface.rs | |||
There was a problem hiding this comment.
I think this file name is incorrect now. To be honest, I'm not sure why the file name is included at the top of these files. It seems redundant.
There was a problem hiding this comment.
Yeah, I was also wondering about this, but assumed that perhaps there was some kind of tooling I don't know about, which requires these comments.
I will fix the name here, but if there is no reason for these comments (anymore), then I'd also be happy to open a PR that removes all of the filenames at the top.
| // Safety: `OH_NativeWindow_NativeWindowHandleOpt` takes two output i32 pointers as | ||
| // variable arguments when called with `GET_BUFFER_GEOMETRY`. See the OHNativeWindow | ||
| // documentation for details: | ||
| // https://gitee.com/openharmony/docs/blob/master/en/application-dev/reference/apis-arkgraphics2d/_native_window.md |
There was a problem hiding this comment.
Perhaps this should move to a rustdoc safety section?
There was a problem hiding this comment.
This comment is intended for reviewers of the unsafe block, since OH_NativeWindow_NativeWindowHandleOpt takes different arguments depending on the value of second parameter, and thus needs to be manually cross-referenced with the documentation during review.
Callers of this function don't need to know about OH_NativeWindow_NativeWindowHandleOpt.
To be honest I'm not really sure what the Safety preconditions of create_window_surface() should be (and if it needs to be unsafe). But I'm pretty sure the Safety preconditions should be the same as the android version (which isn't documented either, though)
There was a problem hiding this comment.
I added a Safety section to the rustdoc of the function declaration in ohos_ffi.rs
There was a problem hiding this comment.
I agree with your original statement and IMO the original placement isn't an issue. I've just seen that clippy often complains if an unsafe function does not have a "# Safety" section. It just seems that since you have already gone through the trouble of writing a nice comment here, we can use it to make clippy happy as well. :D
|
Is this still a draft or is it okay to merge this? |
jschwe
changed the title
|
It's okay to merge from my side. Should I squash the review changes into the second commit, or will you squash everything into one commit anyway? |
|
The merge queue automatically squashes commits, so it shouldn't be an issue. Thanks! |
Follow-up PR to #285.
Todo: