dioxuslabs / taffy Goto Github PK

What problem does this solve or what need does it fill?

Avoid issues with stale dependencies like #61

What solution would you like?

Enable dependabot. See https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file on how to do it

This is commonly desired within the games ecosystem, and if we can swing it, would be a good fit for a foundational crate like this.

Monolithic error types like this are rarely useful, and it make reading the code hard.

What problem does this solve or what need does it fill?

Based on this comment but generally having a changelog help users quickly get a high-level overview of the project and how it compares to its forks

What solution would you like?

• Create a Changelog.md in root and keep it up to date according to https://keepachangelog.com/en/1.0.0/
• Add a note in PR-template that user should update changelog if relevant
• All pull requests that contains a relevant change, should add single-line note of the change in the Changelog.md under the ## [Unreleased] section
• When cutting a release:
  1. Replace the header ## [Unreleased] with the date and version of the release: ## [x.y.z] - YYYY-MM-DD
  2. Add a new ## [Unreleased] header near the top of the document

What alternative(s) have you considered?

Keep as is and don't keep a changelog.

Source: https://github.com/DioxusLabs/sprawl/blob/main/src/geometry.rs

1. flex_start and flex_end aren't agnostic to reversed FlexDirection. This likely leads to complications elsewhere.
2. It's not at all clear why Rect / Size / Point is generic over T, if it's only ever used with an f32.
3. There is no guarantee that left < right, or bottom < top
4. The fields are called start and end, rather than the more consistent left and right.
5. No unit tests are present.
6. map and zip_size seem over-engineered.
7. Size::undefined is horrible. See #83.
8. I kind of just want to store Size and Layout
9. Size::zero and Point::zero should be associated constants.

What problem does this solve or what need does it fill?

Automatically testing with the nightly Rust toolchain will a) give us more heads up on problems that we need to solve and b) improve the developer experience for contributors using nightly c) ensure that the crate works when nightly users use it in their apps.

What solution would you like?

1. Add a new CI task that tests the crate with default features enabled using nightly.
2. Fix up any resulting errors or warnings so that the CI passes.

What alternative(s) have you considered?

#56 is a partial fix for b), but doesn't solve the other problems. It has other benefits though (contributors will be pushed to use the latest stable version of Rust, rather than an old version), so I'm happy to experiment with it.

The cargo run --package gentest command to run the generated tests does not seem to work. Both I and @CobaltCause ran into issues while trying to make it work:

Discussed in #46

   Compiling gentest v0.1.0 (/home/tim/Documents/stretch/scripts/gentest)
    Finished dev [unoptimized + debuginfo] target(s) in 2.16s
     Running `target/debug/gentest`
Starting ChromeDriver 101.0.4951.41 (93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}) on port 4444
Only local connections are allowed.
Please see https://chromedriver.chromium.org/security-considerations for suggestions on keeping ChromeDriver safe.
ChromeDriver was started successfully.
thread 'tokio-runtime-worker-4' panicked at 'unexpected webdriver error; webdriver returned error: unknown error: DevToolsActivePort file doesn't exist', /home/tim/.cargo/registry/src/github.com-1ecc6299db9ec823/fantoccini-0.11.9/src/session.rs:328:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'tokio-runtime-worker-3' panicked at 'called `Option::unwrap()` on a `None` value', /home/tim/.cargo/registry/src/github.com-1ecc6299db9ec823/fantoccini-0.11.9/src/session.rs:282:68

I'm new to Rust so this might be a basic problem. I couldn't find an example of MeasureFuncs in stretch.

I'm trying to create a text leaf node. I wrote a font loading mechanism which returns Font, and I wrote a basic text layout algorithm which takes a borrow to Font (lets call this function layout(in_bounds: Size<Number>, f: &Font) -> Size<f32>, where the returned vector is the bounds of the text. It also takes the bounds where we should layout text into). I'd like to be able to call this layout function inside the MeasureFunc, but no matter what I try it says font does not live long enough. borrowed value does not live long enough.

let mut stretch = stretch2::node::Stretch::new();
let font = load_font(...);

let text_node = stretch.new_leaf(
  Style { ..Default::default() },
  MeasureFunc::Boxed(Box::new(|size|{
    layout(size, &f)
  }))).unwrap();
stretch.compute_layout(text_node, Size{ width: Number::Defined(500), height: Number::Undefined }).unwrap();

I tried boxing and Rc-ing the font with no luck.

let mut stretch = stretch::node::Stretch::new();
let font = load_font(...);

let rc_font = Rc::new(font);

let text_node = stretch.new_leaf(
  Style { ..Default::default() },
  MeasureFunc::BoxedBox::new(|size|{
    layout(size, rc_font.as_ref())
  }))).unwrap();
stretch.compute_layout(text_node, Size{ width: Number::Defined(500), height: Number::Undefined }).unwrap();

The reason I want this is because the text layout should change based on the size of the container. What's the proper way to do this? An example of this in the future would be immensely useful!

Upstream is abandoned, so we should investigate any relevant open issues and PRs, and migrate them here as is appropriate.

As discussed in #23, we need to be sure that this continues to work.

This could just as easily be a raw Size.

We should launch the crate in earnest on crates.io ASAP, so people can start relying on it and the name doesn't get squatted.

Is there anything else that must be done before this? Anything that would be nice?

IMO the initial work that needs to be done is:

1. Code quality.
2. Better docs.
3. Design work for multi-layout and cross-language integration.

1 and 3 are irrelevant, 2 can be added in minor versions.

What problem does this solve or what need does it fill?

The current implementation is very tightly coupled and will be hard to maintain in the future. The compute_internal function has about 1k lines of code-- this will not be sustainable in the long run and might lead to code duplication once we have multiple layout algorithms.

What solution would you like?

The compute_internal function should be split up into multiple smaller functions that correspond to the steps of the layout algorithm. Each function should clearly document the spec that it implements, ideally also linking to the spec docs. The original function can then simply call the steps one-by-one, similar to the template method pattern.

We need to carefully watch how this affects performance using benchmarks, probably adding #[inline] annotations to each step.

In the future we should also have tests for each step to make sure that we fully correspond to the spec. This can be done in a separate PR though.

Acceptance Criteria

• compute_internal has been separated into multiple functions, corresponding to the steps of the layout algorithm.
• #[allow(clippy::cognitive_complexity)] has been removed.
• Each function is documented, referencing the flexbox specs.
• No functional changes have been made.
• The benchmarks don't show any major performance regressions.

Additional context

This will be the first step towards improving the functionality of the layout algorithm with #39.

Docs matter, and this is a great way to verify that we've cleaned up the docs, and don't add new code without docs.

CI is required to pass for code to be merged, and warnings will block CI without being quite as annoying as deny or forbid for local development.

Currently, there's no use of unsafe code, and we should keep it that way.

We don't use circle-ci, so we can probably just remove that folder.

We have a bit of markdown now, and the amount will increase over time.

This is helpful to avoid frustrating formatting diffs and surprising rendering.

Just spent an hour debugging why the leaf node I measured is set to 63.0 when my measure function returned 63.375

When I looked at the sources I found the explicit rounding in the algo.rs

fn round_layout(
    nodes: &mut [NodeData],
    children: &[sys::ChildrenVec<NodeId>],
    root: NodeId,
    abs_x: f32,
    abs_y: f32,
) {
    let layout = &mut nodes[root].layout;
    let abs_x = abs_x + layout.location.x;
    let abs_y = abs_y + layout.location.y;

    layout.location.x = sys::round(layout.location.x);
    layout.location.y = sys::round(layout.location.y);

    layout.size.width = sys::round(abs_x + layout.size.width) - sys::round(abs_x);
    layout.size.height = sys::round(abs_y + layout.size.height) - sys::round(abs_y);

    for child in &children[root] {
        Self::round_layout(nodes, children, *child, abs_x, abs_y);
    }
}

I couldn't find any mentions why the rounding is needed, thus I'm curious. It seems to be 100% intentional. Maybe add an explanation in the code annotations?

What problem does this solve or what need does it fill?

Some test cases compare the behavior of this library with Chrome using chromedriver (see the README). If chromedriver is not installed, you get the following error:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', scripts/gentest/src/main.rs:38:88

This is not very helpful to determine the issue that caused the error.

What solution would you like?

https://github.com/DioxusLabs/stretch/blob/0030950f7bf7a973e1cd44d99cd0a13ded6749a6/scripts/gentest/src/main.rs#L38

The .unwrap() should be replaced by an expect(), which explains that you have to install chromedriver for the tests to work.

Generated bindings require the inclusion of binary code, as seen in #4. This is effectively impossible to audit, and a pain for contributors and maintainers.

These should be built automatically in some form instead.

This can and should be very simple to start.

Previous text:

Contributing

Contributions are very welcome. Though we ask that you open an issue or pull request early in the process (before writing code) so we can discuss solutions and additions before you start spending time on implementing them. There are some specific areas where we would be extra happy to receive contributions in.

• Binary size reduction
• Runtime performance
• Ensure build / test environment works well on non macOS platforms
• Alternate layout systems (grid layout perhaps?)
• Web compatibility tests
• RTL support
• Platform bindings
• API improvements
• Documentation & Examples

This was done by stretch originally, but relied on their own proprietary infrastructure.

We should either remove these bits of the code base, or navigate the JS / Kotlin / Swift / other ecosystems to do this properly again.

I have no personal need for these integrations: we should try to onboard maintainers who do care if we want to keep this functionality.

The bug template here is particularly useful: bevyengine/bevy#4652

I would also probably grab the Enhancement issue template from Bevy.

This is a consistent problem in the code base.

This field is correctly flagged as being never read from.

However, there's a non-trivial write to it in the final layout pass here.

My initial guess was that this was recorded to be serialized, in order to be compared against some result. However the struct itself isn't Serialize.

In sys.rs, we define some core data structures in 3 ways:

1. If std is enabled.
2. If alloc is enabled.
3. If neither is enabled.

Unfortunately, this isn't how Rust's features work. They should be additive, and all possible combinations should be valid.
The std feature and alloc feature currently do not work together, because this yields a painful name-space collision.

This results in a large number of Rust Analyzer errors, complaining that names are ambiguous.

If both are enabled, std should take priority over alloc.

Ideally, with decent setup instructions.

stretch2 is not exactly a compelling name.

The sooner we rebrand, the less confusing it will be.

What problem does this solve or what need does it fill?

The issue is as old as flex box: spacing between elements

What solution would you like?

Implement the spec: https://developer.mozilla.org/en-US/docs/Web/CSS/gap#specifications
Or at least a reasonable subset, note that calc should be probably outside of the scope

Thus, I think, implementing just explicit values should be good enough (at least as a start)

from spec:

gap: 20px; /* <--- that would be totally enough for me */


/* these are probably way less useful and applicable */
gap: 1em;
gap: 3vmin; 
gap: 0.5cm;
/* One <percentage> value */
gap: 16%;
gap: 100%;

What alternative(s) have you considered?

Work around is simple: margins + negative padding, but super cumbersome

As discussed by the original stretch project, the ability to multithread layout can be very useful.

We should have opt-in ways to do so.

In Cargo.toml, there is still a reference to the vislyhq/stretch repository for a circle-ci badge.

I'm not really sure what this means, but I assume it will have to either be removed or updated to point to DioxusLabs/stretch.

[badges]
circle-ci = { repository = "vislyhq/stretch", branch = "master" }
maintenance = { status = "experimental" }

This is helpful to ensure that PRs are well-described and easy to review. Feel free to steal from https://github.com/bevyengine/bevy/blob/main/.github/pull_request_template.md

What problem does this solve or what need does it fill?

Some people have nightly set as their default toolchain, but we use stable in CI.
This might cause inconsistencies with compile errors and clippy warnings that are generated locally vs. in the CI.

What solution would you like?

Create a top-level rust-toolchain.toml file with the following content:

[toolchain]
channel = "stable"

What alternative(s) have you considered?

Of course it's possible to set the toolchain using command overrides like cargo +stable check or rustup override set stable.

Personally I think it's better to define this directly in the source to avoid unintended behavior. I personally already encountered a clippy warning difference between CI and my local setup due to different toolchains.

Additional context

Documentation on Rust toolchain overrides: https://rust-lang.github.io/rustup/overrides.html

Tracking Issues:

• Add feature flag for Flexbox: #298
• CSS Grid: #204
• Morphorm: #308

This library will be dramatically more useful if it can be used to support multiple layout strategies. This is useful because:

• it centralizes community effort
• it enables fair benchmarks
• it lets users easily swap out approaches
• it creates the potential for blended layouts in the same app

Must have:

• each layout algorithm lives behind its own feature flag
• this crate provides a relatively unified interface to them
  • a trait seems like the natural fit here

Nice to have:

• low or zero abstraction overhead
• the ability to nest multiple distinct layout strategies, ala flutter
• competitive benchmarks between the different strategies
• rosetta-stone comparisons of practical examples, demonstrating how to create the same / comparable layouts in different paradigms
• users can create their own layout algorithms using our primitives in an interoperable way

Possible layout algorithms we may want to support:

• hstack / vstack
• Swift-UI style
• Flutter style
• CSS grid
• Morphorm
• Cassowary

What problem does this solve or what need does it fill?

When running cargo bench, I get the following warning:

WARNING: HTML report generation will become a non-default optional feature in Criterion.rs 0.4.0.
This feature is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'html_reports' feature in your Cargo.toml.

What solution would you like?

As explained in the warning, we need to either switch to cargo-criterion or enable the html_reports feature in our Cargo.toml.

This should be updated as part of each PR, see #30.

Feel free to steal the structure from https://github.com/Leafwing-Studios/leafwing_input_manager/blob/main/RELEASES.md

One question I have is, do we want to rename sprawl::node::Stretch to sprawl::node::Sprawl (or something else) first?

Originally posted by @CobaltCause in #60 (comment)

The entire Number enum appears to be an unidiomatic reimplementation of Option<f32> (or perhaps a Result).

Remove this type, and replace it throughout the code base.

This is breaking, because this type is public 🥲.

https://crates.io/crates/stretch2 points to https://github.com/dioxuslabs/dioxus, but should point to this repo.

#[no_mangle]
pub unsafe extern "C" fn stretch_node_compute_layout(
    stretch: *mut c_void,
    node: *mut c_void,
    width: f32,
    height: f32,
    create_layout: fn(*const f32) -> *mut c_void,
) -> *mut c_void {
    let mut stretch = Box::from_raw(stretch as *mut Stretch);
    let node = Box::from_raw(node as *mut Node);

    stretch
        .compute_layout(
            *node,
            Size {
                width: if f32::is_nan(width) { Number::Undefined } else { Number::Defined(width) },
                height: if f32::is_nan(height) { Number::Undefined } else { Number::Defined(height) },
            },
        )
        .unwrap();

    let mut output = vec![];
    copy_output(&stretch, *node, &mut output);

    Box::leak(stretch);
    Box::leak(node);

    create_layout(output.as_ptr())
}

The unwinding of stretch.compute_layout will introduce double free due to dropping stretch and node in the unwinding path. With the appearance of this question, we noticed that this kind of from_raw usage is not strong enough for FFI functions😨.

The leak should take forward to avoid the automated deallocation towards objects generating by from_raw. We found std::mem::ManuallyDrop is a good wrapper for us to avoid the drop as well as making the variable act like the smart pointer.

The fixed code we listed below. And this refinement is zero-cost.

#[no_mangle]
pub unsafe extern "C" fn stretch_node_compute_layout(
    stretch: *mut c_void,
    node: *mut c_void,
    width: f32,
    height: f32,
    create_layout: fn(*const f32) -> *mut c_void,
) -> *mut c_void {
    let mut stretch = mem::ManuallyDrop::new(Box::from_raw(stretch as *mut Stretch));
    let node = mem::ManuallyDrop::new(Box::from_raw(node as *mut Node));

    stretch
        .compute_layout(
            *node,
            Size {
                width: if f32::is_nan(width) { Number::Undefined } else { Number::Defined(width) },
                height: if f32::is_nan(height) { Number::Undefined } else { Number::Defined(height) },
            },
        )
        .unwrap();

    let mut output = vec![];
    copy_output(&stretch, *node, &mut output);
    create_layout(output.as_ptr())
}

We think all the usage (involving 24 functions) of from_raw in bindings for Swift and Kotlin should be changed by wrapping with ManuallyDrop to avoid double free incurred by unwinding 😊.

Seems to be a link to kotlin integration?

Avoid item-level imports

The code base is littered with snippets like:

    pub(crate) nodes: sys::Vec<NodeData>,

these should be refactored to use the clearer (and more standard) form:

use sys::Vec

   pub(crate) nodes: Vec<NodeData>,

Avoid glob imports

Some internal files use glob imports aggressively (e.g. use crate::style::*;). This leads to confusing ambiguities, and warnings like:

`ChildrenVec` is ambiguous
ambiguous because of multiple glob imports of a name in the same module
consider adding an explicit import of `ChildrenVec` to disambiguate
consider adding an explicit import of `ChildrenVec` to disambiguate

Remove these; you can find them by searching ::* in the code base.

Importing prelude::* in examples is fine to keep.

What problem does this solve or what need does it fill?

lazy_static involves macros, takes a bit longer to compile, and once_cell provides more flexible ways on the way to being added to std.

What solution would you like?

Replace all usages of lazy_static with static VAR: Lazy<T>.

One potential drawback is that lazy_static supports spin-waiting on the result, while Lazy doesn't. I don't think any of the crate's usages are perf-bound by this though.

What alternative(s) have you considered?

Leave it as is.

1. Remove from README (duplicative).
2. Figure out how to properly credit both Stretch2 contributors and original Visly team.

I'd like to cut a new release soon, and that means we need a clean and useful changelog.

Problem

While browsing the implementation of the algorithm in this project, I noticed some things that probably should be cleaned up in the future.

Missing Implementation

There's a few steps that are not implemented yet and probably should be.

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L296-L302

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L304-L310

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L304-L310

Out of spec Implementation

It seems like there's some steps in the implemented algorithm that are outside of the Flexbox specification.

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L269-L270

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L360-L363

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L454-L455

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L605-L609

Tight coupling

Maintainability of the Flexbox algorithm in the future is important, more so if more algorithms are implemented side-by-side next to it.
Thus, I think that the flexbox (and future) algorithm implementation(s) should be divided by steps.
While it requires more code to setup and do properly, it will allow to have simpler and shorter methods for each step of the layouting.

I hope I didn't miss anything, first time I am opening an issue here.

Now that the algorithm has been broken down into chunks in #88; we can start unit testing each of the parts.

This is essential for #39, and helps us decide what we want to do about the generated tests in #65.

If possible, I'd like to fuzz these (or use property-based testing): edge cases are hard.

This is used sparingly, is causing us issues with duplicate dependencies, and pulls in a C library.

@colepoirier is preparing a PR to include the relevant source code directly in this crate.

This is an unclear, overengineered abstraction.

We should re-architect to avoid it completely.

It also causes usability headaches, as seen in #57.