Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX #287602 Page Settings and Units Issues #4906

Open
wants to merge 2 commits into
base: master
from

Conversation

@sidewayss
Copy link
Contributor

commented Apr 12, 2019

This is the first PR in a series of PRs that are pieces of this PR that was deemed to be too broad: #4456

A new [EPIC] issue has been created here:
https://musescore.org/en/node/287602

This code no longer relies on the Sid::pageWhatever values at run-time. Sid::pageTwosided is the only exception to that. Instead the MPageLayout objects provide all the numeric values for page size and margins. That is the primary reason why these changes extend to so many source files.
The page width/height/margins style values are only used when reading/writing from/to a .mscx file.

@sidewayss sidewayss referenced this pull request Apr 12, 2019
libmscore/cmd.cpp Outdated Show resolved Hide resolved
libmscore/read206.h Outdated Show resolved Hide resolved
libmscore/score.cpp Outdated Show resolved Hide resolved
libmscore/style.cpp Outdated Show resolved Hide resolved
mscore/importmxmlpass1.cpp Outdated Show resolved Hide resolved
@@ -5,6 +5,7 @@
<currentLayer>0</currentLayer>
<Division>480</Division>
<Style>
<pageTwosided>0</pageTwosided>

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

This is caused by a bug fix in my code. The source file is not two-sided.

@@ -5,15 +5,6 @@
<currentLayer>0</currentLayer>
<Division>480</Division>
<Style>
<pageWidth>8.26771</pageWidth>

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

You'll see a lot of this in the -ref.mscx files below here. It is an improvement. These values are all rounded versions of the baseStyle values. The PR's code recognizes this and sets them to the baseStyle values internally. Thus the values are optimized out when you save the file.

cp $P/mtest/libmscore/measure/measure-7.mscx measure-7-ref.mscx
cp $P/mtest/libmscore/measure/measure-8.mscx measure-8-ref.mscx
cp $P/mtest/libmscore/measure/measure-9.mscx measure-9-ref.mscx
cp $P/mtest/libmscore/measure/measure-1.mscx measure-1-ref.mscx

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

Yes, some formatting changes along with some real changes. I can back those out if it's preferred.

cp ../../../build.debug/mtest/libmscore/spanners/glissando-cloning03.mscx glissando-cloning03-ref.mscx
cp ../../../build.debug/mtest/libmscore/spanners/glissando-cloning04.mscx glissando-cloning04-ref.mscx
cp ../../../build.debug/mtest/libmscore/spanners/glissando-cloning05.mscx glissando-cloning05-ref.mscx
cp ../../../build.debug/mtest/libmscore/spanners/glissando-cloning02.mscx glissando-cloning02-ref.mscx

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

...and again, formatting changes. Let me know if I can let this slide, of if you want me to revert the formatting.

@@ -0,0 +1,4 @@
#!/bin/bash

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

I added this one updateReference script too. Same reason as the edited updateReference files - a major convenience when running and updating all the mtests. This mtest was added recently, and the creator did not create an updateReference script. They should have. Because of this, I modified the online mtest instructions to include a note telling users to always create an updateReference script for any new mtest directory with -ref.mscx files.

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz Apr 15, 2019

Contributor

#!/bin/sh should be enough and more portable

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

I just changed it. But I copied this from an existing updateReference script. I think they're all this way. See the two I modified above here. I might as well change those too.

_pageOdd.setOrientation(isLandscape ? QPageLayout::Landscape
: QPageLayout::Portrait);
_pageOdd.setMargins(oddMarg);
if (MScore::testMode && _pageOdd.margins().left() == 0) { /// Travis workaround

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

The basic problem in Travis is that the QMarginsF values are all zero. It is only a problem on Travis, though no one has tested it besides me on my Ubuntu partition. But the code makes sense, and the Travis behavior and qDebug messages do not make sense. The code assigns values, but on Travis they are never actually assigned.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

Does the error reproduce on Ubuntu if running tests within xvfb-run, with a command like this one?

QT_QPA_PLATFORM=vnc xvfb-run ./tst_compat114

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

I just tried this for compat114, measure, and split mtests. It acts normally, and does not reproduce the Travis behavior. My qDebug statements are never executed.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

I'm very concerned and curious about this, so I'll fiddle around and verify a bit further, but so far, the answer is No.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

Attached is the log file from my latest local run of ctest on Ubuntu. I ran this command:
QT_QPA_PLATFORM=vnc xvfb-run ctest
LastTest.log

As you can see, none of my qDebug statements execute. (a search for "Odd Left" produces zero results). My current workaround includes a check for zero margins, so it's more limited than before. That means I can now create an mtest for pagesettings.ui, so long I the test does not include setting the odd left margin specifically to zero. That is a case that is easily avoided.
It's still a bummer that it must be worked around, but it's much less intrusive than my original workaround.

I'll run Travis in debug mode on my repo and get the log file from this latest push, so we have current data. That will take a little while. I'll post it here later.

Unfortunately I was unable to capture the travis log file during my debug session, and it seems that you only get one try. I'll try again tomorrow after I clean up my current commit mess.

@sidewayss sidewayss force-pushed the sidewayss:units branch 2 times, most recently from ac2c623 to 42cc158 Apr 12, 2019
@@ -1096,6 +1096,94 @@ enum class Sid {
STYLES
};

// Internal resolution factor that multiplies values originally in points
static constexpr qreal DPI_F = 5; // screen resolution factor

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

I separated this out for readability. In a future PR I will be adding one more constexpr here named MSC_F, for file storage units as whole numbers.

// Default spatium values: MuseScore and MusicXML
// Both values are equivalent: 25.0 @360dpi == 10.0 @144dpi == 5pt
static constexpr qreal SPATIUM20 = DPI_F * (DPI / PPI); // 25.0 == 5pt == DPI_F * DPI_F
static constexpr qreal SPATIUM_XML = 10.0; // MusicXML, in demi-points @144dpi

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

SPATIUM_XML replaces the TEN constant that I had declared in exportxml.cpp. Now I can use this constant in importmxmlpass1.cpp too, and the name has a much clearer meaning.

mscore/exportxml.cpp Outdated Show resolved Hide resolved
@sidewayss sidewayss force-pushed the sidewayss:units branch 3 times, most recently from 558aee2 to f7a41ab Apr 12, 2019
// undoChangeStylePtr
//---------------------------------------------------------

void Score::undoChangeStylePtrs(QPageSize& ps, MPageLayout& odd, MPageLayout& even)

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

class MStyle has 3 new object member variables:
QPageSize _pageSize;
MPageLayout _pageOdd;
MPageLayout _pageEven;
This is their undo class. Maybe it should be called undoChangeStyleObjects, since they are not really pointers anymore...

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

Maybe undoChangeStylePageSettings or something similar? Neither "pointers" nor "objects" tell the reader nothing about what are these pointers/objects for.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

Changed to undoChangePageSettings in next push.

double bottomMarginPoints() const { return margins().bottom() * factor(); }
double paintWidthPoints() const { return paintRect().width() * factor(); }

// For screen layout

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

For future file storage of values in Points * 10,000 units, there will be another set of these functions that multiply by 10,000 instead of DPI_F. For example:

double widthMsc() const { return widthPoints() * MSC_F; }

It all revolves around Points as the core units:
MuseScore DPI == Points * 5
MusicXML units are Points * 2 * (score->spatium() / SPATIUM20)
File storage units will be Points * 10,000 in order to store integer values, not floats.

mscore/musescore.cpp Outdated Show resolved Hide resolved
@sidewayss sidewayss force-pushed the sidewayss:units branch from f7a41ab to dcbe2a2 Apr 12, 2019
static constexpr qreal DPI_F = 5; // screen resolution factor

// Unit conversion factors
static constexpr qreal INCH = 25.4; // millimeters per inch

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

These 3 variable names stink. They should be renamed. INCH is the one that pre-dates this PR, and it's the one that is used the most in the code. I'd like to do it here, but you probably want it in a separate PR. Here is the change I would propose:
Rename INCH to MMPI which stands for millimeters per inch. That is consistent with the variable names directly below.

As for the other two names, that's uglier. Both refer to units that start with the same letter as another, more common unit. I don't mind leaving them the same as they are now. Otherwise I could:
Rename PICA to PPP or PIPP or PIPT or POINT2PICA ...only the last one doesn't stink, but it's long.
Rename DIDOT to DPP or DIPP or DIPPT or DIDOT2POINT ...again, not so great.

@sidewayss sidewayss force-pushed the sidewayss:units branch from dcbe2a2 to 8726ebb Apr 12, 2019
// a) Display and widget control settings for pagesettings.ui
// b) Contents for the units drop-down lists in pagesettings and prefsdialog
// c) Unrounded unit conversions, because QPageLayout rounds everything
// d) _key is a lookup key and the value written to the .mscx file

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

This comment applies to one of my next PRs in this batch, when the pageUnits tag is introduced. Please ignore it for now.

// Qt provides only an enumeration for units: QPageLayout::Unit
// MuseScore needs all this additional data per unit. The data is for:
// a) Display and widget control settings for pagesettings.ui
// b) Contents for the units drop-down lists in pagesettings and prefsdialog

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

The pagesettings.ui units list is in this PR, but is it always hidden. It will be enabled in one of the next PRs in this batch.

libmscore/style.h Outdated Show resolved Hide resolved
libmscore/style.h Outdated Show resolved Hide resolved
@sidewayss sidewayss force-pushed the sidewayss:units branch from 8726ebb to b259a63 Apr 12, 2019
@@ -277,12 +277,13 @@ class ExportMusicXml {
Ottava const* ottavas[MAX_NUMBER_LEVEL];
Trill const* trills[MAX_NUMBER_LEVEL];
int div;
double millimeters;
int tenths;
double scale; // a multiplier, used in the getTenths functions

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

This module has been refactored in a major way. The numeric formulas for calculating "tenths" and other units are completely redesigned and greatly simplified.
Some of the functions have also been generally simplified, beyond the numeric formulas. For example, they were passing the class member variables as arguments all the time, for no reason. Only the member variables were ever passed in. So I removed the arguments and use the member variables instead.
Here is the Excel file I used to develop the new formulas, but it might be too raw to be helpful to someone else. In any case it's small:
tenths.xlsx

@@ -5437,12 +5441,12 @@ bool saveXml(Score* score, const QString& name)
QFile f(name);
if (!f.open(QIODevice::WriteOnly))
return false;

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 12, 2019

Author Contributor

This appears to be github getting confused. A - on one side and a + on the other side, both for empty lines.

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz Apr 15, 2019

Contributor

Not really an empty line, but a removed whitespace (a tab probably, or several spaces).
And as such a good change, style wise. More of those furter down.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

Probably done by Qt Creator, which I have set to strip trailing whitespace.

Copy link
Contributor

left a comment

Aside from the notes I left as inline comments, I would like to state some most important questions or suggestions here.

  1. As far as I understand, there are a lot of fuzzySet() calls now at reading page settings values. However they make sense only in case the values stored in a file are close to default values. Isn't the approach with not storing the default values better? And is there a reason to target the precise default settings values so much, if the rounding is anyway small?
  2. There are now several sets of page formats within libmscore. So two questions here:
    2.1) Why do they need to appear inside libmscore? They don't seem to be used there (except for filtering out page size IDs within read301.cpp where it doesn't seem to have any benefit).
    2.2) In any case, could you please extract their initialization to a separate function? Or maybe it would be even better to make their initialization not rely on explicitly calling anything before their usage.
  3. I didn't review the page settings dialog UI closely but the actual bug in the page settings dialog seems to be only partially fixed. I left an inline note about that.
// undoChangeStylePtr
//---------------------------------------------------------

void Score::undoChangeStylePtrs(QPageSize& ps, MPageLayout& odd, MPageLayout& even)

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

Maybe undoChangeStylePageSettings or something similar? Neither "pointers" nor "objects" tell the reader nothing about what are these pointers/objects for.

@@ -3292,7 +3292,7 @@ System* Score::collectSystem(LayoutContext& lc)
qreal layoutSystemMinWidth = 0;
bool firstMeasure = true;
bool createHeader = false;
qreal systemWidth = styleD(Sid::pagePrintableWidth) * DPI;
qreal systemWidth = style().pageOdd().paintWidth();

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

Am I correct that the available width can theoretically be now different for odd and even pages? If so, the layout should probably handle this potential difference correctly instead of just using one set of values. Or we should avoid having different page width for even and odd pages.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

_pageEven is only used for the margin values. Both MPageLayout objects share the same QPageSize object, so the page sizes are identical in both.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 16, 2019

Contributor

Yes, page size is the same, but system width should take margins into account, shouldn't it?

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

Exactly, thus the paintWidth() method which uses the QPageLayout.paintRect() instead of the fullRect(). The paintRect excludes margins. It is the equivalent of the Sid::pagePrintableWidth.

@@ -79,6 +79,7 @@ bool MScore::saveTemplateMode = false;
bool MScore::noGui = false;

MStyle* MScore::_defaultStyleForParts;
int MScore::_unitsValue;

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

What is _unitsValue? And why is it int? If it is a value from QPageSize::PageSizeId enum then exactly this type should be used in declarations.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

It is an integer because pageUnits is an array and the drop-down lists use an integer index to set and get the value. It is used more as an integer than as a QPageLayout::Unit enum value.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 16, 2019

Contributor

int tells the reader absolutely nothing about what is the meaning of this variable and which values it can take. If your array's indices are synchronized with that enum then it is better to declare this variable with that enum type. We already do it for many enums, for example, for ElementType enum which is synchronized with the list of elements' names to be able to use enum value (converted to int) as that array index. If the fact of such a usage is explicitly stated in comments it should be OK to do so.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

OK, no problem, I'll declare it as QPageLayout::Unit and cast as int everywhere.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

The preference will still be an int though. Or should I create a whole new type of preference?

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

I have resolved this by changing only mscore.h. The private variable is now declared as QPageLayout::Unit, but the public unitsValue() and setUnitsValue() functions take and return int. Those functions do all the type casting. The rest of the code remains the same. Easier than I imagined :)

@@ -2143,7 +2145,31 @@ void MStyle::precomputeValues()

bool MStyle::isDefault(Sid idx) const
{
return value(idx) == MScore::baseStyle().value(idx);

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

Is this function rewritten just to introduce the Q_ASSERT_X below? If so, is it necessary to leave it? If it is still necessary then the type checks should probably be done inside that Q_ASSERT in order not to introduce that series of string comparisons to release builds.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

This function is rewritten because comparing raw Variants is bad practice, and it caused me errors.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 16, 2019

Contributor

Why is it a bad practice, and why did it cause errors?
If the source of the errors is type conversions between QVariants, you can, for example, also check their types for being equal.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

Here is the spot in the developers' chat where Matt got me on this path:
https://t.me/musescoreeditorchat/22472
But maybe it's no longer an issue and I can revert the function. I'll give it a try.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

I reverted this to the master, and I am not having problems with mtest or anything else that I can see (yet...). It's in the next push.

@@ -4326,6 +4364,7 @@ Adjusting latency can help synchronize your MIDI hardware with MuseScore's inter
</tabstops>
<resources>
<include location="musescore.qrc"/>
<include location="musescore.qrc"/>

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

Should this line be repeated twice?

This comment has been minimized.

Copy link
@Jojo-Schmitz

Jojo-Schmitz Apr 15, 2019

Contributor

Probably not, but I've seen QtDesigner doing this under the covers.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

I can remove it in a text editor, if it's a problem.

_pageOdd.setOrientation(isLandscape ? QPageLayout::Landscape
: QPageLayout::Portrait);
_pageOdd.setMargins(oddMarg);
if (MScore::testMode && _pageOdd.margins().left() == 0) { /// Travis workaround

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

Does the error reproduce on Ubuntu if running tests within xvfb-run, with a command like this one?

QT_QPA_PLATFORM=vnc xvfb-run ./tst_compat114
// To create excerpt clone to show when changing PageSettings
// Use MasterScore::clone() instead
//---------------------------------------------------------

Score* Score::clone()
{
style().fromPageLayout301(); // syncs page settings styles/XML to objects

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

That doesn't seem to be good if one must explicitly synchronize something that is purely internal for MStyle object. It is easy to forget to do this. Can this be avoided?

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

It is necessary when reading/writing the score file, but that's all internal to MStyle. Here is a case where Score::clone clones the score by generating the XML that is generated when saving, and then reading that as if it were a new file. Seems to me there should be a more binary way of cloning a score than worrying about this detail.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 16, 2019

Contributor

My concern is about the fact that something that is internal to MStyle and is necessary for it to work properly should be handled everywhere in the code. That doesn't look like a good design for MStyle class.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

I don't disagree, but this is the only place where it's used outside of MStyle class. The other solution is to stick it in the XMLReader, or in the Score constructor. That is the same issue. I don't see a solution for this at this time.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

OK, I am able to delete this line. The normal process handles this, as I thought it should. At one point last week I had a problem here. It might have been in the same time in which I was confused by vs Score::style() vs MasterScore::style() and the Score::_style variable. It's confusing, but I understand it now.
There was a buggy behavior I was trying to fix with this. I will test this some more before committing and pushing it, but in the process of stepping the call stack just now I confirmed that it's not needed. After commenting it out I don't see the buggy behavior that I recall. Why did it happen in the first place? Now I may never know. The only explanation I can think of is that I added this line of code unnecessarily during my confusion about _style vs style().

sizesCommon.push_back(QPageSize::Ledger);
sizesCommon.push_back(QPageSize::Tabloid);
sizesCommon.push_back(QPageSize::Imperial9x12);
sizesCommon.push_back(QPageSize::Imperial10x13);

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

It would probably be better to initialize it with initializer list rather than pushing each value separately. It would be both faster and more readable.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

I don't know how to do that with std::vector and std::set. I looked online for help or examples and found none. If you can show me how, I'll do it.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

Do you want me to change this for the std::set variables too, or just for the vector with the special sort order, sizesCommon? The for loops are convenient for the other 3, but I can initialize them with literal values too, if that is preferred. This is fixed in the next push.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

In the next push you'll see a new MScore::initPageSizes() function. Along the same lines, it's tempting to create a MScore::initStyles() function.

MPageLayout& even = prev->style().pageEven();

// Has anything changed? This prevents extra undo commands on the stack
// when the user clicks Apply or OK with no changes.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

Actually here is the chance to fix the whole issue with rounding values on measurement units changes, not only extra undo stack entries. There is no need to change the actual value until the user explicitly changes it, and it should not depend on which units are selected. And only the changed values should be actually overwritten, you should not just overwrite all values in case one small setting is changed. This seems to be a simple and logical rule, and conforming it would actually fix the most part of the issues you are trying to fix. I tried to propose it in the forum topic (for example, here), but you seem to use the approach with largely changing the way MuseScore reads the style values from the file — is that really necessary to do?

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

Changing the dialog to operate the way editstyle.ui does, with no Apply button, is possible, but it's a big change. And I don't know that it would help with anything. I tried to fix all of this with an upgrade to the way MuseScore handles page settings. I could have hacked something, but I didn't want to do that.

Here is the basic idea:
MPageLayout stores the values in the user units. That way the source values are exactly the way the user wants them.
MPageLayout then provides ways to extract unrounded, converted values for MuseScore, in whatever units MuseScore needs.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 16, 2019

Contributor

with no Apply button

I don't propose not having Apply button, I propose just changing the way both Apply and OK buttons work. The proposed changes should probably be able to avoid most of cases of unintended page settings changes which was, as far as I understand, one of the most important issues originally.

Concerning MPageLayout, it may indeed be convenient to have such a class to be a "bridge" between the human-readable values and MuseScore internal values. In the approach I propose though it should be not necessary to have this class built directly into MStyle, and the changes could limit just to reworking the page settings dialog itself.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

I'll think about this, but I don't know how it would work off the top of my head. The page settings end up as styles when reading/writing the file. The conversion to/from styles would have to be elsewhere.
How would this work when opening an existing score from a file? Score is in libmscore. MStyle is in libmscore. What part of opening an existing score is in mscore? I suppose the menu command to initiate it is in mscore, as is the startup of the application, which can open existing files.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

MStyle::pageOdd() is used by these libmscore modules, in addition to style.cpp, read206.cpp, read301.cpp:

layout.cpp: checkDivider() and Score::checkSystem()
page.cpp: all the margins functions, tm() lm() bm() and rm()
score.cpp: Score::Score() and innerWidth() + innerHeight()
undo.cpp: my new ChangePageSettings class

This is MPageLayout providing unrounded values in MuseScore units. I don't see a way to encapsulate this in the mscore project. It must be available in libmscore. And it's not a static instance of anything in class MScore. These are individual objects that pertain to each score, not to the application.

If undo.cpp is in libmscore, and I need to undo this stuff, how can it be only in mscore? If there's a way to do this that I'm not understanding, please tell me.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

One solution that comes to mind is to create a static MScore variable:
static std::map<Score*, PageSettings*> _pageSettings;

It would require a new PageSettings class in mscore. Then the libmscore code that needs the page settings values could look up the score's objects and values in the map.

It also would require MStyle to add a variable: Score* _score; That then requires code to set this variable. I don't think that's desirable either. Currently MStyle knows nothing of the Score that contains it.

Unless you could manage it this way:
static std::map<MStyle&, PageSettings&> _pageSettings;
It seems convoluted to me, but if keeping it out of libmscore is important, it might work.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 16, 2019

Author Contributor

btw - Marc is having a similar issue now with the _chordsList member variable of MStyle. It is the one other style value not in _values, and it is also an object, not a Variant. I'm unsure of the solution he is pursuing.
https://t.me/musescoreeditorchat/34248

_chordsList is different than the page settings objects because it does not duplicate any data in _values. I'm not sure how significant a difference that is within this discussion.

If MStyle is best as purely a container for _values and _precomputedValues, then all the object member variables should move elsewhere. Otherwise they must be managed outside of the rest of the styles that are so conveniently arranged in the _values array.

Here's another possibility: QVariant can contain objects too, right?
How about adding Sid::pageSize, Sid::pageOdd, Sid::pageEven?
There would be special cases to avoid writing them to the file. The Sids won't match up with the style tags one-to-one, as is the case for simple data types, but that doesn't have to be a problem.
This does not move MPageLayout to mscore, but it's potentially a better approach than my current one.

Would this be beneficial? Hmmm.
It looks like the main result of this change would be to move code into MStyle::load() and MStyle::save(). It could eliminate the need for the current Sid::pageXXX styles altogether.

void MStyle::spatium301(XmlReader& e)
{
QString txt = e.readElementText();
if (txt == "1.76389" || txt == "1.764" || txt == "1.7526" || txt == "1.753")

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Apr 15, 2019

Contributor

This might be good to convert some old files if you are sure that all 1.7526 in your files result exactly from this bug, but should this conversion rule be applied always?

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

These values appear in new user files today too.
I see your concern that the user might actually set their spatium to something equivalent to the last two of the four values. I can see if that's possible with the new code. I doubt that it is, because these numbers are based on specific rounding being done.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

Duh! The user can enter 1.753 directly in the dialog box, and I am excluding that from the range of possible values. It's an unusual value to enter intentionally, but still it's bad practice to exclude it. AFAICT it is not possible to generate and store a value of 1.7526 with this PR's code.

This is part of why I want to move towards storing integer values and then all of this can go away some day. As of v4, it would be relegated to the 301 code only.

In MuseScore 3.1 beta and previous versions:
1.7526 millimeters exists in any file where the user selects the Inches radio button in pagesettings.ui, leaves the spatium value at 0.069 inches, then clicks Apply or OK.
1.753 value happens subsequently, when the user switches back to Millimeters, then clicks Apply or OK. Or if they truly intend to have this spatium value, which is unlikely, but possible.

I don't see a way of converting old files, except via a compatibility build and a major version change. mtest files could be converted, but then they would not be representing the real world situation, which is not good.

In terms of risk: I believe this one exception is very low risk. I highly doubt that 1.753 exists anywhere due to the user inputting that value intentionally, without ever switching to Inches. 1.750 is a common value. 1.753 is a rounding error. From what I've heard, the 3rd decimal place is generally for things like 1.875, divisions by 8 that can put a 5 in the 3rd decimal slot.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

Or screw the old files. Let them live on with their slightly off-default spatium values. That's not a bad idea. You can always correct an old file by opening pagesettings.ui and setting a default spatium value there.
We're only talking about 1.7526 and 1.753, correct? 1.764 and 1.76389 are clearly intended to be equal to SPATIUM20.

This comment has been minimized.

Copy link
@sidewayss

sidewayss Apr 15, 2019

Author Contributor

I just commented out the comparisons to 1.7526 and 1.753. Then ctest ran without errors. I do see one mtest file that has 1.7526 in it here:

<Spatium>1.7526</Spatium>

But that file appears to have no diff in the test, so there's no change there.

I'm going to remove these two comparisons, and you'll see the change in the next push.

@sidewayss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

>As far as I understand, there are a lot of fuzzySet() calls now at reading page settings values.
This is partly so that mtest doesn't go nuts. It's also partly because I'm breaking this PR into pieces. The code does optimize out the baseStyle values, that does not change.

>There are now several sets of page formats within libmscore
There are 3 new variables, _pageSize, _pageOdd, and _pageEven. Is that what you mean?

2.1) Why do they need to appear inside libmscore? They don't seem to be used there (except for filtering out page size IDs within read301.cpp where it doesn't seem to have any benefit).
They are part of the MStyle class and the score's _style member variable. That's all libmscore. They are used when reading and writing the file. I don't understand the question.
Filtering page size ids is important too.

2.2) In any case, could you please extract their initialization to a separate function? Or maybe it would be even better to make their initialization not rely on explicitly calling anything before their usage.
The alternative is to write other, additional code to synchronize the Sid values with the MPageLayout values. The initialization is less of an issue than the synchronization.
Or the alternative is to eliminate these Sid values and store the tags in the file from the MPageLayout objects - but that's very messy for MStyle::save(), which simply iterates over the Sid values.

I didn't review the page settings dialog UI closely but the actual bug in the page settings dialog seems to be only partially fixed. I left an inline note about that.
Fully fixing the pagesettings dialog rounding when switching units is implausible, maybe impossible. Not worth the effort. In this PR, the user cannot change the units in the dialog, so it's much less of a concern.

@sidewayss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

To repeat a general statement I made in a review comment:

MPageLayout stores all the values in the user's units. Those values are exactly what the user specified, without any conversions.
MPageLayout then provides methods to get unrounded values in the units that MuseScore needs.

This separation of values and units avoids changes to the user's original values that might be caused by unit conversions when caching a converted value to the MStyle's _values array (_values is accessed by the MStyle::value() and MStyle::set() methods).

@sidewayss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Regarding fuzzySet():
When reading a file, it avoids changing cached baseStyle or defaultStyle values that are rounded in the file. This way the internal values at run-time are correct, based on the intention of using the base or default style.
So at both read and write time, I ensure that clean default values are used when appropriate. It might be overly fastidious, but it's correct.

@sidewayss sidewayss closed this Apr 15, 2019
@sidewayss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

What did I just do? I just closed this PR the same way Jojo marks his own comments as spam :)

@sidewayss sidewayss reopened this Apr 15, 2019
@sidewayss sidewayss force-pushed the sidewayss:units branch from b259a63 to 43f8b85 Apr 15, 2019
libmscore/edit.cpp Outdated Show resolved Hide resolved
libmscore/undo.h Outdated Show resolved Hide resolved
@sidewayss sidewayss force-pushed the sidewayss:units branch from 43f8b85 to e410fcf Apr 15, 2019
@sidewayss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

dammit! I created a commit that included files from both my previous commits, with the mtest files separate. I didn't realize until I tried to squash commits and it failed. After that, something went wrong. Maybe I typed rebase -i HEAD~14 instead of 4. I thought I had aborted that rebase.
What's the best way to start fresh here?

@dmitrio95

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

You can start the same interactive rebase and just drop extra commits

@dmitrio95

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

>As far as I understand, there are a lot of fuzzySet() calls now at reading page settings values.
This is partly so that mtest doesn't go nuts. It's also partly because I'm breaking this PR into pieces. The code does optimize out the baseStyle values, that does not change.

What is wrong with mtest in case you don't do this? It was completely working before that change.

2.1) Why do they need to appear inside libmscore? They don't seem to be used there (except for filtering out page size IDs within read301.cpp where it doesn't seem to have any benefit).
They are part of the MStyle class and the score's _style member variable. That's all libmscore. They are used when reading and writing the file. I don't understand the question.
Filtering page size ids is important too.

When are they used? It seems I didn't notice their usage.

2.2) In any case, could you please extract their initialization to a separate function? Or maybe it would be even better to make their initialization not rely on explicitly calling anything before their usage.
The alternative is to write other, additional code to synchronize the Sid values with the MPageLayout values. The initialization is less of an issue than the synchronization.
Or the alternative is to eliminate these Sid values and store the tags in the file from the MPageLayout objects - but that's very messy for MStyle::save(), which simply iterates over the Sid values.

Actually I was talking about the page sizes sets initialization inside MScore::init(): it is better at least to have a call to a separate function there rather than just adding a code chunk just inside that function.

Fully fixing the pagesettings dialog rounding when switching units is implausible, maybe impossible. Not worth the effort. In this PR, the user cannot change the units in the dialog, so it's much less of a concern.

I am talking about fixing unintended rounding in case user didn't change some particular setting. As I wrote above, it should be enough to fix almost all the targeted issues (except for reworking the page settings UI itself).

@sidewayss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

>As far as I understand, there are a lot of fuzzySet() calls now at reading page settings values.
This is partly so that mtest doesn't go nuts. It's also partly because I'm breaking this PR into pieces. The code does optimize out the baseStyle values, that does not change.

What is wrong with mtest in case you don't do this? It was completely working before that change.

What's wrong is with my code and mtest and values that change slightly. I think my comment here is not as useful as the comment I made later about why this is here.

2.1) Why do they need to appear inside libmscore? They don't seem to be used there (except for filtering out page size IDs within read301.cpp where it doesn't seem to have any benefit).
They are part of the MStyle class and the score's _style member variable. That's all libmscore. They are used when reading and writing the file. I don't understand the question.
Filtering page size ids is important too.

When are they used? It seems I didn't notice their usage.

The page size ids? They are used in pagesettings.ui in the drop-down lists. The code must find the score's page size id to display it. The drop-down lists are a subset of the full QPageSize:PageSIzeId enum.

Actually I was talking about the page sizes sets initialization inside MScore::init(): it is better at least to have a call to a separate function there rather than just adding a code chunk just inside that function.

OK, no problem

Fully fixing the pagesettings dialog rounding when switching units is implausible, maybe impossible.

Not worth the effort. In this PR, the user cannot change the units in the dialog, so it's much less of a concern.

I am talking about fixing unintended rounding in case user didn't change some particular setting. As I wrote above, it should be enough to fix almost all the targeted issues (except for reworking the page settings UI itself).

There are certainly simpler ways to avoid rounding the default spatium value. If that's all this PR wanted to do, then sure. When I was starting this 2nd PR attempt, I proposed some basic fixes like this and there was some objection. I thought that wrapping it up in some enhancements to pagesettings.ui and moving the units configuration to preferences would help get it all approved.

@sidewayss sidewayss force-pushed the sidewayss:units branch from e410fcf to 3aa5542 Apr 16, 2019
@sidewayss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

my rebasing efforts continue to misfire. I'm still working on it...
After wasting time trying to solve this within the same branch, I have now created a new branch from master and copied my latest files into it. I rebased to the master before copying my latest files, built, confirmed ctest results, and ran this command:
git push -f origin newUnits:units
Now we're back to only 2 commits and the code should be all up-to-date.

@sidewayss sidewayss force-pushed the sidewayss:units branch 2 times, most recently from 45acbb5 to 0607c58 Apr 16, 2019
@sidewayss sidewayss force-pushed the sidewayss:units branch 3 times, most recently from b850847 to 39b0fe4 May 8, 2019
@sidewayss sidewayss force-pushed the sidewayss:units branch from 72299c4 to 26aa4b8 May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.