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

Add DPI scale factor to the render root state #872

Merged
merged 5 commits into from
Feb 20, 2025

Conversation

eugenesvk
Copy link
Contributor

so that it's available to widgets that can fix blurry lines due to bad overlapping positioning of its internal elements

partially addresses #869

so that it's available to widgets
as it's not called on dpi changes, so will have outdated info
@DJMcNab
Copy link
Member

DJMcNab commented Feb 18, 2025

I'll defer to @PoignardAzur, but I am happy to approve in principle, as a stop-gap until we get a more coherent design.

(This would also be useful for e.g. Image type widgets, to know which resolution of an image to load from the remote)

Obviously it is true that this value is pretty meaningless for widgets with a transform applied, but that's pretty unavoidable; Transforms are designed to break these kinds of things anyway.

Copy link
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

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

LGTM.

We're overdue for a coherent, well-documented handling of scaling, and this is a first step towards that.

Since it only causes re-render, so app logic doesn't need it.
let it live in RenderRootState only
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Thank you for your patience. I don't need you to do anything else on this; I'll apply the suggestions and merge tomorrow.

Comment on lines 783 to 784
// Methods on selected context types.
// Access selected global state information.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just remove these comments - they give very little information.

I'll do this tomorrow if this hasn't landed by then.

// Methods on selected context types.
// Access selected global state information.
impl_context_method!(PaintCtx<'_>, AccessCtx<'_>, {
/// Get DPI scaling factor.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Get DPI scaling factor.
/// Get DPI scaling factor.
///
/// This is not required for most widgets, and should be used only for precise
/// rendering, such as rendering single pixel lines or selecting image variants.
/// This is currently only provided in the render stages, as these are the only passes which
/// are re-run when the scale factor changes.
///
/// Note that accessibility nodes and paint results will automatically be scaled by Masonry.
/// This also doesn't account for the widget's current transform, which cannot currently be
/// accessed by widgets directly.

@DJMcNab DJMcNab enabled auto-merge February 20, 2025 15:06
@DJMcNab DJMcNab added this pull request to the merge queue Feb 20, 2025
Merged via the queue into linebender:main with commit 5e85438 Feb 20, 2025
18 checks passed
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