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

Feat: Restoring instruments list while switching back and forth between in instruments and templates #25019

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chilkaditya
Copy link
Contributor

Resolves: #14342

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)


StyledDialogView {
id: root

required property InstrumentsOnScoreListModel instrumentsOnScoreModel
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not work: InstrumentsDialog is not instantiated directly from QML, but as a dialog. Therefore, it won't be possible to pass anything to this required property, and the dialog will fail to open.

Instead, you will need to instantiate an InstrumentsOnScoreListModel here inside InstrumentsDialog, just like you do in ChooseInstrumentsAndTemplatesPage.


Item {
id: root
required property InstrumentsOnScoreListModel instrumentsOnScoreModel

readonly property bool hasInstruments: instrumentsOnScoreModel.count > 0
readonly property alias isMovingUpAvailable: instrumentsOnScoreModel.isMovingUpAvailable
Copy link
Contributor

Choose a reason for hiding this comment

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

The errors you are seeing are caused by these two aliases:

13:08:15.122 | DEBUG | main_thread     | createDialog    | [qml] failed create component: ../../MuseScore/Project/NewScoreDialog.qml, err: qrc:/qml/MuseScore/Project/NewScoreDialog.qml:87 Type ChooseInstrumentsAndTemplatesPage unavailable
qrc:/qml/MuseScore/Project/internal/NewScore/ChooseInstrumentsAndTemplatesPage.qml:174 Type ChooseInstrumentsPage unavailable
qrc:/qml/MuseScore/InstrumentsScene/ChooseInstrumentsPage.qml:196 Type InstrumentsOnScoreView unavailable
qrc:/qml/MuseScore/InstrumentsScene/internal/InstrumentsOnScoreView.qml:36 Invalid alias reference. Unable to find id "instrumentsOnScoreModel"

Indeed, instrumentsOnScoreModel is not an id anymore in this context, but a property. That means that it cannot be used for alias properties.
Fortunately, the solution is easy: replace alias with bool.
But there is also another solution, that may be even better. If you look where isMovingUpAvailable and isMovingDownAvailable are used, you see that they are only used in ChooseInstrumentsPage.qml. But now that file has direct access to the instrumentsOnScoreModel, so there is no need anymore to take these properties via InstrumentsOnScoreView.
So, the solution I would recommend is:

  • remove these two alias properties from InstrumentsOnScoreView
  • In ChooseInstrumentsPage, replace instrumentsOnScoreView.isMovingUpAvailable with root.instrumentsOnScoreModel.isMovingUpAvailable, and similar for isMovingDownAvailable.

While at it, you'll probably notice that InstrumentsOnScoreView contains a lot of functions (instruments(), moveSelectedInstrumentsDown(), etc) that are only for the purpose of exposing information of instrumentsOnScoreModel. But again, that is not necessary anymore, because now instrumentsOnScoreModel is available directly. So, it would be great to remove those functions, and use instrumentsOnScoreModel directly in ChooseInstrumentsPage.

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 is kind of frustrating for me that after so much of instructions I could not find out the correct solution but I am listing my progress. @cbjeukendrup As per your instructions I update the InstrumentsDialog.qml, ChooseInstrumentsPage.qml and InstrumentsOnScoreView.qml. And code of those files are as follows:
InstrumentsDialog.qml

StyledDialogView {
    ......
    InstrumentsOnScoreListModel {
        id: instrumentsOnScoreModel
    }
    ......
}

ChooseInstrumentsPage.qml

function instruments() {
        if (root.canSelectMultipleInstruments) {
            return root.instrumentsOnScoreModel.instruments() // changed code
        }

        return [ instrumentsModel.selectedInstrument ]
}
function currentOrder() {
        if (root.canSelectMultipleInstruments) {
            return root.instrumentsOnScoreModel.currentOrder() // changed code
        }

        return undefined
}
function addSelectedInstrumentsToScore() {
        root.instrumentsOnScoreModel.addInstruments(instrumentsModel.selectedInstrumentIdList()) // changed code

        Qt.callLater(root.instrumentsOnScoreModel.scrollViewToEnd) // changed code
}
FlatButton {
        enabled: root.instrumentsOnScoreModel.isMovingUpAvailable // changed code
}
FlatButton {
        enabled: root.instrumentsOnScoreModel.isMovingDownAvailable // changed code
}

And I deleted two alice properties isMovingUpAvailable, isMovingDownAvailable and all those functions instruments(), currentOrder(), addInstruments, moveSelectedInstrumentsUp(), moveSelectedInstrumentsDown(), scrollViewToEnd() from the InstrumentsOnScoreView.qml.
After this Newscore creation dialog is opening but i can not add instruments to the list and It is showing in application output that
error: qrc:/qml/MuseScore/InstrumentsScene/ChooseInstrumentsPage.qml:85: TypeError: Cannot read property 'addInstruments' of null

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that scrollViewToEnd() is different; that one will have to stay in InstrumentsOnScoreView.qml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes Yes, it is there in the InstrumentsOnScoreView.qml ,I mistakenly add in the comment.

Comment on lines 64 to 66

InstrumentsOnScoreListModel {
id: instrumentsOnScoreModel
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to delete unnecessary whitespace; one empty line is enough :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, got it

Item {
id: root

required property InstrumentsOnScoreListModel instrumentsOnScoreModel
Copy link
Contributor

Choose a reason for hiding this comment

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

This property should not be here. In this file, the InstrumentsOnScoreListModel is instantiated, so there should be no property. Properties are usually only used to pass information from outside the file into the file, or to expose information from inside the file to outside the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, got it.

@@ -168,6 +173,7 @@ Item {

ChooseInstrumentsPage {
navigationSection: root.navigationSection
instrumentsOnScoreModel: instrumentsOnScoreModel
Copy link
Contributor

Choose a reason for hiding this comment

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

QML might be confused by this: the instrumentsOnScoreModel after the : should refer to the instance of InstrumentsOnScoreListModel by its id, but perhaps QML thinks that it refers to the instrumentsOnScoreModel property of ChooseInstrumentsPage, so we're assigning the property to itself.

Easy way around it: replace id: instrumentsOnScoreModel with id: theInstrumentsOnScoreModel, and then write instrumentsOnScoreModel: theInstrumentsOnScoreModel here.

@@ -27,10 +27,11 @@ import Muse.Ui 1.0
import Muse.UiComponents 1.0
import MuseScore.Project 1.0
import MuseScore.InstrumentsScene 1.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary deletion

@chilkaditya
Copy link
Contributor Author

@cbjeukendrup I commit my progress. Not getting any error but not restoring the instruments list while switching between two. Main logic is not working, I think we have to do some changes in addSelectedInstrumentsToScore() function in ChooseInstrumentsPage.qml. There we call addInstruments() that is not exist anymore.
Please ignore code comments as of now, I will delete those in my final and working commit.

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.

[MU4 Issue] New Score creation: switching back and forth between instruments and templates
2 participants