Draw System Outline #550
Draw System Outline #550
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a user preference to control the visibility of the bounding rectangle drawn around music notation systems. The setting is persisted through the application's settings system and can be toggled via the preferences dialog.
Changes:
- Added a new boolean setting
DrawStaffRectanglewith a default value oftrueto preserve existing behavior - Created UI controls in the preferences dialog to allow users to toggle the rectangle visibility
- Integrated the setting into ScoreArea to load and respond to changes dynamically
- Modified the SystemRenderer to conditionally draw the system bounding rectangle based on the user's preference
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| source/app/settings.h | Declares the new DrawStaffRectangle setting |
| source/app/settings.cpp | Defines the DrawStaffRectangle setting with default value of true |
| source/dialogs/preferencesdialog.ui | Adds checkbox UI control for the rectangle drawing preference |
| source/dialogs/preferencesdialog.cpp | Implements loading and saving of the rectangle drawing preference |
| source/app/scorearea.h | Adds getter method and loader for the rectangle drawing setting |
| source/app/scorearea.cpp | Implements rectangle drawing setting loading and change subscription |
| source/painters/systemrenderer.cpp | Conditionally draws system rectangle based on the user preference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
source/app/settings.h
Outdated
| extern const Setting<std::vector<std::string>> RecentFiles; | ||
| extern const Setting<ScoreTheme> Theme; | ||
| extern const Setting<bool> OpenFilesInNewWindow; | ||
| extern const Setting<bool> DrawStaffRectangle; |
There was a problem hiding this comment.
The setting name "DrawStaffRectangle" is misleading. Based on the code context, this setting controls the rectangle around the entire system (which can contain multiple staves), not individual staff rectangles. The comment on line 84 says "Draw the bounding rectangle for the system." Consider renaming this to "DrawSystemRectangle" or "DrawSystemBorder" for clarity. This would require updating:
- The setting name in settings.h/cpp
- The UI label and checkbox names
- The method names in scorearea.h/cpp
- The member variable name
source/dialogs/preferencesdialog.ui
Outdated
| <item row="3" column="0"> | ||
| <widget class="QLabel" name="drawStaffRectangleLabel"> | ||
| <property name="text"> | ||
| <string>Draw Staff Rectangle:</string> |
There was a problem hiding this comment.
This UI label says "Draw Staff Rectangle" but the feature actually controls drawing the rectangle around the entire system (which can contain multiple staves). Consider changing this to "Draw System Rectangle:" or "Draw System Border:" to accurately reflect what the setting does.
| <string>Draw Staff Rectangle:</string> | |
| <string>Draw System Rectangle:</string> |
There was a problem hiding this comment.
Also change to left alignment of elements in the preferences window and larger window
from
to
|
Thanks, the changes look good to me! The naming is the only thing I was unsure about - as the Copilot review also pointed out, the rectangle is actually around the whole system (group of staves), not each individual staff. How about |
done ! |
|
Thanks! |
2 participants
Description of Change(s)
I add the option to draw or not draw black lines around staff. Maybe the name of this option could be changed !