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 Context::parent_viewport_id_of #4217

Draft
wants to merge 43 commits into
base: master
Choose a base branch
from
Draft

Conversation

rustbasic
Copy link
Contributor

@rustbasic rustbasic commented Mar 23, 2024

fix : ViewportInfo.parent doesn't always store any information.

and

addition ViewportInfo.this

@rustbasic rustbasic changed the title fiz : ViewportInfo.parent doesn't always store any information. fix : ViewportInfo.parent doesn't always store any information. Mar 24, 2024
@@ -599,6 +599,14 @@ impl ContextImpl {
.unwrap_or(&ViewportId::ROOT)
}

/// Return the `ViewportId` of his parent.
pub(crate) fn get_parent_viewport_id(&self, viewport_id: ViewportId) -> ViewportId {
Copy link
Owner

Choose a reason for hiding this comment

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

https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter

Suggested change
pub(crate) fn get_parent_viewport_id(&self, viewport_id: ViewportId) -> ViewportId {
pub(crate) fn parent_viewport_id_of(&self, viewport_id: ViewportId) -> ViewportId {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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/context.rs Outdated Show resolved Hide resolved
@@ -176,8 +176,11 @@ pub enum ViewportEvent {
#[derive(Clone, Debug, Default, PartialEq)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub struct ViewportInfo {
/// this viewport, if known.
pub this: Option<ViewportId>,
Copy link
Owner

Choose a reason for hiding this comment

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

This is unused?

I think everywhere we have a ViewportInfo we have a ViewportId for it as the key

Copy link
Contributor Author

@rustbasic rustbasic Mar 25, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unused?

I think everywhere we have a ViewportInfo we have a ViewportId for it as the key

no. There are places where viewport_id is missing. It's hard to explain.

Copy link
Owner

Choose a reason for hiding this comment

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

Try to explain and/or make this a ids: ViewportIdPair which is set in a constructor, instead of sometimes being None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try to explain and/or make this a ids: ViewportIdPair which is set in a constructor, instead of sometimes being None.

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Then add ids: ViewportIdPair to ViewportInfo (and remove the Default bound on it), so users can depend on the ids

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try to explain and/or make this a ids: ViewportIdPair which is set in a constructor, instead of sometimes being None.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then add ids: ViewportIdPair to ViewportInfo (and remove the Default bound 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.

@emilk emilk changed the title fix : ViewportInfo.parent doesn't always store any information. Add Context::viewport_parent_id_of Mar 25, 2024
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@rustbasic rustbasic requested a review from emilk March 25, 2024 11:33
@rustbasic rustbasic changed the title Add Context::viewport_parent_id_of Add Context::parent_viewport_id_of Mar 26, 2024
@@ -176,8 +176,11 @@ pub enum ViewportEvent {
#[derive(Clone, Debug, Default, PartialEq)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub struct ViewportInfo {
/// this viewport, if known.
pub this: Option<ViewportId>,
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
pub this: Option<ViewportId>,
pub this: ViewportId,

@rustbasic rustbasic requested a review from emilk March 29, 2024 12:30
@@ -176,8 +176,11 @@ pub enum ViewportEvent {
#[derive(Clone, Debug, Default, PartialEq)]
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Suggested change
#[derive(Clone, Debug, Default, PartialEq)]
#[derive(Clone, Debug, PartialEq)]

@rustbasic rustbasic requested a review from emilk March 31, 2024 03:06
@emilk emilk marked this pull request as draft April 21, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ViewportInfo.parent doesn't always store any information.
2 participants