-
Notifications
You must be signed in to change notification settings - Fork 326
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
Set properties to null when none are available #5986
Conversation
@jacob314 if I remember correctly, if a widget doesn't have any layout properties, is the layout explorer supposed to show the layout properties for its closest ancestor? |
packages/devtools_app/lib/src/screens/inspector/layout_explorer/ui/layout_explorer_widget.dart
Outdated
Show resolved
Hide resolved
This sound familiar based on some code that I stumbled upon, it was causing some weird behaviour since all of the parents here don't have layout data. If it turns out that null properties is an expected behaviour then I could look at this a bit deeper to see if I could be more specific to only show the message if no parents have layout info either. |
That is correct. In general I thought that happened automatically as each Element got an associated RenderObject even if that was just the one shared with its ancestor. |
Perhaps this changed with @goderbauer's refactors to support multiple windows. We should rethink what makes sense. Generally what we are looking for is just the RenderObject that best approximates the space the widget takes up. Potentially now we need to go down the tree instead of up but would be good to have a principled design in what we are trying to do. Some special cases like Root might no longer have a real size given multiple windows and that is fine. |
With the multi view changes it makes sense that some widgets do not have layout properties anymore. Prior to this change, every element always had an associated render object (i.e. In the multi view world some parts of the widget tree are no longer backed by render objects. This is due to the fact that we have a single unified widget tree describing the content for all views. To this widget tree, multiple render trees (one for each view) are attached, but some widgets/elements may not have a render object at all (and MyDataProvider(
data: _myAppData,
child: ViewCollection(
views: <Widget>[
View(
view: <...>
child: ColoredBox(color: Colors.green),
),
View(
view: <...>
child: ColoredBox(color: Colors.blue),
),
],
)
); This is a single widget tree defining the content of two Views (one green, one blue). The View widgets each bootstrap their own render trees, so if we were to call
The widgets above the tl;dr: It makes sense to me to disable the layout explorer for widgets that are not backed by a render tree (i.e. widgets that appear above the start of the render tree). The signal I would use is does |
Note, though, that I would expect widgets that are clearly part of a render tree to still show the layout explorer. For example, if the View(
view: <..>
child: MyDataProvider(
data: <..>
child: SizedBox(),
),
); ... I would expect that the layout explorer shows me the layout properties of the |
packages/devtools_app/lib/src/screens/inspector/layout_explorer/ui/layout_explorer_widget.dart
Show resolved
Hide resolved
@jacob314 I've recreated goderbauer's provider example here and noticed that we The parent of the builder is the context that we see, not the child. I've grabbed a screenshot to show what it looks like, does this look like it is working like you would expect it? |
I think this matches my expectation, yes. (I think in my original comment I had up and down mixed up, showing the parent is the right thing, though) |
If you select a widget thats parent is a Row/Column we show that parents and its children. What we try to do is highlight the child that you selected but we clearly aren't doing a good job in that case. Seems like the vertical scroll in the layout explorer is off so you can't see that the child it is scrolled to is the ValueListenableBuilder. |
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.
Addresses #5933
Certain multi view widgets don't have layout properties so the layout explorer wasn't updating properly.
This PR sets the properties to null when none are available and add's some state to communicate that to the layout explorer.
Before Fix
Screen.Recording.2023-07-05.at.11.18.03.AM.mov
After Fix
Screen.Recording.2023-07-05.at.11.16.26.AM-FIXED.mov