Enhance Properties panel section behavior and persistence#3787
Enhance Properties panel section behavior and persistence#3787jsjgdh wants to merge 2 commits intoGraphiteEditor:masterfrom
Conversation
Summary of ChangesHello @jsjgdh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience of the properties panel by enabling sections to be collapsed and expanded, with their state being persisted. It also introduces convenient shortcuts for managing the expansion of all sections at once, improving navigation and focus within complex property layouts. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a great enhancement to the Properties panel by making sections collapsible and persisting their state. The implementation is well-structured, touching both the backend state management and the frontend UI components. The use of Alt+click to expand/collapse all sections is a nice UX improvement.
My main feedback is regarding the implementation of the "expand/collapse all" feature, which currently has a bug where it doesn't affect sections that haven't been individually toggled. I've left detailed comments with suggestions on how to address this by introducing a global expansion state.
Overall, this is a solid contribution that improves user experience. Addressing the issue I've pointed out will make it even more robust.
| #[derive(Debug, Clone, Default, ExtractField)] | ||
| pub struct PropertiesPanelMessageHandler {} | ||
| pub struct PropertiesPanelMessageHandler { | ||
| pub section_expanded: HashMap<u64, bool>, | ||
| } |
There was a problem hiding this comment.
The current implementation of SetAllSectionsExpanded has a flaw where it only affects sections that have been individually toggled. To fix this, I suggest adding a field to track a global expansion state. I'll leave another comment on the message handlers with more details.
| #[derive(Debug, Clone, Default, ExtractField)] | |
| pub struct PropertiesPanelMessageHandler {} | |
| pub struct PropertiesPanelMessageHandler { | |
| pub section_expanded: HashMap<u64, bool>, | |
| } | |
| #[derive(Debug, Clone, Default, ExtractField)] | |
| pub struct PropertiesPanelMessageHandler { | |
| pub section_expanded: HashMap<u64, bool>, | |
| pub all_sections_expanded: Option<bool>, | |
| } |
| PropertiesPanelMessage::SetAllSectionsExpanded { expanded } => { | ||
| for value in self.section_expanded.values_mut() { | ||
| *value = expanded; | ||
| } | ||
| responses.add(PropertiesPanelMessage::Refresh); | ||
| } | ||
| PropertiesPanelMessage::SetSectionExpanded { node_id, expanded } => { | ||
| self.section_expanded.insert(node_id, expanded); | ||
| responses.add(PropertiesPanelMessage::Refresh); | ||
| } |
There was a problem hiding this comment.
Using the new all_sections_expanded field, this updated logic correctly handles the global expand/collapse behavior.
SetAllSectionsExpandednow acts as a global override by settingall_sections_expandedand clearing individual states.SetSectionExpandedclears the global override, allowing individual control again.
To complete this change, you'll also need to:
- Pass
self.all_sections_expandedtoNodePropertiesContextin theRefreshhandler (this requires addingall_sections_expanded: Option<bool>toNodePropertiesContext). - In
generate_node_properties, prioritizeall_sections_expandedwhen determining a section'sexpandedstate.
| PropertiesPanelMessage::SetAllSectionsExpanded { expanded } => { | |
| for value in self.section_expanded.values_mut() { | |
| *value = expanded; | |
| } | |
| responses.add(PropertiesPanelMessage::Refresh); | |
| } | |
| PropertiesPanelMessage::SetSectionExpanded { node_id, expanded } => { | |
| self.section_expanded.insert(node_id, expanded); | |
| responses.add(PropertiesPanelMessage::Refresh); | |
| } | |
| PropertiesPanelMessage::SetAllSectionsExpanded { expanded } => { | |
| self.all_sections_expanded = Some(expanded); | |
| self.section_expanded.clear(); | |
| responses.add(PropertiesPanelMessage::Refresh); | |
| } | |
| PropertiesPanelMessage::SetSectionExpanded { node_id, expanded } => { | |
| self.all_sections_expanded = None; | |
| self.section_expanded.insert(node_id, expanded); | |
| responses.add(PropertiesPanelMessage::Refresh); | |
| } |
| let is_merge_node = context | ||
| .network_interface | ||
| .reference(&node_id, context.selection_network_path) | ||
| .as_ref() | ||
| .is_some_and(|id| id.implementation_name_from_identifier() == "Merge"); | ||
| let default_expanded = !is_merge_node; |
There was a problem hiding this comment.
This logic needs to occur when originally creating the merge node, like when adding a new layer to the document.
2 participants
discussion : https://discord.com/channels/@me/1461994742291234997/1473618001944252530 from DM with @Keavon