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

WIP: Java bindings #705

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

WIP: Java bindings #705

wants to merge 32 commits into from

Conversation

adjabaev
Copy link
Contributor

@adjabaev adjabaev commented Aug 19, 2024

Objective

Make taffy usable in Java programs

Feedback wanted

Feel free to give me advice over what I am doing as it is the first time I am actually writing Rust/ jni bindings

TODO

  • Implement style in taffy_tree
  • Implement children in taffy_tree (basically returning a list)
  • Figure out whether dirty and mark_dirty might be useful to Java users
  • Implement NodeContext related stuff
  • Find a way to implement compute_layout_with_measure, (do I write a Java wrapper around MeasureFunction?)

@alice-i-cecile alice-i-cecile added enhancement New feature or request controversial This work requires a heightened standard of review due to implementation or design complexity labels Aug 19, 2024
@adjabaev
Copy link
Contributor Author

Do you think it is a good idea keeping Taffy in name of every class (I am asking the question because I feel like java doesn't have as much concise ways to express certain ideas which leads to some code looking quite long) (not even talking about the fact that in Java the types of variables are usually explicitly written).
Also I'd be very curious knowing what do you think so far

@adjabaev adjabaev mentioned this pull request Aug 21, 2024
37 tasks
@nicoburns
Copy link
Collaborator

@adjabaev very excited to see this. I'm on vacation (with limited access to computers / internet) atm. But I will definitely review this when I get back.

One thing that I have noticed from writing C and WASM Bindings (neither finished/merged yet), and that I also see here, is that there is a lot of repetitive code related to defining enums, converting enums, fields on Style, methods on TaffyTree, etc that is duplicated in each binding and in the main Rust implementation. And I have been thinking it might be good from a maintenance perspective to use code generation to automatically generate this code from a metadata definition similar to Yoga's enums.py script (this would be pretty helpful when we add new style fields for example).

Regarding naming conventions, I'm not sure. Just Java have namespaces? C doesn't, which is the main reason why it has everything prefixed.

@adjabaev
Copy link
Contributor Author

@adjabaev very excited to see this. I'm on vacation (with limited access to computers / internet) atm. But I will definitely review this when I get back.

Glad you like the initiative! :)
I hope you will enjoy the rest of your well deserved vacations (I actually think so, it is crazy the amount of things that have changed since the last time I have seen it!)

One thing that I have noticed from writing C and WASM Bindings (neither finished/merged yet), and that I also see here, is that there is a lot of repetitive code related to defining enums, converting enums, fields on Style, methods on TaffyTree, etc that is duplicated in each binding and in the main Rust implementation. And I have been thinking it might be good from a maintenance perspective to use code generation to automatically generate this code from a metadata definition similar to Yoga's enums.py script (this would be pretty helpful when we add new style fields for example).

It is definitely something I have noticed and I would be more than glad to find a way to automate this stuff, for enum related stuff, I feel like its not gonna be very difficult (thanks to Java's Enum.ordinal(). But in the same time I am wondering if we couln't just Serialise/ Deserialise stuff as Json strings. At least I feel like it would lower the implied work by a lot, however I cannot tell what it is going to be performance wise.

Regarding naming conventions, I'm not sure. Just Java have namespaces? C doesn't, which is the main reason why it has everything prefixed.

Yes, Java (and Kotlin) have packages which are basically namespaces, so I removed Taffy from every class (every file still start with "package com.dioxuslabs.taffy;" (except for TaffyTree because it is its original name)

@adjabaev
Copy link
Contributor Author

adjabaev commented Aug 26, 2024

Hi :)
I did some changes to how enums work, these are now generated automatically. This is probably something that could go into a meta PR that would help all bindings, but I thought it would be nice to showcase the idea here before flooding with another PR. What do you guys think?

Also for now, it only generates classic enums without any parameter because in Java there is not such notion as enums that have different parameters (signatures)


Also another question, the MeasureFunction/ NodeContext stuff seems very hard to translate to Java dynamically as NodeContext can by anything, if anyone has an idea, i'm all ears! :)

bindings/scripts/src/main.rs Outdated Show resolved Hide resolved
bindings/scripts/Cargo.toml Outdated Show resolved Hide resolved
@@ -0,0 +1,251 @@
use crate::conversions::f_get_value;
use crate::primitives::f_i32_from_primitive;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this file could be auto-generated too. My suggested strategy would be:

  1. Create a trait:
trait FromJavaEnum {
     const JAVA_CLASS : &'static str;
     fn from_ordinal(ordinal: i32) -> Option<RustType>;
}
  1. Auto-generate an impl of this trait for each rust enum when generating the java enum.

  2. This file should then just need two (generic) functions: one for T (that returns default if the value is null) and one for Option

Copy link
Contributor Author

@adjabaev adjabaev Sep 2, 2024

Choose a reason for hiding this comment

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

This was actually an incredible idea, just commited an implem of this, would be very interested in knowing what you do think about it :)

Also i did a little refactor, so we can split different steps (enum generation/ transformers) and different languages (just java.rs 's for now, but we can imaging variants for js, c, cpp, etc)

private float scrollbarWidth;

private Position position;
private Rect<LengthPercentageAuto> inset;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could consider "flattening" the fields that use Rect, Size, and Line (so have width and height fields instead of size). I reckon that would simplify the API quite a bit (and mean you wouldn't need to define the geom java types or conversions at all.

Copy link
Contributor Author

@adjabaev adjabaev Sep 2, 2024

Choose a reason for hiding this comment

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

I guess I could look at it, but what about situations where only a part of components of a geom is defined? (let's say only left is defined in a Rect)

* @param margin The size of the margin of the node
*/
public record Layout(
int order,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess if you flatten Style you'll probably also want to flatten Layout.

@adjabaev
Copy link
Contributor Author

adjabaev commented Sep 2, 2024

Also clippy doesn't validate but it looks like it isn't due to my code (and it wasn't the case before i wrote it, I tried looking for the reasons but i do not manage to understand it and would like to avoid modifying Taffy code if it isn't necessary)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controversial This work requires a heightened standard of review due to implementation or design complexity enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants