Add transparency option to PDF export #25407 #25688

LeoTaoeee · 2024-11-29T20:35:58Z

Resolves: #25407

I signed the CLA
The title of the PR describes the problem it addresses
Each commit's message describes its purpose and effects, and references the issue it resolves
If changes are extensive, there is a sequence of easily reviewable commits
The code in the PR follows the coding rules
There are no unnecessary changes
The code compiles and runs on my machine, preferably after each commit individually
I created a unit test or vtest to verify the changes I made (if applicable)

uhm

cbjeukendrup

Thanks for your PR, it looks generally good! A few minor remarks.

cbjeukendrup · 2024-11-29T23:12:26Z

src/importexport/imagesexport/iimagesexportconfiguration.h

Please make sure that there are no unnecessary whitespace changes in this file (and other files)

cbjeukendrup · 2024-11-29T23:13:23Z

src/importexport/imagesexport/internal/imagesexportconfiguration.cpp

@@ -51,6 +52,18 @@ void ImagesExportConfiguration::setExportPdfDpiResolution(int dpi)
    settings()->setSharedValue(EXPORT_PDF_DPI_RESOLUTION_KEY, Val(dpi));
 }

+//pdf with transparency


This comment is not necessary, since the name of the method is already meaningful enough

cbjeukendrup · 2024-11-29T23:13:35Z

src/importexport/imagesexport/internal/imagesexportconfiguration.h

@@ -33,6 +33,10 @@ class ImagesExportConfiguration : public IImagesExportConfiguration
    int exportPdfDpiResolution() const override;
    void setExportPdfDpiResolution(int dpi) override;

+    //pdf with transparent


(Same here)

cbjeukendrup · 2024-11-29T23:15:10Z

src/importexport/imagesexport/internal/pdfwriter.cpp

@@ -65,9 +65,15 @@ Ret PdfWriter::write(INotationPtr notation, io::IODevice& destinationDevice, con
        return false;
    }

+    //transparency handling


This comment doesn't add much information either; we prefer only writing comments when they contain information that cannot (easily) be deduced from the code itself.

cbjeukendrup · 2024-11-29T23:16:12Z

src/importexport/imagesexport/internal/pdfwriter.cpp

+    const bool TRANSPARENT_BACKGROUND = options.find(OptionKey::TRANSPARENT_BACKGROUND) != options.end()
+        ? options.at(OptionKey::TRANSPARENT_BACKGROUND).toBool()
+        : configuration()->exportPdfWithTransparentBackground();


PngWriter uses muse::value for this; might be good to use that here too, for consistency.

cbjeukendrup · 2024-11-29T23:16:36Z

src/importexport/imagesexport/internal/pdfwriter.cpp

+    const bool TRANSPARENT_BACKGROUND = options.find(OptionKey::TRANSPARENT_BACKGROUND) != options.end()
+        ? options.at(OptionKey::TRANSPARENT_BACKGROUND).toBool()
+        : configuration()->exportPdfWithTransparentBackground();


(Same here)

cbjeukendrup · 2024-11-29T23:24:23Z

src/project/view/exportdialogmodel.h

+    Q_PROPERTY(bool pdfTransparentBackground READ pdfTransparentBackground
+               WRITE setPdfTransparentBackground NOTIFY pdfTransparentBackgroundChanged)


Would be good to keep the order of the Q_PROPERTIES consistent, so this one should come after Q_PROPERTY(int pdfResolution …

cbjeukendrup · 2024-11-29T23:26:05Z

src/project/qml/MuseScore/Project/internal/Export/PdfSettingsPage.qml

@@ -55,4 +55,25 @@ ExportSettingsPage {
            }
        }
    }
+
+    // Add transparency option


This comment is not necessary

cbjeukendrup · 2024-11-29T23:28:44Z

src/project/qml/MuseScore/Project/internal/Export/PdfSettingsPage.qml

+
+    StyledTextLabel {
+        width: parent.width
+        text: qsTrc("project/export", "Each page of the selected parts will be exported as a separate %1 file.").arg("PDF")
+    }


I believe this label shouldn't be here; the text is not accurate, and for PDF there are already those radio buttons to select how parts should be exported

LeoTaoeee added 2 commits November 27, 2024 21:47

added transparency to pdf export

f984b77

Merge branch 'master' of https://github.com/musescore/MuseScore

c380adc

uhm

cbjeukendrup changed the title ~~Ads transparency option to PDF export #25407~~ Add transparency option to PDF export #25407 Nov 29, 2024

cbjeukendrup reviewed Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transparency option to PDF export #25407 #25688

Add transparency option to PDF export #25407 #25688

LeoTaoeee commented Nov 29, 2024 •

edited

Loading

cbjeukendrup left a comment

cbjeukendrup Nov 29, 2024

cbjeukendrup Nov 29, 2024

cbjeukendrup Nov 29, 2024

cbjeukendrup Nov 29, 2024

cbjeukendrup Nov 29, 2024

cbjeukendrup Nov 29, 2024

cbjeukendrup Nov 29, 2024

cbjeukendrup Nov 29, 2024

cbjeukendrup Nov 29, 2024

		Q_PROPERTY(bool pdfTransparentBackground READ pdfTransparentBackground
		WRITE setPdfTransparentBackground NOTIFY pdfTransparentBackgroundChanged)

Add transparency option to PDF export #25407 #25688

Are you sure you want to change the base?

Add transparency option to PDF export #25407 #25688

Conversation

LeoTaoeee commented Nov 29, 2024 • edited Loading

cbjeukendrup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeoTaoeee commented Nov 29, 2024 •

edited

Loading