Added Capo transposition modes #29367
Conversation
src/engraving/dom/noteentry.cpp
Outdated
| @@ -66,7 +66,7 @@ NoteVal Score::noteValForPosition(Position pos, AccidentalType at, bool& error) | |||
| ClefType clef = st->clef(tick); | |||
| const Instrument* instr = st->part()->instrument(s->tick()); | |||
| NoteVal nval; | |||
| const StringData* stringData = 0; | |||
| const StringData* stringData = st->part()->stringData(s->tick(), st->idx()); | |||
There was a problem hiding this comment.
It's better to declare it where it's needed
src/engraving/dom/staff.cpp
Outdated
| } | ||
|
|
||
| void Staff::insertCapoParams(const Fraction& tick, const CapoParams& params) | ||
| void Staff::applyCapoTranspose(int startTick, int endTick, const CapoParams& params, int notePitchCorrection /* = 0 */) |
There was a problem hiding this comment.
This function is very hard to read. A lot is going on in it, with a lot of nested code. It would be better to split it into several smaller functions
| @@ -77,7 +81,7 @@ public slots: | |||
|
|
|||
| private: | |||
| const mu::engraving::CapoParams& params() const; | |||
|
|
|||
| int m_transposeMode; | |||
There was a problem hiding this comment.
| int m_transposeMode; | |
| int m_transposeMode = 0; |
| @@ -37,6 +37,7 @@ class CapoSettingsModel : public AbstractElementPopupModel | |||
| { | |||
| Q_OBJECT | |||
|
|
|||
| Q_PROPERTY(uint8_t transposeMode READ transposeMode WRITE setTransposeMode NOTIFY transposeModeChanged) // CapoParams::TransposeMode | |||
There was a problem hiding this comment.
| Q_PROPERTY(uint8_t transposeMode READ transposeMode WRITE setTransposeMode NOTIFY transposeModeChanged) // CapoParams::TransposeMode | |
| Q_PROPERTY(int transposeMode READ transposeMode WRITE setTransposeMode NOTIFY transposeModeChanged) // CapoParams::TransposeMode |
| navigation.accessible.name: titleLabel.text + " " + currentText | ||
|
|
||
| model: [ | ||
| { text: qsTrc("global", "Affects playback only") }, |
There was a problem hiding this comment.
It should be "notation" instead of "global"
alexpavlov96
force-pushed
the
gp-capo-import
branch
from
3248dd8 to
0b18ac2
Compare
…opening file and pressing "undo"
| @@ -282,6 +283,7 @@ class Staff final : public EngravingItem | |||
| friend class Excerpt; | |||
| void setVoiceVisible(voice_idx_t voice, bool visible); | |||
| void updateVisibilityVoices(const Staff* masterStaff, const TracksMap& tracks); | |||
| void applyCapoTranspose(int startTick, int endTick, const CapoParams& params, int notePitchCorrection = 0); | |||
There was a problem hiding this comment.
I think these new functions (and all of the helper functions in the cpp file) should not be in the Staff class because they do a lot of functionality, and we try to keep only the essential engraving information inside our classes. For these functions, I would make a separate helper class, which could be called EditCapos or something like that, on separate .cpp and .h files, and make these all static functions. See for example the EditTimeTickAnchors and MoveElementAnchors classes for a similar pattern.
| @@ -310,7 +312,25 @@ class Staff final : public EngravingItem | |||
|
|
|||
| std::map<int, int> m_channelList[VOICES]; | |||
| std::map<int, SwingParameters> m_swingList; | |||
| std::map<int, CapoParams> m_capoMap; | |||
|
|
|||
| class SelectionKeeper | |||
There was a problem hiding this comment.
This also shouldn't be in the Staff class, it can go in the separate file too. It also makes me wonder is it really necessary to select/deselect everything during this operation?
| NOTATION_ONLY = 1, | ||
| TAB_ONLY = 2, | ||
| }; | ||
| enum class Transition { |
There was a problem hiding this comment.
This enum looks quite strange. A struct like CapoParams is meant to hold data about Capo. This Transition enum doesn't look like data, it seems to just describe the set of operations that needs to be done when changing the capo parameters, so it's like if the elements of the enum are not representing data, but functions. It also seems that these Transition parameters don't exist independently, cause they are set in Capo::setProperty depending on the TransposeMode parameter.
My feeling is that this enum could be eliminated and should be just replaced by functions, but I don't know enough about this functionality to know for sure. If we want to keep it, then we should definitely:
- Move it out ot the CapoParams struct and of the Staff class
- Probably put it in the new EditCapo class together with the functions that will use it, and perhaps rename it something like CapoTransposeParams
- Construct it during the editing operations, not in Capo::setProperty
| struct CapoState { | ||
| CapoParams params; | ||
| bool needUpdate = false; | ||
| bool ignorePrevious = false; // If just added update according to previous capo if exists |
There was a problem hiding this comment.
Similar comment here. These two booleans look like information that should be retrieved/constructed during the editing operations, not information that should to be stored as a data member
No description provided.