-
-
Notifications
You must be signed in to change notification settings - Fork 405
refactor: View Range window layout and bindings #2866
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
Conversation
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.
PR Summary:
This PR transforms the ViewRange tool from a read-only visualization window into an interactive editor. The XAML refactor introduces:
- Editable TextBox inputs for modifying view range plane offsets
- ComboBox controls for selecting associated levels
- Action buttons (Apply Changes, Reset to Original)
- Enhanced layout with column headers and warning message display
Review Summary:
Review identified 3 critical/high severity issues that prevent this PR from functioning. The most critical issue is that the XAML introduces extensive new data bindings and event handlers that have no corresponding Python backend implementation in script.py. Additionally, the refactor introduces a bug that hardcodes units to "feet" (breaking the existing dynamic unit support), and uses an incorrect property path for ComboBox level selection that will fail in IronPython with Revit API objects.
The review leveraged knowledge of pyRevit's WPF forms architecture, IronPython 2.7 constraints, and Revit API patterns to validate the XAML bindings against the Python backend.
Follow-up suggestions:
| <TextBlock Grid.Row="1" Grid.Column="1" Text="Top Plane" VerticalAlignment="Center" Margin="5,0"/> | ||
| <TextBlock Grid.Row="1" Grid.Column="2" Text="{Binding topplane_elevation}" VerticalAlignment="Center" Margin="5,0"/> | ||
| <TextBox Grid.Row="1" Grid.Column="3" x:Name="topplane_input" | ||
| Text="{Binding topplane_new_value, UpdateSourceTrigger=PropertyChanged}" |
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.
Missing Python backend implementation for all new bindings and functionality
The XAML introduces multiple new data bindings that have no corresponding implementation in script.py:
Missing reactive properties in MainViewModel:
topplane_new_value,cutplane_new_value,bottomplane_new_value,viewdepth_new_valuetopplane_level_id,bottomplane_level_id,viewdepth_level_idavailable_levels,cutplane_level_namecan_modify_view,warning_message
Missing event handlers in MainWindow:
apply_changes_click(line 100)reset_values_click(line 102)
Missing business logic:
- Level collection and filtering
- Offset calculation and validation
- Transaction handling for applying changes
- Error handling and validation messages
This PR cannot function as-is and will cause runtime binding errors when the window loads. The Python backend implementation needs to be included for this refactor to be complete.
Action required: Either:
- Include the complete script.py changes in this PR, or
- If this is part of a multi-PR implementation, mark it as draft and link to the related PRs
actions
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
...ools.extension/pyRevit.tab/Toggles.panel/toggles3.stack/ViewRange.pushbutton/MainWindow.xaml
Outdated
Show resolved
Hide resolved
...ools.extension/pyRevit.tab/Toggles.panel/toggles3.stack/ViewRange.pushbutton/MainWindow.xaml
Outdated
Show resolved
Hide resolved
…ggles3.stack/ViewRange.pushbutton/MainWindow.xaml Co-authored-by: devloai[bot] <168258904+devloai[bot]@users.noreply.github.com>
…ggles3.stack/ViewRange.pushbutton/MainWindow.xaml Co-authored-by: devloai[bot] <168258904+devloai[bot]@users.noreply.github.com>
…UI with new layout and input fields for plane elevations and associated levels. Added functionality for applying changes and resetting values, along with warning messages for user feedback.
|
ported changes in #2865 |
|
📦 New work-in-progress (wip) builds are available for 5.2.0.25307+2011-wip |
|
📦 New work-in-progress (wip) builds are available for 5.2.0.25307+2045-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.0.25307+2237-wip |
|
📦 New work-in-progress (wip) builds are available for 5.3.0.25308+0721-wip |
|
📦 New public release are available for 5.3.0.25307+2146 |
Refactor View Range window layout and bindings
Description
This pull request enhances the ViewRange tool's user experience by improving interface:
Key Improvements:
Checklist
Before submitting your pull request, ensure the following requirements are met:
pipenv run black {source_file_or_directory}Related Issues
If applicable, link the issues resolved by this pull request:
Additional Notes
User Impact:
Thank you for contributing to pyRevit! 🎉