Add `Context::parent_viewport_id_of` #4217

rustbasic · 2024-03-23T13:39:48Z

Closes ViewportInfo.parent doesn't always store any information. #4216

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

and

addition ViewportInfo.this

emilk · 2024-03-25T10:08:55Z

crates/egui/src/context.rs

@@ -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 {


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 {

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

emilk · 2024-03-25T10:12:12Z

crates/egui/src/data/input.rs

@@ -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>,


This is unused?

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

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.

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.

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

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.

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

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.

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.

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>

emilk · 2024-03-29T11:21:42Z

crates/egui/src/data/input.rs

@@ -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>,


Suggested change

pub this: Option<ViewportId>,

pub this: ViewportId,

emilk · 2024-03-30T18:23:43Z

crates/egui/src/data/input.rs

@@ -176,8 +176,11 @@ pub enum ViewportEvent {
 #[derive(Clone, Debug, Default, PartialEq)]


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 added 11 commits March 23, 2024 15:37

Update memory.rs

f0b7737

Update epi_integration.rs

3b98d7f

Update memory.rs

af5a7bb

Update memory.rs

7f66b56

Update epi_integration.rs

6396c4a

Update context.rs

1282780

Update input.rs

0256e50

Update glow_integration.rs

2078da3

Update wgpu_integration.rs

f25bfe1

Update wgpu_integration.rs

c22a9ba

Update input.rs

5c19e80

rustbasic mentioned this pull request Mar 23, 2024

Fix: Viewport Normal operation & Shaking when Resize #4182

Closed

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

emilk requested changes Mar 25, 2024

View reviewed changes

emilk changed the title ~~fix : ViewportInfo.parent doesn't always store any information.~~ Add Context::viewport_parent_id_of Mar 25, 2024

Update crates/egui/src/context.rs

d371cdc

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>

rustbasic requested a review from emilk March 25, 2024 11:33

rustbasic added 8 commits March 25, 2024 23:01

Merge branch 'emilk:master' into patch30

404dcb6

Update context.rs

61953dc

Update context.rs

afd7555

Update glow_integration.rs

893973a

Update wgpu_integration.rs

9a23df7

Update context.rs

9cd270b

Update wgpu_integration.rs

b232e5d

Update glow_integration.rs

559a410

rustbasic changed the title ~~Add Context::viewport_parent_id_of~~ Add Context::parent_viewport_id_of Mar 26, 2024

rustbasic added 4 commits March 26, 2024 20:31

Merge branch 'emilk:master' into patch30

47eaebf

Merge branch 'emilk:master' into patch30

b474de0

Merge branch 'emilk:master' into patch30

f96d170

Merge branch 'emilk:master' into patch30

2c9ade8

emilk reviewed Mar 29, 2024

View reviewed changes

rustbasic requested a review from emilk March 29, 2024 12:30

rustbasic added 7 commits March 29, 2024 21:35

Update input.rs

4f9cff1

Update glow_integration.rs

69b517c

Update wgpu_integration.rs

a487a19

Merge branch 'emilk:master' into patch30

c886a94

Merge branch 'emilk:master' into patch30

8b36cc7

Merge branch 'emilk:master' into patch30

e31bdd1

Merge branch 'emilk:master' into patch30

afbf277

emilk reviewed Mar 30, 2024

View reviewed changes

rustbasic added 3 commits March 31, 2024 09:57

Merge branch 'emilk:master' into patch30

d0a1e22

Update input.rs

232453e

Update input.rs

41f79e0

rustbasic requested a review from emilk March 31, 2024 03:06

rustbasic added 9 commits April 1, 2024 18:08

Merge branch 'emilk:master' into patch30

3d911d1

Merge branch 'emilk:master' into patch30

8bc544b

Merge branch 'emilk:master' into patch30

bf686d3

Merge branch 'emilk:master' into patch30

f413a94

Merge branch 'emilk:master' into patch30

d252bc6

Merge branch 'emilk:master' into patch30

af00549

Merge branch 'emilk:master' into patch30

aef721f

Merge branch 'emilk:master' into patch30

d91dd92

Merge branch 'emilk:master' into patch30

8f45660

emilk marked this pull request as draft April 21, 2024 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Context::parent_viewport_id_of` #4217

Add `Context::parent_viewport_id_of` #4217

rustbasic commented Mar 23, 2024 •

edited

emilk Mar 25, 2024

rustbasic Mar 26, 2024

emilk Mar 25, 2024

rustbasic Mar 25, 2024 •

edited

rustbasic Mar 25, 2024

emilk Mar 25, 2024

rustbasic Mar 25, 2024

emilk Mar 25, 2024

rustbasic Mar 25, 2024

rustbasic Mar 25, 2024

emilk Mar 29, 2024

emilk Mar 30, 2024

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

		@@ -176,8 +176,11 @@ pub enum ViewportEvent {
		#[derive(Clone, Debug, Default, PartialEq)]

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

Add Context::parent_viewport_id_of #4217

Are you sure you want to change the base?

Add Context::parent_viewport_id_of #4217

Conversation

rustbasic commented Mar 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbasic Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Context::parent_viewport_id_of` #4217

Add `Context::parent_viewport_id_of` #4217

rustbasic commented Mar 23, 2024 •

edited

rustbasic Mar 25, 2024 •

edited