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 LogicalPoint, PhysicalPoint and Point #3529

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

amrbashir
Copy link
Contributor

@amrbashir amrbashir commented Feb 26, 2024

part of #2395

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@daxpedda daxpedda added the S - api Design and usability label Feb 28, 2024
@daxpedda
Copy link
Member

Looking at #2395 I don't really follow what the motivation here is exactly.
Is there some previous related discussion?

@amrbashir
Copy link
Contributor Author

amrbashir commented Feb 28, 2024

#2395 is not implemented in winit (yet?) but already implemented in tao here and the idea is to be able to only constraint the min-width while keeping other window dimensions unconstrainted.

Once this new dpi crate is published, we can use it not only in tao but in a number of other crates we have along with the main tauri crate itself. This PR is a blocker for tao only not other crates

@daxpedda
Copy link
Member

My question is specifically how does Logical/PhysicalPoint relate to that? Why can that not be done with the types we have now?

@amrbashir
Copy link
Contributor Author

Because the existing types require setting both width/height, and so you can't say I want to constraint only width or only height.

@daxpedda
Copy link
Member

Ah, I see, thank you!

@daxpedda daxpedda added the C - nominated Nominated for discussion in the next meeting label Feb 28, 2024
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Should probably update the docs in ### Position and Size types with the point stuff.

@kchibisov
Copy link
Member

Besides.

@amrbashir given that you had an interest in such crate in the first place, how you deal with e.g. rounding? For example in winit we have special rounding rules on Wayland, because it's rounding is a bit special, but maybe we should define such things in this crate as well, so the to_logical/to_physical will account for rounding and make the default like it was before?

@amrbashir
Copy link
Contributor Author

We didn't give it much thought, since our windows and macOS backend were forked from winit and the current behavior of to_logical/to_physical have been working fine for us.

@amrbashir
Copy link
Contributor Author

I just realized that the naming Point might be confusing when used with width/height as it conveys position than a dimension, maybe it should be renamed to LogicalUnit, PhysicalUnit and Unit or PixelUnit ?

@kchibisov
Copy link
Member

I just realized that the naming Point might be confusing when used with width/height as it conveys position than a dimension, maybe it should be renamed to LogicalUnit, PhysicalUnit and Unit or PixelUnit ?

Maybe LogicalUnit/PhysicalUnit is better.

dpi/CHANGELOG.md Show resolved Hide resolved
@kchibisov kchibisov force-pushed the feat/dpi-single-unit branch from b637001 to bdeeaa2 Compare March 8, 2024 16:05
@kchibisov kchibisov merged commit 563b0bf into rust-windowing:master Mar 8, 2024
52 checks passed
@kchibisov
Copy link
Member

Thanks

@madsmtm madsmtm removed the C - nominated Nominated for discussion in the next meeting label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - api Design and usability
Development

Successfully merging this pull request may close these issues.

5 participants