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

iOS support for label, text input, font, more tests #55

Merged
merged 13 commits into from
Jul 10, 2023

Conversation

simlay
Copy link
Contributor

@simlay simlay commented Aug 30, 2022

I was on vacation last week and did this for fun so it's a bit disorganized. I started out doing unit tests and then got off track. This PR should probably be broken up into a few things.

changes

  • layer on ImageView
  • Added uikit support for Label via UILabel
  • TextField and TextFieldDelegate viaUITextField (though, this uses itself as a delegate which seems less than ideal)
  • Font unit tests and what I think is an okay impl Default for Font with uikit.
  • ios-beta has a Label, TextView<ConsolLogger> and the ImageView is in the center of the 3rd view:

image

Tests on:

  • Partial on Label
  • Full on Font
  • Partial on TextView
  • Minimal on View.

Protip:

There's not a great way to develop for mutually exclusive feature flags but what I have found is that if we structure things like:

#[cfg(feature = "appkit")]
mod appkit;

#[cfg(feature = "appkit")]
use appkit::{register_view_class, register_view_class_with_delegate};

#[cfg(feature = "uikit")]
mod uikit;

#[cfg(all(feature = "uikit", not(feature = "appkit")))]
use uikit::{register_view_class, register_view_class_with_delegate};

In an editor config (I use vim with an LSP client and rust-analyzer) enable both of those feature flags you can reasonably navigate the codebase. The "jump to definitionofregister_view_classis this case would go toappkitdefinition but to get to theuikitdefinition, you have to jump to definition of themod uikitand then theregister_view_class`.

Here's the .vim/settings.json I used in this PR (I think this is either identically or similar for most other setups):

{
    "rust-analyzer": {
        "cargo": {
            "target": "aarch64-apple-ios-sim",
            "features": ["uikit", "autolayout","appkit", "cloudkit", "user-notifications", "quicklook", "webview"],
            "noDefaultFeatures": true
        },
        "initialization_options": {
            "cargo": {
                "buildScripts": {
                    "enable": true
                }
            },
            "completion": { "autoimport": {"enable": true } },
            "procMacro": {
                "enable": true
            }
        }
    }
}

@ryanmcgrath
Copy link
Owner

This is so cool! Really excited to see the iOS side coming together - hella appreciated as always. Will look at merging this soon, sat down to look at the objc2 stuff today but got pulled into other issues work-related. :(

Re: the appkit/uikit breakdown, one thing I did in the view module was this:

cacao/src/view/mod.rs

Lines 67 to 69 in f84a9df

#[cfg_attr(feature = "appkit", path = "appkit.rs")]
#[cfg_attr(feature = "uikit", path = "uikit.rs")]
mod native_interface;

Curious for your thoughts on it - is it better/worse/irrelevant given your point regarding feature flags above?

@simlay
Copy link
Contributor Author

simlay commented Aug 30, 2022

Will look at merging this soon, sat down to look at the objc2 stuff today but got pulled into other issues work-related. :(

No pressure! I was tempted to wait on the objc2 stuff but got excited doing this.

Re: the appkit/uikit breakdown, one thing I did in the view module was this:

cacao/src/view/mod.rs

Lines 67 to 69 in f84a9df

#[cfg_attr(feature = "appkit", path = "appkit.rs")]
#[cfg_attr(feature = "uikit", path = "uikit.rs")]
mod native_interface;

Curious for your thoughts on it - is it better/worse/irrelevant given your point regarding feature flags above?

This is actually one of the few places I noticed it and found it a little difficult. I might submit a separate PR. I doubt there's any clippy/fmt-esk tool to help with this.

@simlay simlay force-pushed the more-ios-tests branch from 70d05ef to d9d8b23 Compare May 1, 2023 03:57
@simlay simlay marked this pull request as ready for review May 14, 2023 19:37
@ryanmcgrath
Copy link
Owner

@simlay is this okay to be merged? Looking to cut a 3.3-beta1 or something to crates.io and figured I should tie off some of the older PRs where I can.

@simlay
Copy link
Contributor Author

simlay commented Jun 9, 2023

@simlay is this okay to be merged? Looking to cut a 3.3-beta1 or something to crates.io and figured I should tie off some of the older PRs where I can.

Yeah. I actually updated this branch out of the blue 3 weeks ago. It looks like there might be more merge conflicts. Do you wanna deal with those or shall I?

@ryanmcgrath
Copy link
Owner

Hmmm, I think it's actually just a formatting pass that's needed... otherwise this stuff could fit into a 0.4.0-beta2.

@simlay
Copy link
Contributor Author

simlay commented Jun 9, 2023

Hmmm, I think it's actually just a formatting pass that's needed... otherwise this stuff could fit into a 0.4.0-beta2.

Hopefully I just fixed that. We'll see.

@ryanmcgrath
Copy link
Owner

Aye, looks like it's all smooth now - shall I merge?

@SmartBoy84
Copy link

Merge! (pretty please)

@ryanmcgrath ryanmcgrath merged commit e4785bb into ryanmcgrath:trunk Jul 10, 2023
@ryanmcgrath
Copy link
Owner

Merged. :)

I'll likely cut a 0.4.2 after one or two more PRs get in, or next week if they take longer.

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

Successfully merging this pull request may close these issues.

3 participants