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

Rename Points/points to Length/length in many APIs #459

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

SimonSapin
Copy link
Contributor

⚠️ This is a fairly disruptive breaking changes, although the fix for users is mostly search-and-replace.

Objective

The old name for this one-dimensional length:

  • Suggests PostScript points, but is documented as an abstract unit: “Users of Taffy may define what they correspond to in their application (pixels, logical pixels, mm, etc) as they see fit.”

  • Is spelled similar to the two-dimensional Point though its meaning is unrelated.

  • In CSS syntax, 1px = 0.75pt but the former is much more commonly used. When parsing CSS into a Style we’ll probably want the default mapping to be 1px => 1f32 abstract unit, so naming the abstract unit "points" would be confusing.

Context

This was discussed in #440 (comment):

Taffy's Dimension::Points is best thought of as pixels. I believe every consumer of Taffy is using it as such. However, there is a slight complication in that it could be interpreted as either logical pixels (as in CSS) or physical device pixels. At least bevy is passing Taffy physical device pixels. And Taffy will optionally round the final layout to the nearest pixel which makes more sense to do with physical pixels.

Feedback wanted

Are the benefits listed above worth the disruption to existing users?

@SimonSapin
Copy link
Contributor Author

Ah, this already has conflicts. Given the number of changes (although many are in generated files), I’ll wait for a maintainer to approve the spirit of the change and say they’re ready to merge before I rebase and fix conflicts.

@alice-i-cecile
Copy link
Collaborator

I agree with moving away from Points: the reasons you've cited are good and the migration is straightforward. This is a hold-over from our adopted codebase.

@nicoburns
Copy link
Collaborator

I'm definitely in agreement that "points" is a poor name, and that it ought to be renamed.

"length" seems reasonable. My only question is whether we might be better off naming things "px" (or "Pixels") where that is explicitly documented to be a "logical pixel" (as px is in CSS). My understanding is that the unit will be a "px" regardless of whether it is named that.

"Length" does make sense in that it fits with "LengthPercentage" and "LengthPercentage" which we already have in the codebase. I'm just not sure that people will intuitively understand it in the way that they will know what a pixel is.

Thoughts?

@alice-i-cecile
Copy link
Collaborator

I think Length is clearer: it makes it more obvious that this is an abstract unit of measure. I don't think that Pixel communicates that effectively to most users, who lack the detailed familiarity with the CSS logical pixel.

@SimonSapin
Copy link
Contributor Author

And Taffy::enable_rounding tends toward the internal unit being device pixel more often than logical pixel

@nicoburns
Copy link
Collaborator

I think Length is clearer: it makes it more obvious that this is an abstract unit of measure. I don't think that Pixel communicates that effectively to most users, who lack the detailed familiarity with the CSS logical pixel.

It's not only CSS that deals with logical pixels: anything that does supports hidpi will have a similar concept. I'm a little concerned that Length doesn't communicate that it's an "absolute unit" at all unless one is familiar with CSS spec terminology. But I'm happy to go with Length if that's the consensus.

And Taffy::enable_rounding tends toward the internal unit being device pixel more often than logical pixel

I was thinking that having pixel_ratio enables us to do the opposite: we have "logical in, physical out". And as rounding happens as a separate tree pass at the end, we can keep everything in logical coordinates until that stage. That also minimises the cost of conversion, as we only have to convert output size/position (four f32s per node), whereas otherwise we have to convert every style. We'll presumably also want to pass logical units to "measure functions" (i.e. when integrating with non-taffy layout), so if we're using physical units internally then we'll have to convert back and forth.

@alice-i-cecile
Copy link
Collaborator

I really like the pixel_ratio idea, but that should obviously be a seperate PR.

On the naming topic, I definitely think that Length is clearer, and less likely to confuse users.

@alice-i-cecile
Copy link
Collaborator

Alright @SimonSapin we've come to a consensus that this is a path we'd like to pursue; can you please rebase this or otherwise resolve merge conflicts? :)

@alice-i-cecile alice-i-cecile added usability Make the library more comfortable to use breaking-change A change that breaks our public interface labels Apr 21, 2023
@nicoburns
Copy link
Collaborator

@alice-i-cecile pixel_ratio is currently implemented in https://github.com/DioxusLabs/taffy/pull/460/files (diff will be easier to see once this one is merged), but I agree it probably ought to be standalone.

I do think that these docs:

/// An absolute length in some abstract units. Users of Taffy may define what they correspond
/// to in their application (pixels, logical pixels, mm, etc) as they see fit.

are wrong if we have pixel_ratio. Having that really makes the units "logical pixels" not abstract units that could be something like mm, etc. But perhaps we can address that in a PR implementing pixel_ratio.

But yes, happy to see this merged once conflicts are resolved :)

⚠️ This is a fairly disruptive breaking changes,
although the fix for users is mostly search-and-replace.

The old name for this one-dimensional length:

* Suggests PostScript points, but is documented as an abstract unit:
  “Users of Taffy may define what they correspond to in their application
   (pixels, logical pixels, mm, etc) as they see fit.”

* Is spelled similar to the two-dimensional `Point` though its meaning is unrelated.

* In CSS syntax, `1px = 0.75pt` but the former is much more commonly used.
  When [parsing CSS into a `Style`](DioxusLabs#440)
  we’ll probably want the default mapping to be `1px` => 1f32 abstract unit,
  so naming the abstract unit "points" would be confusing.
@SimonSapin
Copy link
Contributor Author

Rebased!

I'm a little concerned that Length doesn't communicate that it's an "absolute unit" at all

Right, I also thought of "absolute" as an alternative name for that reason. But then it wouldn’t say at all that it’s a one-dimensional measure of space. It would rely entirely on context for that.

@nicoburns nicoburns merged commit 55abeaf into DioxusLabs:main Apr 21, 2023
@SimonSapin SimonSapin deleted the unpoint branch April 21, 2023 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A change that breaks our public interface usability Make the library more comfortable to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants