Conversation
📝 WalkthroughWalkthroughThe changes introduce a new view mode system for video export, replacing the existing auto-mode and paged-mode enum values with Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as MP4SettingsPage
participant Model as ExportDialogModel
participant Config as VideoExportConfiguration
participant Writer as VideoWriter
participant Renderer as Rendering Engine
User->>UI: Select ViewMode (PageFull/Flexible)
UI->>Model: setViewMode(mode)
Model->>Config: setViewMode(mode)
Note over Config: Stores selected mode
User->>UI: Click Export
UI->>Model: Trigger export
Model->>Writer: Create VideoWriter with config
Writer->>Config: viewMode()
Config-->>Writer: Return selected mode
alt PageFull Mode
Writer->>Writer: Calculate page-fitting scale
Writer->>Writer: Set moveToCenter for alignment
Writer->>Renderer: Render score at calculated scale
Renderer-->>Writer: Rendered frame
else Flexible Mode
Writer->>Writer: Suppress instrument names/vbox
Writer->>Renderer: Render score with defaults
Renderer-->>Writer: Rendered frame
end
Writer-->>User: Export video
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| width: videoSettingsComp.width | ||
| spacing: 4 | ||
|
|
||
| model: [ |
There was a problem hiding this comment.
let's move it to c++ model, see available... methods in ExportDialogModel for example
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9e5afd0-69fa-45b7-8fbc-9ae2b4a205c7
📒 Files selected for processing (9)
src/importexport/videoexport/internal/videoexportconfiguration.cppsrc/importexport/videoexport/internal/videoexportconfiguration.hsrc/importexport/videoexport/internal/videowriter.cppsrc/importexport/videoexport/internal/videowriter.hsrc/importexport/videoexport/ivideoexportconfiguration.hsrc/project/qml/MuseScore/Project/ExportDialog.qmlsrc/project/qml/MuseScore/Project/internal/Export/Mp4SettingsPage.qmlsrc/project/qml/MuseScore/Project/internal/Export/exportdialogmodel.cppsrc/project/qml/MuseScore/Project/internal/Export/exportdialogmodel.h
|
|
||
| int VideoExportConfiguration::videoStyle() const | ||
| { | ||
| return m_videoStyle ? m_videoStyle.value() : 0; | ||
| } | ||
|
|
||
| void VideoExportConfiguration::setVideoStyle(int v) | ||
| { | ||
| m_videoStyle = v; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a named constant for the default value.
Other configuration values in this file use named constants (e.g., DEFAULT_VIEW_MODE, DEFAULT_FPS). The hard-coded 0 on line 113 would be clearer with a constant.
♻️ Proposed change for consistency
Add at the top of the file with other defaults:
static const int DEFAULT_VIDEO_STYLE = 0; // VideoStyle::PAGEThen update the getter:
int VideoExportConfiguration::videoStyle() const
{
- return m_videoStyle ? m_videoStyle.value() : 0;
+ return m_videoStyle ? m_videoStyle.value() : DEFAULT_VIDEO_STYLE;
}| cfg.leadingSec = configuration()->leadingSec(); | ||
| cfg.trailingSec = configuration()->trailingSec(); | ||
|
|
||
| cfg.style = static_cast<VideoStyle>(configuration()->videoStyle()); |
There was a problem hiding this comment.
Clamp videoStyle before casting.
videoStyle() is an int, so a stale/corrupted setting can produce an enum value that matches neither PAGE nor FLEXIBLE. That drops export into an unintended mixed path below. Default invalid values here instead of static_casting blindly.
🛡️ Proposed fix
- cfg.style = static_cast<VideoStyle>(configuration()->videoStyle());
+ const int rawStyle = configuration()->videoStyle();
+ cfg.style = rawStyle == static_cast<int>(VideoStyle::FLEXIBLE)
+ ? VideoStyle::FLEXIBLE
+ : VideoStyle::PAGE;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cfg.style = static_cast<VideoStyle>(configuration()->videoStyle()); | |
| const int rawStyle = configuration()->videoStyle(); | |
| cfg.style = rawStyle == static_cast<int>(VideoStyle::FLEXIBLE) | |
| ? VideoStyle::FLEXIBLE | |
| : VideoStyle::PAGE; |
|
|
||
| virtual int videoStyle() const = 0; | ||
| virtual void setVideoStyle(int v) = 0; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent setter signature pattern.
Other setters in this interface use std::optional<T> (e.g., setViewMode(std::optional<ViewMode>), setFps(std::optional<int>)), but setVideoStyle takes a plain int. This breaks the established pattern and prevents resetting to default.
Consider aligning with the existing convention:
♻️ Proposed change for consistency
- virtual void setVideoStyle(int v) = 0;
+ virtual void setVideoStyle(std::optional<int> v) = 0;This would also require updating the implementation in VideoExportConfiguration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| virtual int videoStyle() const = 0; | |
| virtual void setVideoStyle(int v) = 0; | |
| virtual int videoStyle() const = 0; | |
| virtual void setVideoStyle(std::optional<int> v) = 0; |
| ExportOptionItem { | ||
| id: videoStyleLabel | ||
| text: qsTrc("project/export", "Video style") | ||
|
|
||
| RadioButtonGroup { | ||
| width: videoSettingsComp.width | ||
| spacing: 4 | ||
|
|
||
| model: [ | ||
| { text: qsTrc("project/export", "Page"), value: 0 }, | ||
| { text: qsTrc("project/export", "Flexible"), value: 1 } | ||
| ] | ||
|
|
||
| delegate: FlatRadioButton { | ||
| width: 126 | ||
| height: 30 | ||
|
|
||
| text: modelData.text | ||
| checked: modelData.value === root.model.videoStyle | ||
| onToggled: root.model.videoStyle = modelData.value | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing null guard on root.model access.
The radio button bindings access root.model.videoStyle without null checks (lines 156-157), unlike other controls in this file that guard with root.model ? (e.g., lines 126, 130). This could cause errors if the model is not yet initialized.
🛡️ Proposed fix to add null guard
delegate: FlatRadioButton {
width: 126
height: 30
text: modelData.text
- checked: modelData.value === root.model.videoStyle
- onToggled: root.model.videoStyle = modelData.value
+ checked: root.model ? modelData.value === root.model.videoStyle : false
+ onToggled: {
+ if (root.model) {
+ root.model.videoStyle = modelData.value
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ExportOptionItem { | |
| id: videoStyleLabel | |
| text: qsTrc("project/export", "Video style") | |
| RadioButtonGroup { | |
| width: videoSettingsComp.width | |
| spacing: 4 | |
| model: [ | |
| { text: qsTrc("project/export", "Page"), value: 0 }, | |
| { text: qsTrc("project/export", "Flexible"), value: 1 } | |
| ] | |
| delegate: FlatRadioButton { | |
| width: 126 | |
| height: 30 | |
| text: modelData.text | |
| checked: modelData.value === root.model.videoStyle | |
| onToggled: root.model.videoStyle = modelData.value | |
| } | |
| } | |
| } | |
| ExportOptionItem { | |
| id: videoStyleLabel | |
| text: qsTrc("project/export", "Video style") | |
| RadioButtonGroup { | |
| width: videoSettingsComp.width | |
| spacing: 4 | |
| model: [ | |
| { text: qsTrc("project/export", "Page"), value: 0 }, | |
| { text: qsTrc("project/export", "Flexible"), value: 1 } | |
| ] | |
| delegate: FlatRadioButton { | |
| width: 126 | |
| height: 30 | |
| text: modelData.text | |
| checked: root.model ? modelData.value === root.model.videoStyle : false | |
| onToggled: { | |
| if (root.model) { | |
| root.model.videoStyle = modelData.value | |
| } | |
| } | |
| } | |
| } | |
| } |
mike-spa
force-pushed
the
addPageVideoStyle
branch
from
59a8c25 to
41de519
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/project/qml/MuseScore/Project/internal/Export/Mp4SettingsPage.qml (1)
147-153:⚠️ Potential issue | 🟡 MinorMissing null guard on
root.modelaccess.The delegate bindings access
root.model.viewModewithout null checks (lines 151-152), unlike other controls in this file that guard withroot.model ?(e.g., lines 126, 130, 143). This could cause errors if the model is not yet initialized.🛡️ Proposed fix to add null guard
delegate: RoundedRadioButton { required property var modelData text: modelData.text - checked: root.model.viewMode === modelData.value - onToggled: root.model.viewMode = modelData.value + checked: root.model ? root.model.viewMode === modelData.value : false + onToggled: { + if (root.model) { + root.model.viewMode = modelData.value + } + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca1438b8-6342-457b-8014-6e1322fd569a
📒 Files selected for processing (8)
src/importexport/videoexport/internal/videoexportconfiguration.cppsrc/importexport/videoexport/internal/videowriter.cppsrc/importexport/videoexport/internal/videowriter.hsrc/importexport/videoexport/videoexporttypes.hsrc/project/qml/MuseScore/Project/ExportDialog.qmlsrc/project/qml/MuseScore/Project/internal/Export/Mp4SettingsPage.qmlsrc/project/qml/MuseScore/Project/internal/Export/exportdialogmodel.cppsrc/project/qml/MuseScore/Project/internal/Export/exportdialogmodel.h
| if (config.viewMode == ViewMode::PageFull) { | ||
| double scaleX = config.width / page->width(); | ||
| double scaleY = config.height / page->height(); | ||
| double scale = std::min(scaleX, scaleY); | ||
| config.canvasDpi = scale * engraving::DPI; | ||
| config.moveToCenter = muse::PointF(0.5 * (config.width / scale - page->width()), 0.5 * (config.height / scale - page->height())); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider guarding against zero page dimensions.
The scaling calculation divides by page->width() and page->height(). While pages should always have positive dimensions after doLayout(), a defensive check would prevent potential division by zero if layout fails silently.
🛡️ Proposed defensive check
if (config.viewMode == ViewMode::PageFull) {
+ if (page->width() <= 0 || page->height() <= 0) {
+ LOGE() << "Invalid page dimensions";
+ restoreScore(notation, result);
+ return std::nullopt;
+ }
double scaleX = config.width / page->width();
double scaleY = config.height / page->height();2 participants
Summary by CodeRabbit
New Features
UI/UX