[MU4] [WIP] GSoC 2020: Albums #6145

SKefalidis · 2020-05-29T10:15:24Z

Resolves: Albums missing :-)

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

libmscore/layout.cpp

njvdberg · 2020-05-29T12:17:13Z

libmscore/layout.cpp

-      if (!page || curPage >= score->npages()) {
-            page = new Page(score);
-            score->pages().push_back(page);
+      if (!page || curPage >= dominantScore->npages()) {


Since this is based on master, better to start using the new coding standard?

njvdberg · 2020-05-29T12:18:43Z

libmscore/layout.cpp

+//    append systems to the page until you find a page break
+//    or run out of Movements (MasterScores)
+//
+//    called after LayoutContext::getNextPage()


Good to see this kind of comments! Makes it more clear.

These comments are so good that you should prefix them with /// (three slashes) to expose them to Doxygen. Don't use three slashes before the function name (// collectPage) or lines with dashes (//-----------).

njvdberg · 2020-05-29T12:22:32Z

libmscore/layout.cpp

                  }
            if (breakPage) {
                  qreal dist = qMax(prevSystem->minBottom(), prevSystem->spacerDistance(false));
                  dist = qMax(dist, slb);
                  layoutPage(page, ey - (y + dist));
                  // if we collected a system we cannot fit onto this page,
                  // we need to collect next page in order to correctly set system positions
-                  if (collected)
+                  if (collected) { // έχει γίνει κατάχρηση της collected


Sorry, my Greek is not good enough to read this ;-)

😆 the comments in Greek are my notes, they will be removed. This one says that I've abused collected.

njvdberg · 2020-05-29T12:23:49Z

libmscore/layout.cpp

@@ -4579,14 +4619,18 @@ void Score::doLayoutRange(const Fraction& st, const Fraction& et)
 void LayoutContext::layout()
      {
      MeasureBase* lmb;
+      dominantScore = static_cast<MasterScore*>(score); // set the first score (masterscore = movement) as the dominant


Can't you use toScore(score) instead of this cast?

toScore seems to return a Score* not a MasterScore*

And there is no toMasterScore() (unless you add it in libmscore/scoreElement.h 😉). In general we try to prevent these kind of casts.

njvdberg · 2020-05-29T12:39:50Z

mscore/albummanagerdialog.cpp

@@ -0,0 +1,55 @@
+#include "albummanagerdialog.h"


Standard header missing (copyright etc.)

Thanks, I will add it :-)

njvdberg · 2020-05-29T12:40:20Z

mscore/albummanagerdialog.h

@@ -0,0 +1,29 @@
+#ifndef ALBUMMANAGERDIALOG_H


Standard header missing.

njvdberg · 2020-05-29T12:46:37Z

mscore/file.cpp

@@ -298,7 +299,8 @@ void MuseScore::openFiles(bool switchTab, bool singleFile)
             << tr("Bagpipe Music Writer Files (experimental)") + " (*.bww)"
             << tr("Guitar Pro Files") + " (*.gtp *.gp3 *.gp4 *.gp5 *.gpx *.gp)"
             << tr("Power Tab Editor Files (experimental)") + " (*.ptb)"
-             << tr("MuseScore Backup Files") + " (*.mscz, *.mscx,)";
+             << tr("MuseScore Backup Files") + " (*.mscz, *.mscx,)"
+             << tr("MuseScore Album Files") + " (*.msca *.album)";


Are both .msca and .album identical? Or is one of them a compressed file?

.msca is the new albums file.
.album is the MuseScore2 album type.

They are same-ish, the new one has more information and is (in my opinion) more fitting. .album could be anything.

njvdberg · 2020-05-29T12:50:14Z

mscore/revision.h

@@ -1 +1 @@
-3543170
+e122e7c


Is this file supposed to check-in???

Ooops, I'll remove that.

Better install that hook to prevent this. Check https://musescore.org/en/handbook/developers-handbook/finding-your-way-around/git-workflow#Summary step 4:

In your clone directory, copy the file build/git/hooks/post-checkout to the directory .git/hooks in order for mscore/revision.h to be maintained automatically by git. See build/git/hooks/README for details. Note that the .git directory is hidden.

Thanks, I'll do that tomorrow.

Done. Do I need to squash my commits for this to take effect?

Yes, I think so

njvdberg · 2020-05-29T12:57:07Z

mtest/mscore/albums/smallPianoAlbumTest.msca

+    <name>My Album</name>
+    <Score>
+      <alias>Piano1</alias>
+      <path>/home/boop/Documents/GSoC_2020/mscore/MuseScore/mtest/mscore/albums/Piano1.mscx</path>


Having these absolute path makes it difficult to transfer albums.
Is it planned to have albums as compressed folders, containing all scores etc., just like the mscz file is a compressed folder containing the score plus some other files?

I am planning to have both absolute and relative paths (if one does not work, the other might save the day).

How about a compressed file containing all the score of the album, together with the new msca file? But that can be done after the albums itself are finalized.

Jojo-Schmitz · 2020-05-30T14:07:46Z

Once more a rebase seems needed. And the Travis fails is not fixed, see https://travis-ci.org/github/musescore/MuseScore/jobs/692713789#L3927-L3929, it is missing ui_albummanager.h

Jojo-Schmitz · 2020-05-31T13:25:29Z

2 mtest failures, https://travis-ci.org/github/musescore/MuseScore/jobs/693124658#L4386-L4406 and https://travis-ci.org/github/musescore/MuseScore/jobs/693124658#L4453-L4454

Jojo-Schmitz · 2020-05-31T13:28:05Z

mscore/albummanager.h

-    QTableWidgetItem* listItem          { nullptr };
-    QTableWidgetItem* listDurationItem  { nullptr };
+    QTableWidgetItem* listItem          { nullptr }; // αυτά δεν πρεπει να ειναι εδω
+    QTableWidgetItem* listDurationItem  { nullptr }; // και αυτο


While I can understand that this a) is Work In Progress and b) Greek is your native language and as such more comfortable to you, I'd still prefer comments in English, as soon as it gets published here on GitHub

I will take care of that tomorrow :-)

Jojo-Schmitz · 2020-06-01T14:58:10Z

Now there are linker errors and some compiler warnings too on AppVeyor and mtest fails on Travis

Jojo-Schmitz · 2020-06-02T21:23:24Z

rebase needed...

SKefalidis · 2020-06-05T08:35:46Z

Rebased and updated relative file path code.

Jojo-Schmitz · 2020-06-05T14:21:21Z

mscore/albummanager.cpp

-            tempScore = _items.at(0)->albumItem->score->clone(); // TODO: clone breaks editing sync for the 1st movement
+            tempScore = _items.at(0)->albumItem->score->clone(); // clone breaks editing sync for the 1st movement
+            while (tempScore->systems().size() > 1) { // remove the measures of the cloned masterscore, that way editing is synced
+                for(auto x : tempScore->systems().last()->measures()) {


space between forand (

Harmoniker1 · 2020-06-06T16:16:42Z

libmscore/album.cpp

+//---------------------------------------------------------
+//---------------------------------------------------------
+//
+//   AlbumItem
+//
+//---------------------------------------------------------
+//---------------------------------------------------------


I don't think we need a comment chunk so large...

//--------------------------------------------------------- // AlbumItem //---------------------------------------------------------

is good already.

If it isn't too much of a problem, I would like to keep it like that until I the PR is ready.

Harmoniker1 · 2020-06-06T16:18:03Z

libmscore/album.cpp

+
+AlbumItem::AlbumItem(Album* album)
+{
+    this->album = album;


Make class variables prefixed with m_, as stated in the new code guideline. Then we don't need this-> here.

Thank you, I'll take care of it

Harmoniker1 · 2020-06-06T16:18:38Z

libmscore/album.cpp

+    this->album = album;
+}
+
+AlbumItem::AlbumItem(Album* album, MasterScore *score, bool enabled) : AlbumItem(album)


It's best to take : AlbumItem(album) down a new line.

I agree. I have ignored the line width limits in multiple places. I will take care of that in my next refactor.

Harmoniker1 · 2020-06-06T16:19:01Z

libmscore/album.cpp

+
+AlbumItem::AlbumItem(Album* album, MasterScore *score, bool enabled) : AlbumItem(album)
+{
+    std::cout << "New album item..." << std::endl;


Use qDebug.

I will probably delete it when I reach an acceptable level of stability.

If you use qDebug you probably won't have to delete them :)

Hmmm no, final qDebugs don't need these details. So fine, you can use std::couts and delete them when they aren't needed anymore.

SKefalidis · 2020-06-09T15:19:50Z

Ignore the latest commits, I am reworking them (pushed to see the changes in a better interface).

SKefalidis · 2020-06-11T17:26:25Z

In this file I will be writing down bugs, issues and thoughts so that both you and I can see/remember what I am working on.
https://github.com/SKefalidis/MuseScore/blob/albums/albums_readme.md

jthistle

Some very promising work here! I have a few comments. I've tried to keep style stuff to a minimum, but there's still a few things I thought I should pick up. But really, this is great work! Keep it up 🚀

jthistle · 2020-06-16T13:03:39Z

libmscore/album.cpp

+void Album::addScore(MasterScore *score, bool enabled)
+{
+    if (!score) {
+        std::cout << "There is no score to add to album..." << std::endl;


We tend to use qDebug for messages like this, which also lets us know where the message is coming from if it appears in console, iirc.

edit: although I see Howard has already mentioned this

I'll remove that soon enough.

jthistle · 2020-06-16T13:04:14Z

libmscore/album.cpp

+    return nullptr;
+}
+
+MasterScore* Album::removeScore(int index)


Why have this return a MasterScore?

No reason at all, a mistake that was causing me trouble. I'll change that.

jthistle · 2020-06-16T13:04:30Z

libmscore/album.cpp

+//   getDominant
+//---------------------------------------------------------
+
+MasterScore *Album::getDominant()


Style :)

MasterScore * -> MasterScore*

jthistle · 2020-06-16T13:10:15Z

libmscore/album.cpp

+//   loadFromFile
+//---------------------------------------------------------
+
+bool Ms::Album::loadFromFile(const QString &path)


Style, again:

const QString &path -> const QString& path

I won't mention any more things like this, because I think running uncrustify should be enough to fix it.

I would think so too, those were from before I found out how to edit my QtCreator settings :-)

And where did you find that out?

@Marr11317 QtCreator Menubar -> Tools -> Options -> C++, then:

Select the built-in Qt style.

Press Copy... (because you can't edit the built-in style.

Press Edit...

Go to Pointers and References.

Deselect Identifier and select Type name (and maybe left const/volatile?).

jthistle · 2020-06-16T13:46:11Z

libmscore/score.h

-    virtual bool isMaster() const override { return true; }
+    virtual ElementType type() const override { return ElementType::SCORE; } // TODO: if I change it to MasterScore scores are not drawn in album-mode
+
+    virtual bool isMaster() const override { return true; } // TODO: should this be removed?


TODO: should this be removed?

Probably not! I get 60 results across the codebase.

This could be replaced by is MasterScore (and probably will be in the future if those classes are rewritten) but as of right now a MasterScore needs to have ElementType::SCORE (otherwise nothing gets drawn on the screenview). So right now toMasterScore/isMasterScore are useless.

jthistle · 2020-06-16T14:08:36Z

mscore/albummanager.h

+    Q_OBJECT
+
+public:
+    // can I convert these to references?


Well, will they ever have a nullptr value as pointers? If the answer to that question is 'yes', then the answer is no, you can't convert these to references. That's simply because there's no such thing as a null reference.

jthistle · 2020-06-16T14:15:32Z

mscore/albummanagerdialog.cpp

+        break;
+    case QDialogButtonBox::Ok:
+        apply();
+    // intentional ??


Seems so. 'OK' has the behaviour of hiding the window, whereas 'Apply' does not, by convention.

Just copy pasted that (comment included) from the prefsDialog.cpp.

jthistle · 2020-06-16T14:18:07Z

mscore/seq.cpp

+    midi.setMinChunkSize(10);
+
+    if (!heartBeatTimer->isActive()) {
+        heartBeatTimer->start(20);        // msec


Where does this magic number come from?

Copy-paste from Seq::setScoreView.

jthistle · 2020-06-16T14:18:28Z

mscore/seq.cpp

+    }
+
+    midi = MidiRenderer(cs);
+    midi.setMinChunkSize(10);


Another magic number?

jthistle · 2020-06-16T14:19:33Z

mtest/libmscore/albums/smallPianoAlbumTest.msca

+    <name>My Album</name>
+    <Score>
+      <alias></alias>
+      <path>/home/boop/Documents/GSoC_2020/mscore/MuseScore/mtest/mscore/albums/Piano1.mscx</path>


will /home/boop work when this is being run by Travis? I'd guess no.

You are right, fixing the paths of the tests is on my ToDo list :-)

Would be good to avoid absolute paths where possible to avoid leaking information about the user.

Especially, in this case, absolute paths would need a very smart workaround to not fail. I was thinking to give the users the ability to select whether they want both relative and absolute paths when saving/exporting.

Jojo-Schmitz · 2020-06-20T09:16:33Z

Travis still doesn't like it. Here it shows yellow, in progress, but really it is read, failed, and in the mtests. Several address sanitizer failures, but also other, including tst_albums

SKefalidis · 2020-06-20T09:25:25Z

Travis still doesn't like it. Here it shows yellow, in progress, but really it is read, failed, and in the mtests. Several address sanitizer failures, but also other, including tst_albums

@Jojo-Schmitz

4 of the failures are basically the same problem. I've been working on that for a couple of days (I believe that I will solve this today). The tst_albums one is probably a simple fix and I will address it together with @shoogle's and @jthistle's feedback about absolute paths (works locally). The tst_tools failure is not present locally so that will be the last one that I tackle.

SKefalidis · 2020-06-20T16:51:10Z

Only the album test remains :^)

shoogle · 2020-06-21T21:21:38Z

mscore/albummanager.h

-    void currentScoreChanged(int);
-    void itemChanged(QListWidgetItem*);     // score name in list is edited
-    void buttonBoxClicked(QAbstractButton*);
+    void addClicked(bool throwaway = false);


You don't have to give these throwaway parameters a name. This is valid syntax:

void addClicked(bool = false);

Doing this avoids compiler warnings that appear when you give something a name and never actually use it.

See this comment for an alterative solution using Q_UNUSED(throwaway) in the .cpp file.

Addressed in the latest push.

SKefalidis · 2020-06-24T21:40:49Z

Front-cover related code is hard-coded (for now) for demonstration purposes.

SKefalidis · 2020-06-28T20:19:39Z

I fixed the inspector, hope I didn't break anything :-)

jthistle · 2020-06-29T11:13:42Z

Some comments after having tested this: https://gist.github.com/jthistle/1cffc1393ecddd2841e1f5424340c5a6

All in all, this works very well so far! I was genuinely surprised by how functional it already is. Obviously there are bugs and problems, but that's completely normal, and you have ample time to fix them.

Jojo-Schmitz · 2020-06-30T12:36:41Z

mscore/albummanager.cpp

@@ -238,17 +241,11 @@ void AlbumManager::albumNameChanged(const QString& text)
                t->setPlainText("");
            } else if (t->tid() == Tid::COMPOSER) {
                t->setSize(16);
-                t->setPlainText("Sergios - Anestis Kefalidis\nJames Thistlewood\nJoachim Schmitz");
-//                t->setAlign(Align::CENTER);
+                t->setPlainText(m_album->composers().join("\n"));


Not sure, might better be a comma separated list

Jojo-Schmitz · 2020-06-30T12:36:47Z

mscore/albummanager.cpp

            } else if (t->tid() == Tid::POET) {
                t->setSize(16);
-//                t->setAlign(Align::CENTER);
+                t->setPlainText(m_album->lyricists().join("\n"));


…nabling/disabling for parts, various other improvements

Added tabstops and cleaned ui file, again

RomanPudashkin · 2022-12-20T10:27:44Z

Albums will be reimplemented for 4.x. This PR cannot be merged because it is based on 3.x code, which no longer exists

SKefalidis marked this pull request as draft May 29, 2020 10:15

SKefalidis marked this pull request as ready for review May 29, 2020 10:16

njvdberg reviewed May 29, 2020

View reviewed changes

SKefalidis force-pushed the albums branch 2 times, most recently from 3a52d10 to e8edc65 Compare May 29, 2020 20:10

SKefalidis force-pushed the albums branch from 5da5a32 to 8dc02ed Compare May 31, 2020 12:53

Jojo-Schmitz reviewed May 31, 2020

View reviewed changes

SKefalidis force-pushed the albums branch from b75a206 to fbd1614 Compare June 5, 2020 08:35

Jojo-Schmitz reviewed Jun 5, 2020

View reviewed changes

Harmoniker1 reviewed Jun 6, 2020

View reviewed changes

SKefalidis force-pushed the albums branch 2 times, most recently from 21a84a4 to 5bc6443 Compare June 9, 2020 16:05

jthistle reviewed Jun 16, 2020

View reviewed changes

SKefalidis force-pushed the albums branch from 2fcc9b0 to 69bb522 Compare June 20, 2020 14:51

shoogle reviewed Jun 21, 2020

View reviewed changes

Jojo-Schmitz reviewed Jun 30, 2020

View reviewed changes

SKefalidis added 15 commits September 6, 2020 23:55

changed how scores are deleted and forgot this

f0a8464

fix crash by resetting tempScoreTabindex when closing an album

54debad

another small fix

64e098c

fix crash when drag&drop with disabled movements, added support for e…

5a05400

…nabling/disabling for parts, various other improvements

readme

5c9edff

removed text

79c2d5e

fixed editing parts and updating the view when editing them

6488d88

parts scoreview constraints

4a7f767

final touches

5f96cbe

readme

9b0524c

codestyle

3cea653

fix travis failure

d18a6cd

FIRST COMMIT AFTER GOOGLE SUMMER OF CODE:\

bf5db99

Added tabstops and cleaned ui file, again

text styles for front cover and contents wip

6e6ec9b

text styles for front cover and contents

98efe2b

SKefalidis force-pushed the albums branch from 3f3f6de to 98efe2b Compare September 6, 2020 20:55

SKefalidis added 5 commits September 7, 2020 00:03

fixed rebase

cff3e3f

removed unused variable

e49d71a

codestyle

41c9de3

fixed build failure

6390f10

preferences

7087861

igorkorsukov changed the title ~~[WIP] GSoC 2020: Albums~~ [MU4] [WIP] GSoC 2020: Albums Feb 5, 2021

Jojo-Schmitz mentioned this pull request Sep 6, 2021

[MU3] Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 #9000

Closed

lyrra mentioned this pull request Sep 9, 2022

[Mu3] Albums #13274

Closed

RomanPudashkin added the archived PRs that have gone stale but could potentially be revived in the future label Dec 20, 2022

RomanPudashkin closed this Dec 20, 2022

Jojo-Schmitz mentioned this pull request Mar 1, 2023

Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 Jojo-Schmitz/MuseScore#80

Closed

Jojo-Schmitz mentioned this pull request Jun 6, 2023

Re-implementation of the «Album» feature, or a way to more precisely control which instruments are displayed in different sections of the same score #17868

Open

Jojo-Schmitz mentioned this pull request Feb 6, 2024

support for albums Jojo-Schmitz/MuseScore#79

Draft

6 tasks

		@@ -1 +1 @@
		3543170
		e122e7c

[MU4] [WIP] GSoC 2020: Albums #6145

[MU4] [WIP] GSoC 2020: Albums #6145

Conversation

SKefalidis commented May 29, 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

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

SKefalidis May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz May 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SKefalidis Jun 1, 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

Choose a reason for hiding this comment

Jojo-Schmitz commented May 30, 2020

Jojo-Schmitz commented May 31, 2020

Jojo-Schmitz May 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz commented Jun 1, 2020 • edited Loading

Jojo-Schmitz commented Jun 2, 2020

SKefalidis commented Jun 5, 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

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

SKefalidis commented Jun 9, 2020 • edited Loading

SKefalidis commented Jun 11, 2020 • edited Loading

jthistle 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

Choose a reason for hiding this comment

SKefalidis Jun 18, 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

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

Jojo-Schmitz commented Jun 20, 2020 • edited Loading

SKefalidis commented Jun 20, 2020 • edited Loading

SKefalidis commented Jun 20, 2020

SKefalidis commented May 29, 2020 •

edited

Loading

SKefalidis May 29, 2020 •

edited

Loading

Jojo-Schmitz May 31, 2020 •

edited

Loading

SKefalidis Jun 1, 2020 •

edited

Loading

Jojo-Schmitz May 31, 2020 •

edited

Loading

Jojo-Schmitz commented Jun 1, 2020 •

edited

Loading

SKefalidis commented Jun 9, 2020 •

edited

Loading

SKefalidis commented Jun 11, 2020 •

edited

Loading

SKefalidis Jun 18, 2020 •

edited

Loading

Jojo-Schmitz commented Jun 20, 2020 •

edited

Loading

SKefalidis commented Jun 20, 2020 •

edited

Loading