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

GSoC 2020: Create and display initial tree model #6174

Merged
merged 4 commits into from
Jun 29, 2020

Conversation

krkartikay
Copy link
Contributor

@krkartikay krkartikay commented Jun 7, 2020

Adds tree hierarchy virtual functions treeParent(), treeChild(int idx), and treeChildCount() to descendants of ScoreElement. Also adds a ScoreItemModel (based on QAbstractItemModel) and an iterator to ScoreElement class to iterate over the child elements.

[Edit:]

Latest Blog for this PR: https://musescore.org/en/user/1743616/blog/2020/06/22/gsoc-2020-tree-model-week-3-finished-my-first-pr

image

@krkartikay krkartikay marked this pull request as draft June 7, 2020 05:22
@Jojo-Schmitz
Copy link
Contributor

@krkartikay
Copy link
Contributor Author

See https://travis-ci.org/github/musescore/MuseScore/jobs/695630221#L4559-L4591, there's a SIGSEGV

Yep, working on debugging it! :-)

Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Please make requested changes and squash into 3 commits:

  1. Create score tree model
    • Add libmscore/scoretree.cpp and modify all relevent .h files.
  2. Create ScoreItemModel
    • Add my code in mscore/scoreitemmodel(.h|.cpp)
    • We will make me the author of this commit
  3. Display score model in Tree View
    • Add mscore/scoretreemodel(.h|.cpp)
    • Make all necessary changes to other files (e.g. mscore/scoreitemmodel(.h|.cpp)).

Feel free to add more commits for anything that doesn't logically fit in one of those commits.

libmscore/scoreElement.cpp Outdated Show resolved Hide resolved
libmscore/scoreElement.h Outdated Show resolved Hide resolved
libmscore/element.cpp Outdated Show resolved Hide resolved
mscore/musescore.cpp Outdated Show resolved Hide resolved
libmscore/ambitus.h Outdated Show resolved Hide resolved
@krkartikay krkartikay force-pushed the gsoc-2020-Tree-Model-base branch 2 times, most recently from 5cc0a2e to 70472ea Compare June 16, 2020 10:34
@krkartikay krkartikay marked this pull request as ready for review June 16, 2020 10:36
Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make requested changes. Git's --fixup and --autosquash options make it easy to amend historic commits. See article here.

libmscore/chordrest.h Outdated Show resolved Hide resolved
mscore/scoreitemmodel.h Outdated Show resolved Hide resolved
mscore/scoreitemmodel.h Outdated Show resolved Hide resolved
mscore/scoreitemmodel.h Outdated Show resolved Hide resolved
mscore/scoretreeview.h Outdated Show resolved Hide resolved
mscore/scoreview.cpp Outdated Show resolved Hide resolved
mscore/scoretreeview.h Outdated Show resolved Hide resolved
mscore/scoreview.cpp Outdated Show resolved Hide resolved
mscore/scoreview.cpp Outdated Show resolved Hide resolved
mscore/scoreitemmodel.cpp Outdated Show resolved Hide resolved
@krkartikay krkartikay force-pushed the gsoc-2020-Tree-Model-base branch 3 times, most recently from a6a5f90 to 01e6174 Compare June 17, 2020 08:47
mscore/scoretreewidget.h Outdated Show resolved Hide resolved
mscore/scoretreewidget.h Outdated Show resolved Hide resolved
mscore/scoretreewidget.h Outdated Show resolved Hide resolved
Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there. Please squash these latest changes into the last of the original 3 commits.

mscore/scoretreeview.cpp Outdated Show resolved Hide resolved
mscore/scoretreeview.cpp Outdated Show resolved Hide resolved
mscore/scoretreeview.h Outdated Show resolved Hide resolved
mscore/scoretreewidget.h Outdated Show resolved Hide resolved
mscore/scoretreeview.h Outdated Show resolved Hide resolved
mscore/scoretreeview.cpp Outdated Show resolved Hide resolved
mscore/scoretreeview.cpp Outdated Show resolved Hide resolved
mscore/scoretreeview.cpp Outdated Show resolved Hide resolved
mscore/scoretreeview.cpp Outdated Show resolved Hide resolved
mscore/scoreitemmodel.h Outdated Show resolved Hide resolved
@krkartikay krkartikay force-pushed the gsoc-2020-Tree-Model-base branch 3 times, most recently from 6c43906 to 1c20a31 Compare June 18, 2020 11:33
Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think these will be the final set of changes. You should probably also run the changed C++ files through uncrustify. There is a script in tools/codestyle/uncrustify_run_file.sh. Do an interactive rebase and mark all commits as e for edit, then amend each one in turn. You might find you have fewer merge conflicts if you do them in reverse order:

  1. Amend commit 3 (the last commit).
  2. Amend commit 2 (the middle commit) and then 3 again.
  3. Amend commit 1 (the first commit), then 2, then 3.

mscore/scoretreewidget.cpp Outdated Show resolved Hide resolved
mscore/scoretreewidget.h Outdated Show resolved Hide resolved
mscore/scoreitemmodel.h Outdated Show resolved Hide resolved
libmscore/scoretree.cpp Show resolved Hide resolved
mscore/scoretreeview.cpp Show resolved Hide resolved
mscore/scoretreeview.cpp Show resolved Hide resolved
@ecstrema
Copy link
Contributor

Do an interactive rebase and mark all commits as e for edit, then amend each one in turn. You might find you have fewer merge conflicts if you do them in reverse order:

Why not mark them as s for squash so it will do the amending automatically?

@shoogle
Copy link
Contributor

shoogle commented Jun 18, 2020

@Marr11317, the idea was not to squash the commits but to tidy (or "uncrustify") the code within them. Every commit should do only "one thing" to the code, so I expect this PR to have at least 3 commits:

  1. Create tree model (in libmscore)
  2. Create item model (in mscore)
  3. Display model in tree view

If a commit title has the word "and" then you should probably use 2 commits.

@krkartikay
Copy link
Contributor Author

Added another commit to add spanner elements to the model. Spanners can be attached to either Measures, Segments, Chords or Notes. Their parent is the start element they are attached to, and their children are the SpannerSegments (one for each system the spanner appears in).
image
(@shoogle )

Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kartikay26, the Spanner code looks good! Good job working out what SpannerSegment is for. I've spotted a few more changes that need to be made in addition to those mentioned in my previous review.

libmscore/spanner.cpp Outdated Show resolved Hide resolved
mscore/musescore.cpp Outdated Show resolved Hide resolved
mscore/musescore.cpp Outdated Show resolved Hide resolved
mscore/musescore.cpp Outdated Show resolved Hide resolved
mscore/musescore.cpp Outdated Show resolved Hide resolved
mscore/musescore.cpp Outdated Show resolved Hide resolved
mscore/scoretreeview.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Just needs these two tiny changes and then I think it is done.

mscore/musescore.cpp Outdated Show resolved Hide resolved
mscore/scoretreewidget.cpp Outdated Show resolved Hide resolved
@krkartikay krkartikay changed the title [WIP] GSoC 2020: Add tree hierarchy functions to ScoreElements. GSoC 2020: Create and display initial tree model Jun 20, 2020
Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, @Kartikay26! I think we should merge this and move on to another PR.

libmscore/bsymbol.h Outdated Show resolved Hide resolved

ScoreElement* Score::treeParent() const
{
return nullptr; // Score is root node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe for Score parent is the MasterScore, and the MasterScore should be the root?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still some confusion about what Score and MasterScore actually mean, so my vote is to leave it how it is for now and then wait to see if it causes problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like for now Score and MasterScore actually point to the same object.

@krkartikay krkartikay force-pushed the gsoc-2020-Tree-Model-base branch 3 times, most recently from 62b06e5 to 0990831 Compare June 22, 2020 12:31
@igorkorsukov
Copy link
Contributor

@Kartikay26 Tests do not compile, could you fix it?

krkartikay and others added 3 commits June 23, 2020 12:44
This commit adds virtual functions treeChild, treeParent and
treeChildCount to ScoreElement and implements them in most non-leaf-node
classes. An iterator is also added to ScoreElement class to iterate over
the children of any element.

In this model, Spanners, Beams and Ties are given a single parent, which
is the starting element of the spanner, beam or tie. Also, to ensure
consistency in the model, spanners, beams, and ties appear in the
children list only for their starting element. Children of spanner
elements are SpannerSegments, one for each system the spanner appears
in.
Created a ScoreItemModel using the score tree functions. It is a
QAbstractItemModel which can be used with a QTreeView or
QAbstractItemView.
Added a ScoreTreeView that displays the score tree in a QTreeView,
and also created a QDockWidget called ScoreTreeWidget which is enabled
in the debug builds to inspect the score tree model.
@krkartikay
Copy link
Contributor Author

@igorkorsukov fixed the test and squashed the bug fix into previous commits. I don't know why AppVeyor isn't working though...

@AntonioBL
Copy link
Contributor

I think maybe because ${PROJECT_SOURCE_DIR}/mtest/cmake.inc was added twice in mtest/libmscore/all_elements/CMakeLists.txt

@Jojo-Schmitz
Copy link
Contributor

mtest/libmscore/all_elements/tst_tree_model.cpp Outdated Show resolved Hide resolved
mtest/libmscore/all_elements/tst_tree_model.cpp Outdated Show resolved Hide resolved
mtest/libmscore/all_elements/tst_tree_model.cpp Outdated Show resolved Hide resolved
mtest/libmscore/all_elements/tst_tree_model.cpp Outdated Show resolved Hide resolved
mtest/libmscore/all_elements/CMakeLists.txt Show resolved Hide resolved
Renamed the layout_elements folder to all_elements, for tests that
need to check every element and don't need reference files. Added
tst_tree_model to all_elements directory.
The test checks that for every element el, `el->treeChild(i)->parent`
should be `el`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants