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

[issue #2002] Ensure that absolute LineHeight is always > 0.0 #2059

Merged

Conversation

joshuamegnauth54
Copy link
Contributor

@joshuamegnauth54 joshuamegnauth54 commented Aug 31, 2023

Fixes #2002.

The gist of the problem is that LineHeight::to_absolute in conjunction with other widgets may inadvertently return 0.0. This triggers a panic in cosmic_text's Buffer regardless of the renderer.

The original issue has a minimal example. My patch just ensures that absolute LineHeight is > 0.0 to avoid the panic.

@B0ney
Copy link
Contributor

B0ney commented Aug 31, 2023

Awesome! The patch fixed the issue for me.

@hecrj
Copy link
Member

hecrj commented Sep 3, 2023

Since this is a limitation of a dependency used in the renderers, I believe the fix should live there.

@joshuamegnauth54
Copy link
Contributor Author

joshuamegnauth54 commented Sep 3, 2023

Hmm, the panic is in cosmic_text, but the panic is expected and part of its public API:

    /// Create a new [`Buffer`] with the provided [`FontSystem`] and [`Metrics`]
    ///
    /// # Panics
    ///
    /// Will panic if `metrics.line_height` is zero.
    pub fn new(font_system: &mut FontSystem, metrics: Metrics) -> Self {
        let mut buffer = Self::new_empty(metrics);
        buffer.set_text(font_system, "", Attrs::new(), Shaping::Advanced);
        buffer
    }

So it's the caller's responsibility (iced in this case) to ensure that line height is > 0.0.

Edit: I understand your concerns of course, but I think cosmic_text won't remove the panics since a lot of its public API documents panics for invalid values. Buffer has a few of them.

@hecrj
Copy link
Member

hecrj commented Sep 3, 2023

@joshuamegnauth54 For sure! I meant that the fix should live in iced_wgpu and iced_tiny_skia instead of iced_core. Basically, after calling to_absolute.

@joshuamegnauth54
Copy link
Contributor Author

Alright, that makes sense! I'll look into it soon.

@hecrj hecrj added this to the 0.11.0 milestone Sep 3, 2023
@joshuamegnauth54
Copy link
Contributor Author

I updated the WGPU and Tiny Skia renders. cargo test --all passes and b0ney's code works too.

Let me know if there's anything else I should do.

tiny_skia/src/text.rs Outdated Show resolved Hide resolved
tiny_skia/src/text.rs Outdated Show resolved Hide resolved
@joshuamegnauth54
Copy link
Contributor Author

Okay, so I moved the LineHeight logic to allocate and removed the previous checks. Everything still works!

Comment on lines 409 to 410
key.line_height,
(key.line_height * 1.2).max(f32::MIN_POSITIVE),
Copy link
Member

Choose a reason for hiding this comment

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

Almost! This should look the same as the wgpu code:

Suggested change
key.line_height,
(key.line_height * 1.2).max(f32::MIN_POSITIVE),
key.size,
key.line_height.max(f32::MIN_POSITIVE),

You can test the tiny-skia backend by setting the ICED_BACKEND=tiny-skia env variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I just committed the requested changes.

Also, I've been recompiling iced with default-features=false to test Tiny Skia. 😅 That env var is very helpful.

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks.

@hecrj hecrj enabled auto-merge September 6, 2023 23:44
@hecrj hecrj added the text label Sep 6, 2023
@hecrj hecrj merged commit 6fc88c3 into iced-rs:master Sep 7, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when resizing window with checkbox
3 participants