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.

Sign up for GitHub

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

Jump to bottom

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
+43 −27
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
68829f3
restoring instruments list while switching back and forth between ins…
chilkaditya Oct 3, 2024
f426972
fixed invalid alice reference error regarding the restoring instrumen…
chilkaditya Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/instrumentsscene/qml/MuseScore/InstrumentsScene/ChooseInstrumentsPage.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@ import QtQuick.Layouts 1.15
import Muse.Ui 1.0
import Muse.UiComponents 1.0
import MuseScore.InstrumentsScene 1.0
import MuseScore.Project 1.0

import "internal"

Rectangle {
id: root
required property InstrumentsOnScoreListModel instrumentsOnScoreModel

property bool canSelectMultipleInstruments: true
property string currentInstrumentId: ""
Expand Down Expand Up @@ -201,6 +203,7 @@ Rectangle {

Layout.fillWidth: true
Layout.fillHeight: true
instrumentsOnScoreModel: root.instrumentsOnScoreModel
}

SeparatorLine {
Expand Down
3 changes: 3 additions & 0 deletions src/instrumentsscene/qml/MuseScore/InstrumentsScene/InstrumentsDialog.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ import QtQuick.Layouts 1.15
import Muse.Ui 1.0
import Muse.UiComponents 1.0
import MuseScore.InstrumentsScene 1.0
import MuseScore.Project 1.0

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.

property bool canSelectMultipleInstruments: true
property string currentInstrumentId: ""

Expand Down Expand Up @@ -70,6 +72,7 @@ StyledDialogView {
onSubmitRequested: {
root.submit()
}
instrumentsOnScoreModel: instrumentsOnScoreModel
}

RowLayout {
Expand Down
6 changes: 3 additions & 3 deletions src/instrumentsscene/qml/MuseScore/InstrumentsScene/internal/InstrumentsOnScoreView.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ import QtQuick.Controls 2.12
import Muse.Ui 1.0
import Muse.UiComponents 1.0
import MuseScore.InstrumentsScene 1.0
import MuseScore.Project 1.0

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.

Expand Down Expand Up @@ -60,9 +62,7 @@ Item {
instrumentsView.positionViewAtEnd()
}

InstrumentsOnScoreListModel {
id: instrumentsOnScoreModel
}


Comment on lines 64 to 66
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

Component.onCompleted: {
instrumentsOnScoreModel.load()
Expand Down
8 changes: 7 additions & 1 deletion src/project/qml/MuseScore/Project/internal/NewScore/ChooseInstrumentsAndTemplatesPage.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

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.


property string preferredScoreCreationMode: ""

property string description: Boolean(currentPage) ? currentPage.description : ""
Expand Down Expand Up @@ -140,6 +141,10 @@ Item {
}
}

InstrumentsOnScoreListModel {
id: instrumentsOnScoreModel
}

Loader {
id: pageLoader

Expand Down Expand Up @@ -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.

}
}

Expand Down