[4.7] Video export. Add Page video export style#33030
[4.7] Video export. Add Page video export style#33030mike-spa wants to merge 3 commits intomusescore:4.7from
Conversation
📝 WalkthroughWalkthroughReplaces several legacy video view modes with two modes (PageFull, Flexible); changes default view mode to PageFull; exposes viewMode in export model/UI; updates VideoWriter config and rendering to branch behavior for PageFull (page-fitting scale, centering) and Flexible, and adjusts frame background and painting translation. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as MP4SettingsPage
participant Model as ExportDialogModel
participant Config as VideoExportConfiguration
participant Writer as VideoWriter
participant Renderer as RenderingEngine
User->>UI: choose view mode (PageFull / Flexible)
UI->>Model: setViewMode(mode)
Model->>Config: setViewMode(mode)
Config-->>Model: ack
User->>UI: start export
UI->>Model: requestExport()
Model->>Writer: makeConfig()/startExport()
Writer->>Config: viewMode()
Config-->>Writer: returns mode
alt PageFull
Writer->>Writer: compute page-fitting scale, set canvasDpi & moveToCenter
Writer->>Renderer: render score at computed scale (centered via translate)
Renderer-->>Writer: frame
else Flexible
Writer->>Writer: hide instrument names / VBox
Writer->>Renderer: render score with flexible layout
Renderer-->>Writer: frame
end
Writer-->>UI: export complete / frames written
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();
| RadioButtonGroup { | ||
| orientation: ListView.Vertical | ||
| spacing: 8 | ||
| width: parent.width | ||
|
|
||
| model: root.model ? root.model.availableViewModes().map(function(obj) { | ||
| return { text: obj.text, value: obj.value } | ||
| }) : [] | ||
|
|
||
| delegate: RoundedRadioButton { | ||
| required property var modelData | ||
|
|
||
| text: modelData.text | ||
| checked: root.model.viewMode === modelData.value | ||
| onToggled: root.model.viewMode = modelData.value | ||
| } | ||
| } |
There was a problem hiding this comment.
please add navigation for these buttons
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c408189-52e3-49e1-b134-c525c5ff7118
📒 Files selected for processing (1)
src/project/qml/MuseScore/Project/internal/Export/Mp4SettingsPage.qml
| model: root.model ? root.model.availableViewModes().map(function(obj) { | ||
| return { text: obj.text, value: obj.value } | ||
| }) : [] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider moving the option mapping into the C++ model.
availableViewModes() already returns {value, text} objects (see exportdialogmodel.cpp:618-635), so the .map(...) here is an identity transform. Either drop the .map and use the list directly, or — aligning with prior maintainer feedback on this file — expose the list as a C++ Q_PROPERTY/getter matching the available... pattern used in ExportDialogModel so QML does no transformation.
♻️ Proposed minimal simplification
- model: root.model ? root.model.availableViewModes().map(function(obj) {
- return { text: obj.text, value: obj.value }
- }) : []
+ model: root.model ? root.model.availableViewModes() : []
mike-spa
force-pushed
the
addPageVideoStyle
branch
from
a99514d to
5c63ae8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1f860b3-6371-4bfc-9e4d-4e72725efbbd
📒 Files selected for processing (2)
src/project/qml/MuseScore/Project/internal/Export/Mp4SettingsPage.qmlsrc/project/qml/MuseScore/Project/internal/Export/exportdialogmodel.cpp
| QVariantList ExportDialogModel::availableViewModes() const | ||
| { | ||
| std::map<ViewMode, QString> viewModes { | ||
| { ViewMode::PageFull, muse::qtrc("project/export", "Use page layout") }, | ||
| { ViewMode::Flexible, muse::qtrc("project/export", "Reflow to fit video resolution") }, | ||
| }; | ||
|
|
||
| QVariantList result; | ||
|
|
||
| for (const auto& [value, text] : viewModes) { | ||
| QVariantMap obj; | ||
| obj["value"] = value; | ||
| obj["text"] = text; | ||
| result << obj; | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent value encoding in QVariantMap.
Other similar available...() helpers in this file cast enums to int when populating the QVariantMap (e.g., availableUnitTypes() line 373, musicXmlLayoutTypes() line 864, availableSampleFormats() line 949). Here obj["value"] = value; stores the raw ViewMode enum. Since the Q_PROPERTY viewMode is of type ViewMode and QML compares via ===, this currently works, but aligning with the established pattern avoids subtle mismatches (e.g., JS strict-equality across enum vs int representations) and keeps the codebase consistent.
♻️ Proposed change
for (const auto& [value, text] : viewModes) {
QVariantMap obj;
- obj["value"] = value;
+ obj["value"] = static_cast<int>(value);
obj["text"] = text;
result << obj;
}Also note: iteration over std::map<ViewMode, QString> orders entries by enum value, so the UI display order is coupled to the underlying enum numeric order rather than an explicit desired order. If a specific order (e.g., PageFull first) is desired, consider using an ordered container like std::vector<std::pair<ViewMode, QString>>.
Eism
changed the title
|
Found a crash:
Also, this file has a title page with just two vertical frames and no notation. That page is missing from the exported page view. I think such pages should be included in page view exports even if frames are excluded from "reflow" exports. |
3 participants
Summary by CodeRabbit
New Features
UI/UX
Behavior
Visual