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.

Sign up for GitHub

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

Jump to bottom

Refactor and document Viewport #152

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor and document Viewport #152

wants to merge 2 commits into from

Conversation

matthunz
Copy link
Contributor

  • Replaces Viewport::zoom and Viewport::set_zoom with an exported field
  • Removes Viewport::scale_f64 convenience method
    • My thought here is to keep the codebase minimal while we're still building
  • Docs and general refactors

@matthunz matthunz marked this pull request as ready for review October 27, 2024 20:35
@matthunz matthunz added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 28, 2024
nicoburns
nicoburns requested changes Oct 28, 2024
View reviewed changes
Copy link
Collaborator

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

The documentation additions here are good. The impact of the other changes is IMO negligible (a few single-line methods on Viewport aren't exactly a maintenance burden), but I will accept them (with the requested changes) if you're keen for them.

packages/dioxus-native/src/window.rs
@@ -179,7 +179,7 @@ impl<Doc: DocumentLike> View<Doc> {
let (width, height) = self.viewport.window_size;
self.renderer.render(
self.dom.as_ref(),
self.viewport.scale_f64(),
self.viewport.scale() as _,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have an explicit type (f64) here please. Makes it more obvious why a cast is being done. Some for other uses of _

packages/blitz-dom/src/document.rs
Comment on lines -1135 to +1138
let window_width = self.viewport.window_size.0 as f64 / self.viewport.scale() as f64;
let window_height = self.viewport.window_size.1 as f64 / self.viewport.scale() as f64;
let window_width = self.viewport.window_size.0 as f64 / self.viewport.hidpi_scale as f64;
let window_height = self.viewport.window_size.1 as f64 / self.viewport.hidpi_scale as f64;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this change intentional? It's not equivalent to the previous code (which included the zoom).

If not, then IMO this is actually quite a good argument for having the hidpi_scale and zoom field private as they were before, as it prevents this kind of accidental usage of the wrong value.

If it is, then can you provide a justification for the change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoburns nicoburns nicoburns requested changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthunz @nicoburns