Skip to content

Remove WindowBuilderExtIOS::with_root_view_class#2459

Merged
madsmtm merged 2 commits intomasterfrom
remove-root-view-class
Nov 23, 2022
Merged

Remove WindowBuilderExtIOS::with_root_view_class#2459
madsmtm merged 2 commits intomasterfrom
remove-root-view-class

Conversation

@madsmtm
Copy link
Member

@madsmtm madsmtm commented Sep 2, 2022

This was introduced in #609 (CC @mtak-), and while it makes sense, it feels quite unclean and inhibits other improvements (also, it is unsound, but that could be fixed by marking the function as unsafe).

Primary motivation is that it prevents us from doing the iOS equivalent of #2458, since objc2::declare_class! (intentionally) does not support dynamically declaring classes. If we want this, it should be done in a way that the desired superclass is known at compile-time.

I did a search on GitHub to get a feel for who was using it, seems like it's really only glutin, and that will have to do something else after rust-windowing/glutin#1435 anyhow. Libraries like wgpu and vulkano-win have already found solutions for this, I strongly suspect that glutin could do something similar like (but I don't have a setup to verify that atm.):

let main_layer: Id<CALayer, Shared> = msg_send_id![view, layer];
let new_layer: Id<CALayer, Shared> = msg_send_id![CAEAGLLayer::class(), new];
let _: () = msg_send![main_layer, addSublayer: &*new_layer];
  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • 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

Would like your input on this one @francesca64?

@madsmtm madsmtm added S - api Design and usability DS - uikit Affects the UIKit backend (iOS, tvOS, watchOS, visionOS) S - maintenance Repaying technical debt labels Sep 2, 2022
@madsmtm madsmtm requested a review from august64 as a code owner September 2, 2022 15:53
@madsmtm madsmtm force-pushed the remove-root-view-class branch 5 times, most recently from eb7208b to 53cc8f6 Compare September 2, 2022 17:40
@madsmtm madsmtm mentioned this pull request Sep 2, 2022
1 task
@august64
Copy link
Member

august64 commented Sep 2, 2022

@ArthurKValladares does brainstorm still use this?

@ArthurKValladares
Copy link
Contributor

ArthurKValladares commented Sep 2, 2022

@ArthurKValladares does brainstorm still use this?

We do still use this internally. I can try to carve out some time early next week to explore the alternative solutions linked here used by wgpu and vulkano. We are also already on our own branch, with changes that don't really makes sense to be merged upstream, so I would not consider our internal usage of this feature to be a blocker for the PR.

@madsmtm madsmtm force-pushed the remove-root-view-class branch from 53cc8f6 to 878fa1d Compare September 8, 2022 18:35
@madsmtm
Copy link
Member Author

madsmtm commented Nov 23, 2022

Alternative solution would be to allow overriding the return value from +[UIView layerClass].

I'm going to merge this now, though if you find that none of the other solutions are workable, feel free to post an issue about it, then we can more easily discuss what to do.

@madsmtm madsmtm added this to the Version 0.28 milestone Nov 23, 2022
@madsmtm madsmtm merged commit 6d0cf6a into master Nov 23, 2022
@madsmtm madsmtm deleted the remove-root-view-class branch November 23, 2022 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DS - uikit Affects the UIKit backend (iOS, tvOS, watchOS, visionOS) S - api Design and usability S - maintenance Repaying technical debt

Development

Successfully merging this pull request may close these issues.

3 participants