-
-
Notifications
You must be signed in to change notification settings - Fork 297
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
fix(windows): set physical values in bounds method #1299
fix(windows): set physical values in bounds method #1299
Conversation
Thanks for helping fix the bug! Could you a change file before I can merge it? |
@wusyong |
src/webview2/mod.rs
Outdated
let dpi = unsafe { util::hwnd_dpi(self.hwnd) }; | ||
let scale_factor = util::dpi_to_scale_factor(dpi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need these, since we can just use Physical position without converting to logical
src/webview2/mod.rs
Outdated
) | ||
.into(); | ||
bounds.position = PhysicalPosition::new(position_point[0].x, position_point[0].y) | ||
.to_logical::<f64>(scale_factor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to convert to logical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amrbashir
tauri-apps/tauri#10053
Have you seen this issue?
This is caused by not converting to logical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the field position
and size
in Rect
are of type Position
and Size
which can be constructed directly from Physical
or Logical
type, it is the responsibility of the user to convert the position/size later to their preferred variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do
Rect {
position: PhysicalPositon::new().into(),
}
as well as:
Rect {
position: LogicalPosition::new().into(),
}
and the user of .bounds()
function can convert to their needed types:
let bounds = webview.bounds()?;
let position = bounds.position.to_logical(scale_factor);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what you mean?
bounds.position = PhysicalPosition::new(position_point[0].x, position_point[0].y).into();
The original code is wrong because it uses logical, right?
bounds.position = LogicalPosition::new(position_point[0].x, position_point[0].y).into();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is what I mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still haven't tested this so I am not sure yet if the correct value should be LogicalPosition
or PhysicalPosition
or maybe the bug could in set_bounds
not bounds
where it should be passing logical values to win32 APIs instead of physical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a bit confused by the definition of Logical
lol
- (set) SetWindowPos
in set_bound
should be Logical
, meaning it should have DPI awareness.
- (set)
SetWindowPos
inset_bound
should bePhysical
, since our app is DPI unawareness. - (get) The position we get from
GetClientRect
should bePhysical
since our app is DPI unawareness. (not quite sure)
- So the LogicalPosition
for SetWindowPos
should be the PhysicalPosition
from MapWindowPoints
* dpi_scale_factor
, which gets a larger value if the DPI is higher.
It looks different from macOS's implementation because macOS will get a smaller coordinate if the DPI is higher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what we get from WIN32 API is already scaled now 😢 , but I think we should use Logical
in set_bounds
Update: oh, I forgot we are DPI unawareness, so it should be Physical
, Windows will scale it by the DPI factor
src/webview2/mod.rs
Outdated
} | ||
|
||
bounds.size = PhysicalSize::new(rect.right - rect.left, rect.bottom - rect.top) | ||
.to_logical::<f64>(scale_factor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to convert to logical
549648d
to
ff569a9
Compare
@amrbashir |
2d472d8
to
49acdd9
Compare
Sorry I don't understand the process, is there anything else I should do? |
@amrbashir |
Apologies, I got busy with some other stuff, I will review and test today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
Package Changes Through 566b53bThere are 1 changes which include wry with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
@amrbashir |
wry/src/webview2/mod.rs
Lines 1193 to 1198 in f56ae29
bounds.position/size expects Logical position/size, but position_point and rect are Physical values.
So conversion is necessary.
closes tauri-apps/tauri#10053