fix #303756 and #293370: export to pdf/png/svg with the option to use the page color or the wallpaper #5935

SKefalidis · 2020-04-13T23:03:46Z

Resolves: https://musescore.org/en/node/303756 and https://musescore.org/en/node/293370

Added the ability to use the page color when exporting to PDF/PNG/SVG. The page color still is a preference. I could change that and make it part of each Page object (as proposed in the issue). Opinions on whether I should do that, or leave the page color as a preference are welcome :^)

Exporting the wallpaper seems a bit more problematic. The ScoreView changes the size of the displayed wallpaper depending on the paintEvent and zoom level. At the same time, changing the exported file depending on the zoom level in the program seems weird. I decided to use the size of the page which at least leads to consistent behavior when exporting.

Page Color:

Page Wallpaper:

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
I created the test (mtest, vtest, script test) to verify the changes I made

Jojo-Schmitz · 2020-04-14T06:06:20Z

global/settings/types/preferencekeys.h

@@ -112,6 +112,7 @@
 #define PREF_UI_CANVAS_FG_USECOLOR_IN_PALETTES              "ui/canvas/foreground/useColorInPalettes"
 #define PREF_UI_CANVAS_BG_COLOR                             "ui/canvas/background/color"
 #define PREF_UI_CANVAS_FG_COLOR                             "ui/canvas/foreground/color"
+#define PREF_UI_CANVAS_FG_EXPORT_PDF                        "ui/canvas/foreground/export/pdf"


Isn't that rather "background"?

And why restrict this to PDF and not having it for PNG, SVG, and possibly MusicXML too?

I don't think that it is "background". The background color is the one behind the page.

As far as MusicXML is concerned, I will probably make some decently sized changes in the MusicXML code, so I'll add png and svg for now.

Forground color is that of notes, staff lines etc, (black by default), background is white by default and your PR allows to change that. And not just for PDF anymore now, it seems

The background color is dark-blue by default and the paper color is white.

The paper color is called fgColor in prefsdialog.*

You are probably right, but I seem unable to understand my mistake. I am essentially copying the implementation of useTheSameColorInPalettes (I should change the name of my macro).

SKefalidis · 2020-04-14T09:03:05Z

Added the option for svg and png.

SKefalidis · 2020-04-14T09:09:47Z

Fixed formatting.

Jojo-Schmitz · 2020-04-14T09:53:33Z

The vtests are not happy. Some ore screwed up pretty badly

Jojo-Schmitz · 2020-04-14T10:04:48Z

Hmm, seems the default paper color is #f9f9f9 and not white or #ffffff?

SKefalidis · 2020-04-14T10:08:06Z

I might have caused it by changing the way the pages are painted in svg (for uniformity). I reverted those changes and I am waiting for the new vtest.

Jojo-Schmitz · 2020-04-14T10:08:49Z

mscore/preferences.cpp

@@ -158,6 +158,7 @@ void Preferences::init(bool storeInMemoryOnly)
            {PREF_UI_CANVAS_FG_USECOLOR_IN_PALETTES,               new BoolPreference(false, false)},
            {PREF_UI_CANVAS_BG_COLOR,                              new ColorPreference(QColor("#142433"), false)},
            {PREF_UI_CANVAS_FG_COLOR,                              new ColorPreference(QColor("#f9f9f9"), false)},


I guess we'd need to interpret this color as white?

Maybe not, but why then are the vtests show background color artifacts?

vtests are happy now

Jojo-Schmitz · 2020-04-14T10:10:14Z

mscore/file.cpp

@@ -2225,6 +2234,13 @@ bool MuseScore::savePdf(QList<Score*> cs_, const QString& saveName)
                  if (!firstPage)
                        printer.newPage();
                  firstPage = false;
+
+                  if(usePageColor) {


space missing

Jojo-Schmitz · 2020-04-14T10:10:23Z

mscore/file.cpp

      for (int n = 0; n < pages; ++n) {
            if (!firstPage)
                  printer.newPage();
            firstPage = false;
+
+            if(usePageColor) {


space missing

Philzen · 2020-04-14T13:51:49Z

Awesome that you're taking this up 👍

The page color still is a preference. I could change that and make it part of each Page object (as proposed in the issue).

If it's not too much of a kerfuffle, i would go for that. It seems only logical for preferences to be concerned only with application wide-settings (such as the background color of the viewport), while page-settings ("paper color") would be a score property (as <pageWidth>, <pageHeight>, etc. are also properties of the generated score XML).

Having said that, it may still be useful to leave it in the preferences as "Default Paper color" that will be used when no individual paper color was defined for the score.

I cannot judge the gravity of such a change - it could as well be done as a second iteration. Anyway i'd like to hear what the veterans have to say.

Philzen · 2020-04-14T14:22:16Z

mscore/file.cpp

      for (int n = 0; n < pages; ++n) {
            if (!firstPage)
                  printer.newPage();
            firstPage = false;
+
+            if (usePageColor) {


I sense a code smell here. The code itself looks fine to me, but the fact that it's is copied and pasted two more times does not (as it contradicts the DRY principle).

Suggestion would be to move those lines into a private subroutine (i.e. something like applyPageColor(Printer, pageNum)) and just call that.

But i can see why that happened: At first glance it looks to me as if savePdf(Score* cs_, QPrinter& printer) and savePdf(QList<Score*> cs_, const QString& saveName) already have a certain amount of code duplication (if i'm not misunderstanding something here and they do something completely different), i'd advise to refactor this at some point in the near future to reduce technical debt and make the code more stable and easier to maintain.

Taken care of :-)

Philzen · 2020-04-14T14:43:07Z

One more thought: The Preferences call it "Paper", the variable used in the pull request under discussion refers to it as "Page" (as we're also discussing whether to move the UI component to "Page(s) Settings", while the Qt IDs refer to it as "Foreground") ;)

"Paper" is a nice metaphore, but maybe not the ideal one as we're talking about "virtual" paper at best here...

Suggestion: rename the UI setting to "Page Color" to reduce ambiguity?

Philzen · 2020-04-18T18:50:16Z

That new "Export" checkbox is unnecessary imho:

It may introduce confusion, as it deals with exporting and thus would logically belong onto the "Export" tab - which has a similar checkbox for PNG already, which (as pointed out in #303756) currently only gives the option to export in (partially broken) transparency or plain white (a limitation that the PR under discussion fixes).

If we get #303760 implemented in a followup increment, i believe that would solve current potential user confusion in their export workflow as it would provide a clearer, less ambiguous way of configuring exports ad-hoc, so right now my suggestion is to have this PR just include 1. + 2. of the following list:

page (paper) background color export ✔️
rename "paper" to "page" color in the UI
(optionally) move that setting into the page settings dialog, given everybody agrees this is the way to go. I understand this also requires to move it out of the PREFS_UI* context so it becomes a score property, and that it must then also be stored in the individual score file. But this is only one of three possible variants i can think of (a).

Other variants of 3. would be not removing the preference option but renaming it to either (b) "page display color" or (c) "page default color", where (b) would make it clear that it's just the color it's displayed in, but not the actual page color (which would then be defined in score page settings), or (c) where it would be the default page color for new scores being created.

So maybe that would need some discussion and find an agreement in the design chat first.

SKefalidis · 2020-04-18T20:30:09Z

Added wallpaper export.

SKefalidis · 2020-04-18T20:36:21Z

That new "Export" checkbox is unnecessary imho:

It may introduce confusion, as it deals with exporting and thus would logically belong onto the "Export" tab - which has a similar checkbox for PNG already, which (as pointed out in #303756) currently only gives the option to export in (partially broken) transparency or plain white (a limitation that the PR under discussion fixes).

If we get #303760 implemented in a followup increment, i believe that would solve current potential user confusion in their export workflow as it would provide a clearer, less ambiguous way of configuring exports ad-hoc, so right now my suggestion is to have this PR just include 1. + 2. of the following list:
1. page (paper) background color export heavy_check_mark

2. rename "paper" to "page" color in the UI

3. (optionally) move that setting into the page settings dialog, given everybody agrees this is the way to go. I understand this also requires to move it out of the PREFS_UI* context so it becomes a score property, and that it must then also be stored in the individual score file. But this is only one of three possible variants i can think of (a).
Other variants of 3. would be not removing the preference option but renaming it to either (b) "page display color" or (c) "page default color", where (b) would make it clear that it's just the color it's displayed in, but not the actual page color (which would then be defined in score page settings), or (c) where it would be the default page color for new scores being created.

So maybe that would need some discussion and find an agreement in the design chat first.

I would like to keep this PR about adding the functionality and leave the proposed UI changes (with which I agree) for another PR.

Jojo-Schmitz · 2020-04-19T06:25:00Z

mscore/file.cpp

+                        QPixmap* pm = new QPixmap(preferences.getString(PREF_UI_CANVAS_FG_WALLPAPER));
+                        p.drawTiledPixmap(pl.at(n)->pageBoundingRect(),
+                                          *pm,
+                                          pl.at(n)->pageBoundingRect().topLeft() - QPoint(lrint(mscore->currentScoreView()->matrix().dx()), lrint(mscore->currentScoreView()->matrix().dy())));


Add one more line break there before the 4th arg.

There is no 4th argument, but I see that it is quite convoluted. It should be clearer now.

Hm... the third argument might be both useless and an obstacle in the future, I'll remove it. But if we want the exported wallpaper to be the same as in MuseScore I'll need to change it back.

Jojo-Schmitz · 2020-04-19T06:25:09Z

mscore/file.cpp

+                              QPixmap* pm = new QPixmap(preferences.getString(PREF_UI_CANVAS_FG_WALLPAPER));
+                              p.drawTiledPixmap(pl.at(n)->pageBoundingRect(),
+                                                *pm,
+                                                pl.at(n)->pageBoundingRect().topLeft() - QPoint(lrint(mscore->currentScoreView()->matrix().dx()), lrint(mscore->currentScoreView()->matrix().dy())));


Jojo-Schmitz · 2020-04-19T06:25:18Z

mscore/file.cpp

+            QPixmap* pm = new QPixmap(preferences.getString(PREF_UI_CANVAS_FG_WALLPAPER));
+            p.drawTiledPixmap(pl.at(pageNumber)->pageBoundingRect(),
+                              *pm,
+                              pl.at(pageNumber)->pageBoundingRect().topLeft() - QPoint(lrint(mscore->currentScoreView()->matrix().dx()), lrint(mscore->currentScoreView()->matrix().dy())));


Jojo-Schmitz · 2020-04-19T06:25:27Z

mscore/file.cpp

+                        QPixmap* pm = new QPixmap(preferences.getString(PREF_UI_CANVAS_FG_WALLPAPER));
+                        p.drawTiledPixmap(pl.at(pageNumber)->pageBoundingRect(),
+                                          *pm,
+                                          pl.at(pageNumber)->pageBoundingRect().topLeft() - QPoint(lrint(mscore->currentScoreView()->matrix().dx()), lrint(mscore->currentScoreView()->matrix().dy())));


…olor when exporting to pdf/svg/png

SKefalidis · 2020-04-19T09:39:44Z

Cleanup/refactoring.

igorkorsukov · 2021-02-05T09:37:06Z

Unfortunately, now not actual

Jojo-Schmitz reviewed Apr 14, 2020

View reviewed changes

SKefalidis force-pushed the undo-bold branch from f616ca6 to 755fbe2 Compare April 14, 2020 09:00

SKefalidis force-pushed the undo-bold branch 2 times, most recently from 946ce71 to 9b21805 Compare April 14, 2020 09:09

SKefalidis force-pushed the undo-bold branch 2 times, most recently from 0d1f975 to 7ae13cc Compare April 14, 2020 10:04

Jojo-Schmitz reviewed Apr 14, 2020

View reviewed changes

SKefalidis force-pushed the undo-bold branch from 7ae13cc to 1863e25 Compare April 14, 2020 10:33

SKefalidis changed the title ~~fix #303756: export to pdf with the option to use the page color~~ fix #303756: export to pdf/png/svg with the option to use the page color Apr 14, 2020

Philzen suggested changes Apr 14, 2020

View reviewed changes

SKefalidis force-pushed the undo-bold branch 3 times, most recently from fd99d61 to 459d2ac Compare April 18, 2020 19:32

SKefalidis changed the title ~~fix #303756: export to pdf/png/svg with the option to use the page color~~ fix #303756: export to pdf/png/svg with the option to use the page color or the wallpaper Apr 18, 2020

SKefalidis changed the title ~~fix #303756: export to pdf/png/svg with the option to use the page color or the wallpaper~~ fix #303756 and #293370: export to pdf/png/svg with the option to use the page color or the wallpaper Apr 18, 2020

Jojo-Schmitz reviewed Apr 19, 2020

View reviewed changes

SKefalidis force-pushed the undo-bold branch 2 times, most recently from 589b52d to 6271502 Compare April 19, 2020 09:25

fix #303756 and #293370: Added the option to use the wallpaper/page_c…

a7ab955

…olor when exporting to pdf/svg/png

SKefalidis force-pushed the undo-bold branch from 6271502 to a7ab955 Compare April 19, 2020 09:35

igorkorsukov closed this Feb 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #303756 and #293370: export to pdf/png/svg with the option to use the page color or the wallpaper #5935

fix #303756 and #293370: export to pdf/png/svg with the option to use the page color or the wallpaper #5935

SKefalidis commented Apr 13, 2020 •

edited

Loading

Jojo-Schmitz Apr 14, 2020

Jojo-Schmitz Apr 14, 2020

SKefalidis Apr 14, 2020

SKefalidis Apr 14, 2020

Jojo-Schmitz Apr 14, 2020 •

edited

Loading

SKefalidis Apr 14, 2020 •

edited

Loading

SKefalidis commented Apr 14, 2020

SKefalidis commented Apr 14, 2020

Jojo-Schmitz commented Apr 14, 2020

Jojo-Schmitz commented Apr 14, 2020 •

edited

Loading

SKefalidis commented Apr 14, 2020

Jojo-Schmitz Apr 14, 2020

Jojo-Schmitz Apr 14, 2020

SKefalidis Apr 14, 2020

Jojo-Schmitz Apr 14, 2020

SKefalidis Apr 14, 2020

Jojo-Schmitz Apr 14, 2020

SKefalidis Apr 14, 2020

Philzen commented Apr 14, 2020

Philzen Apr 14, 2020 •

edited

Loading

SKefalidis Apr 19, 2020

Philzen commented Apr 14, 2020 •

edited

Loading

Philzen commented Apr 18, 2020 •

edited

Loading

SKefalidis commented Apr 18, 2020

SKefalidis commented Apr 18, 2020

Jojo-Schmitz Apr 19, 2020

SKefalidis Apr 19, 2020

SKefalidis Apr 19, 2020 •

edited

Loading

Jojo-Schmitz Apr 19, 2020

Jojo-Schmitz Apr 19, 2020

Jojo-Schmitz Apr 19, 2020

SKefalidis commented Apr 19, 2020

igorkorsukov commented Feb 5, 2021

fix #303756 and #293370: export to pdf/png/svg with the option to use the page color or the wallpaper #5935

fix #303756 and #293370: export to pdf/png/svg with the option to use the page color or the wallpaper #5935

Conversation

SKefalidis commented Apr 13, 2020 • edited Loading

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

Jojo-Schmitz Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

SKefalidis Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

SKefalidis commented Apr 14, 2020

SKefalidis commented Apr 14, 2020

Jojo-Schmitz commented Apr 14, 2020

Jojo-Schmitz commented Apr 14, 2020 • edited Loading

SKefalidis commented Apr 14, 2020

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

Philzen commented Apr 14, 2020

Philzen Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philzen commented Apr 14, 2020 • edited Loading

Philzen commented Apr 18, 2020 • edited Loading

SKefalidis commented Apr 18, 2020

SKefalidis commented Apr 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SKefalidis Apr 19, 2020 • edited Loading

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

SKefalidis commented Apr 19, 2020

igorkorsukov commented Feb 5, 2021

SKefalidis commented Apr 13, 2020 •

edited

Loading

Jojo-Schmitz Apr 14, 2020 •

edited

Loading

SKefalidis Apr 14, 2020 •

edited

Loading

Jojo-Schmitz commented Apr 14, 2020 •

edited

Loading

Philzen Apr 14, 2020 •

edited

Loading

Philzen commented Apr 14, 2020 •

edited

Loading

Philzen commented Apr 18, 2020 •

edited

Loading

SKefalidis Apr 19, 2020 •

edited

Loading