Conversation
crates/egui/src/context.rs
Outdated
| } | ||
|
|
||
| /// Return the `ViewportId` of his parent. | ||
| pub(crate) fn get_parent_viewport_id(&self, viewport_id: ViewportId) -> ViewportId { |
There was a problem hiding this comment.
https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter
| pub(crate) fn get_parent_viewport_id(&self, viewport_id: ViewportId) -> ViewportId { | |
| pub(crate) fn parent_viewport_id_of(&self, viewport_id: ViewportId) -> ViewportId { |
There was a problem hiding this comment.
https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter
what do you want?
parent_viewport_id_of() ? (you're writed here)
viewport_parent_id_of() ? (you're writed title)
crates/egui/src/data/input.rs
Outdated
| #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] | ||
| pub struct ViewportInfo { | ||
| /// this viewport, if known. | ||
| pub this: Option<ViewportId>, |
There was a problem hiding this comment.
This is unused?
I think everywhere we have a ViewportInfo we have a ViewportId for it as the key
There was a problem hiding this comment.
This is used.
parent_viewport_id() returns the parent of the last viewport_id, making it difficult to use unless it's inside a crate.
There was a problem hiding this comment.
This is unused?
I think everywhere we have a
ViewportInfowe have aViewportIdfor it as the key
no. There are places where viewport_id is missing. It's hard to explain.
There was a problem hiding this comment.
Try to explain and/or make this a ids: ViewportIdPair which is set in a constructor, instead of sometimes being None.
There was a problem hiding this comment.
Try to explain and/or make this a
ids: ViewportIdPairwhich is set in a constructor, instead of sometimes beingNone.
There are parts where this is absolutely necessary, and users will be able to easily access it. (In fact, this may not be easy for beginners to access.)
This is absolutely necessary, as there are cases where there is currently no way to determine the viewport_id.
Without this, every time you create a viewport, you would have to save the viewport_id somewhere and take care of it.
There was a problem hiding this comment.
Then add ids: ViewportIdPair to ViewportInfo (and remove the Default bound on it), so users can depend on the ids
There was a problem hiding this comment.
Try to explain and/or make this a
ids: ViewportIdPairwhich is set in a constructor, instead of sometimes beingNone.
If you don't save the viewport_id when creating a viewport, the current window sometimes doesn't know what the current viewport_id is. This is difficult to explain. Currently the viewport_id is unknown, so how can I find the parent_id? This is a different case from examples/demo_viewports.
last viewport_id may not be the current viewport.
There was a problem hiding this comment.
Then add
ids: ViewportIdPairtoViewportInfo(and remove theDefaultbound on it), so users can depend on the ids
info.this is absolutely necessary.
If info.this is present, info.parent does not need to be present.
info.parent pre-existed and could not work.
Context::viewport_parent_id_of
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Context::viewport_parent_id_ofContext::parent_viewport_id_of
crates/egui/src/data/input.rs
Outdated
| #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] | ||
| pub struct ViewportInfo { | ||
| /// this viewport, if known. | ||
| pub this: Option<ViewportId>, |
There was a problem hiding this comment.
| pub this: Option<ViewportId>, | |
| pub this: ViewportId, |
| /// | ||
| /// All units are in ui "points", which can be calculated from native physical pixels | ||
| /// using `pixels_per_point` = [`crate::Context::zoom_factor`] * `[Self::native_pixels_per_point`]; | ||
| #[derive(Clone, Debug, Default, PartialEq)] |
There was a problem hiding this comment.
If we should store the ViewportId in this struct, we should force users to initialize it to the correct thing when they create it via a ViewportInfo::new, otherwise it will be wrong.
| #[derive(Clone, Debug, Default, PartialEq)] | |
| #[derive(Clone, Debug, PartialEq)] |
fix : ViewportInfo.parent doesn't always store any information.
and
addition ViewportInfo.this