fix #307721: blank lines ignored at top of text elements #6328

SKefalidis · 2020-07-10T22:01:27Z

Resolves: Blank lines added before the first line of text would disappear when not editing.

Edit: The source of the problem is that blank lines have no bounding boxes, since they are empty. Essentially they are ignored during layout. I added handling for that. I also improved the handling of empty text fragments and modified genText (which saves all information about the text as text) to include them.

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
[n\a] I created the test (mtest, vtest, script test) to verify the changes I made

MarcSabatella · 2020-07-10T22:03:26Z

libmscore/textbase.cpp

      if (_fragments.empty()) {
            QFontMetricsF fm = t->fontMetrics();
            _bbox.setRect(0.0, -fm.ascent(), 1.0, fm.descent());
            _lineSpacing = fm.lineSpacing();
            }
+      else if (_fragments.size() == 1 && _fragments.at(0).text == "") {


I trust you on this, but, why does this turn out to only cause probems for the leading blank lines, not trailing ones?

Also, is comparing against "" really OK here, what if that initial line actually has spaces?

It's comparing against both size of 1 and "". If there is only one fragment and it is empty then we have an empty line that has a fragment to save formatting information.
If you have a line of pure spaces it enters the 'else' block which calculates the bounding box based on the text. But since spaces have no bounding box the line "does not exist".

As far as why it does not cause problems for trailing lines -> 🤷‍♂️

Follow up for completeness: genText ignored formatting information for empty lines. That meant that when createLayout deleted all fragments and reinserted them it did not have the information provided by empty text fragments. That's why leading fragments did not have the desired effect but trailing ones did (since the information was provided by the line of text whose formatting information was saved by genText).

Spire42 · 2020-07-11T00:57:36Z

libmscore/textbase.cpp

      if (_fragments.empty()) {
            QFontMetricsF fm = t->fontMetrics();
            _bbox.setRect(0.0, -fm.ascent(), 1.0, fm.descent());
            _lineSpacing = fm.lineSpacing();
            }
+      else if (_fragments.size() == 1 && _fragments.at(0).text == "") {


No need to compare against a literal C-style string. Just call isEmpty():

else if ((_fragments.size() == 1) && _fragments.at(0).text.isEmpty()) {

(I also added parentheses to the first expression to conform to the coding guidelines.)

Jojo-Schmitz · 2020-07-11T07:27:44Z

The code does work, but places the text much lower than before:

That text "Chor" looks centered to the bracket (which is 2 'instruments, not a grand staff) in 3.4.2. It uses 13 empty lines to get the position right in 3.4.2, now it needs just 4. Not necessarily a bad thing, just a lame workaround needed earlier now biting back I guess ;-)

Jojo-Schmitz · 2020-07-11T08:40:46Z

commit title should start with "fix #307721:"

MarcSabatella · 2020-07-11T12:40:09Z

Yes, the fact that 13 lines were required before was the bug that was fixed originally, and it is good that is fixed. It was clear with elements other than instrument names that the blank lines were way too small, because you could see the size while editing then the layout would change when you left edit mode. With instrument names you didn’t have that same feedback. But it’s pretty clear just by eyeballing it that by all rights it should not have taken 13 blank lines. Should take the same number of blank ones as it would lines containing actual text.

MarcSabatella · 2020-07-11T16:06:59Z

Something is still wrong, though, the blank lines change size when I leave edit mode. The effect is small if I add only one blank line, but try adding six (say, to the front of the "Title" on the default empty score), then hit Esc, and you see a very big jump.

I think the blank lines being added are too small at first, the size on hitting Esc is probably more correct. So, basically, the exact opposite of the original problem.

Again, trailing blank lines seem to work perfectly. I think understanding why that is and hopefully leveraging the same would be better.

Jojo-Schmitz · 2020-07-12T08:28:11Z

Is that that 'magic factor' 0.27? Maybe not playing along with the screens's DPI setting or the score's spatium?

SKefalidis · 2020-07-14T00:02:35Z

It should be good now. I was missing a pretty important part, every time createLayout is called all the text fragments are destroyed so all information was lost and it was not saved by genText because it explicitly didn't bother with empty fragments.

Jojo-Schmitz · 2020-07-14T06:56:03Z

libmscore/textbase.cpp

            _fragments.insert(0, TextFragment(cursor, ""));
+            }


Why braces here... ((I'm not a friend of excessive use of redundant braces)

Jojo-Schmitz · 2020-07-14T06:56:18Z

libmscore/textbase.cpp

@@ -1148,7 +1190,7 @@ void TextBlock::insertEmptyFragmentIfNeeded(TextCursor* cursor)

 void TextBlock::removeEmptyFragment()
      {
-      if (_fragments.size() > 0 && _fragments.at(0).text == "")
+      if (_fragments.size() > 0 && _fragments.at(0).text.isEmpty())


...but not here?

Jojo-Schmitz

Looks good to me (except for a couple stylistic remarks), I've build and tests and it does work for the cases I tested it with (title text and instrument names with leading blank lines)

Jojo-Schmitz · 2020-07-14T07:24:46Z

libmscore/textbase.cpp

+QList<TextFragment>* TextBlock::fragmentsWithoutEmpty()
+      {
+      QList<TextFragment>* list = new QList<TextFragment>();
+      for (auto x :_fragments) {


space between : and _fragments

Jojo-Schmitz · 2020-07-14T07:25:05Z

libmscore/textbase.cpp

+      for (auto x :_fragments) {
+            if (x.text.isEmpty()) {
+                  continue;
+                  }


these braces seem redundant

Also a simple

for (auto x : _fragments) if (!x.text.isEmpty()) list->append(x);

Should be sufficient? No else, no continue needed (braces for the for loop optional)

… top of text elements

…ext elements

MarcSabatella reviewed Jul 10, 2020

View reviewed changes

SKefalidis force-pushed the text-empty-line branch from a996de9 to 44ac994 Compare July 10, 2020 22:24

Spire42 reviewed Jul 11, 2020

View reviewed changes

Jojo-Schmitz mentioned this pull request Jul 11, 2020

MuseScore 3.5 Release Candidate. PRs to be merged #6195

Closed

1 task

Jojo-Schmitz approved these changes Jul 11, 2020

View reviewed changes

SKefalidis force-pushed the text-empty-line branch from 44ac994 to e1a0235 Compare July 13, 2020 22:18

SKefalidis changed the title ~~fix blank lines (empty text fragments) incorrect bounding box~~ fix #307721: blank lines ignored at top of text elements Jul 13, 2020

SKefalidis force-pushed the text-empty-line branch from e1a0235 to 80ac680 Compare July 13, 2020 23:57

fix #307721: blank lines ignored at top of text elements

f80a331

SKefalidis force-pushed the text-empty-line branch from 80ac680 to f80a331 Compare July 14, 2020 00:00

Jojo-Schmitz reviewed Jul 14, 2020

View reviewed changes

Jojo-Schmitz approved these changes Jul 14, 2020

View reviewed changes

Jojo-Schmitz reviewed Jul 14, 2020

View reviewed changes

vpereverzev approved these changes Jul 14, 2020

View reviewed changes

vpereverzev merged commit 25f92af into musescore:3.x Jul 14, 2020

SKefalidis mentioned this pull request Jul 14, 2020

[MU4 Issue] PRs from 3.x to be merged into / ported to master #6079

Closed

29 tasks

anatoly-os added a commit that referenced this pull request Aug 3, 2020

Manually merged #6328 to master

239a287

igorkorsukov added a commit to igorkorsukov/MuseScore that referenced this pull request Feb 12, 2021

completed ported musescore#6328 : fix #307721: blank lines ignored at…

cbb85e4

… top of text elements

igorkorsukov mentioned this pull request Feb 12, 2021

[MU4] Completed ported #6328 : fix #307721: blank lines ignored at top of t… #7507

Merged

vpereverzev pushed a commit that referenced this pull request Feb 12, 2021

completed ported #6328 : fix #307721: blank lines ignored at top of t…

deaaf3f

…ext elements

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #307721: blank lines ignored at top of text elements #6328

fix #307721: blank lines ignored at top of text elements #6328

SKefalidis commented Jul 10, 2020 •

edited

Loading

MarcSabatella Jul 10, 2020

SKefalidis Jul 10, 2020

SKefalidis Jul 14, 2020

Spire42 Jul 11, 2020 •

edited

Loading

Jojo-Schmitz commented Jul 11, 2020 •

edited

Loading

Jojo-Schmitz commented Jul 11, 2020

MarcSabatella commented Jul 11, 2020

MarcSabatella commented Jul 11, 2020

Jojo-Schmitz commented Jul 12, 2020 •

edited

Loading

SKefalidis commented Jul 14, 2020

Jojo-Schmitz Jul 14, 2020 •

edited

Loading

Jojo-Schmitz Jul 14, 2020

Jojo-Schmitz left a comment •

edited

Loading

Jojo-Schmitz Jul 14, 2020

Jojo-Schmitz Jul 14, 2020

Jojo-Schmitz Jul 14, 2020 •

edited

Loading

fix #307721: blank lines ignored at top of text elements #6328

fix #307721: blank lines ignored at top of text elements #6328

Conversation

SKefalidis commented Jul 10, 2020 • edited Loading

MarcSabatella Jul 10, 2020

Choose a reason for hiding this comment

SKefalidis Jul 10, 2020

Choose a reason for hiding this comment

SKefalidis Jul 14, 2020

Choose a reason for hiding this comment

Spire42 Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz commented Jul 11, 2020 • edited Loading

Jojo-Schmitz commented Jul 11, 2020

MarcSabatella commented Jul 11, 2020

MarcSabatella commented Jul 11, 2020

Jojo-Schmitz commented Jul 12, 2020 • edited Loading

SKefalidis commented Jul 14, 2020

Jojo-Schmitz Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Jul 14, 2020

Choose a reason for hiding this comment

Jojo-Schmitz left a comment • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Jul 14, 2020

Choose a reason for hiding this comment

Jojo-Schmitz Jul 14, 2020

Choose a reason for hiding this comment

Jojo-Schmitz Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

SKefalidis commented Jul 10, 2020 •

edited

Loading

Spire42 Jul 11, 2020 •

edited

Loading

Jojo-Schmitz commented Jul 11, 2020 •

edited

Loading

Jojo-Schmitz commented Jul 12, 2020 •

edited

Loading

Jojo-Schmitz Jul 14, 2020 •

edited

Loading

Jojo-Schmitz left a comment •

edited

Loading

Jojo-Schmitz Jul 14, 2020 •

edited

Loading