-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix #16077 part 2: Double click a header/footer to open the corresponding menu #5887
Conversation
mscore/editstyle.cpp
Outdated
@@ -735,6 +735,7 @@ const std::map<ElementType, EditStylePage> EditStyle::PAGES = { | |||
{ ElementType::FIGURED_BASS, &EditStyle::PageFiguredBass }, | |||
{ ElementType::HARMONY, &EditStyle::PageChordSymbols }, | |||
{ ElementType::FRET_DIAGRAM, &EditStyle::PageFretboardDiagrams }, | |||
{ ElementType::TEXT, &EditStyle::PageHeaderFooter }, // gotoElement if the element is Text leads to the footer dialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this also catch other text elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will, as I explained in the original message. It does not cause an immediate problem. In the future, it might, but adding friends is also a problem and this is very elegant (if I may so myself) because it does not change much but acomplishes it's goal. In my opinion headers/footers need to be reworked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, this adds "Style..." to the context menu for every text element, and all of them go to the Header/Footer page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I hadn't seen that connection. I'll change this then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IsaacWeiss I can think of 3 solutions. Either through 'friend' (like the PR mentioned in the op) or by hardcoding the value of the page and having a unit test to check if the value is correct. I could also create a special 'gotoFooterHeader' for this case.
mscore/events.cpp
Outdated
cout << "success" << endl; | ||
} | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SKefalidis unaddressed comment ^
mscore/events.cpp
Outdated
return; | ||
else if (!clickedElement->isEditable()) { | ||
if (clickedElement->isText() | ||
&& (static_cast<Text*>(clickedElement)->tid() == Tid::FOOTER || static_cast<Text*>(clickedElement)->tid() == Tid::HEADER)) { // double-click an instrument name to open the edit staff/part properties menu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use toText(clickedElement)
as a shortcut for the static_cast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
mscore/events.cpp
Outdated
if (clickedElement->isText() | ||
&& (static_cast<Text*>(clickedElement)->tid() == Tid::FOOTER || static_cast<Text*>(clickedElement)->tid() == Tid::HEADER)) { // double-click an instrument name to open the edit staff/part properties menu | ||
elementPropertyAction("style", clickedElement); | ||
cout << "success" << endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging artifact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, this one got away, will fix that.
mscore/scoreview.cpp
Outdated
@@ -4727,6 +4727,10 @@ QList<Element*> ScoreView::elementsNear(QPointF p) | |||
QRectF r(p.x() - w, p.y() - w, 3.0 * w, 3.0 * w); | |||
|
|||
QList<Element*> el = page->items(r); | |||
if(score()->headerText() != nullptr) // gives the ability to select the header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space after if
Fixed formatting. |
de8bf75
to
18e458d
Compare
Everything should be in order now. |
libmscore/score.cpp
Outdated
headerText()->styleChanged(); | ||
if (footerText()) | ||
footerText()->styleChanged(); | ||
for(int i = 0; i < 3; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space after for
libmscore/score.cpp
Outdated
headerText()->styleChanged(); | ||
if (footerText()) | ||
footerText()->styleChanged(); | ||
for(int i = 0; i < 3; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here you need to add a comment what is 3
libmscore/score.h
Outdated
Text* _headerText { 0 }; | ||
Text* _footerText { 0 }; | ||
vector<Text*> _headersText { nullptr, nullptr, nullptr }; | ||
vector<Text*> _footersText { nullptr, nullptr, nullptr }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add std
before vector
(std::vector
)
libmscore/score.h
Outdated
Text* _headerText { 0 }; | ||
Text* _footerText { 0 }; | ||
vector<Text*> _headersText { nullptr, nullptr, nullptr }; | ||
vector<Text*> _footersText { nullptr, nullptr, nullptr }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here you need to add a comment why three
mscore/events.cpp
Outdated
return; | ||
else if (!clickedElement->isEditable()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no need for else
if (!clickedElement)
return;
if (!clickedElement->isEditable()) {
...
return
}
...
mscore/inspector/inspector.cpp
Outdated
ie = new InspectorText(this); | ||
if (element()->isText()) { | ||
// don't allow footers/headers to be edited via the inspector | ||
if(toText(element())->tid() != Tid::FOOTER && toText(element())->tid() != Tid::HEADER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space after if
mscore/scoreview.cpp
Outdated
@@ -4727,6 +4727,12 @@ QList<Element*> ScoreView::elementsNear(QPointF p) | |||
QRectF r(p.x() - w, p.y() - w, 3.0 * w, 3.0 * w); | |||
|
|||
QList<Element*> el = page->items(r); | |||
for(int i = 0; i < 3; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here you need to add a comment what is 3
I think it's even better to add a constant (mscore.h
), since it is used in several places.
The name of the constant will show what it is and where it is used.
9d7f417
to
79fd942
Compare
Updated as advised by @igorkorsukov |
libmscore/page.cpp
Outdated
} | ||
} | ||
else { | ||
text = score()->footerText(); | ||
text = score()->footerText(area - 3); // because they are 3 4 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it better to replace area - 3
with area - MAX_FOOTERS
?
mscore/events.cpp
Outdated
cout << "success" << endl; | ||
} | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SKefalidis unaddressed comment ^
@SKefalidis two comments remain unaddressed and rebase is needed. |
…corresponding menu
@anatoly-os Rebase completed, suggestions incorporated. |
Resolves: https://musescore.org/en/node/16077
Related PR: #5886
Added the ability to double click a header/footer to go their menu (dialog).
First of all, to be able to double click you need to be able to select. Headers/footers were not selectable because they were not in Page->items, which means that they were not in the bspTree. Adding them to the bspTree causes graphical glitches and it requires extensive workarounds to bypass the constness of Page::draw. That's why they are added manually in ScoreView::elementsNear.
After giving them the ability to be selected, they need to be non-editable, which means setting their flag to generated and disabling their inspector. Editing them via the inspector causes more graphical glitches and makes the logic in ScoreView::mouseDoubleClickEvent more complex. If we want the ability to edit them via the inspector, more extensive changes have to be made.
Edited for the latest push: I changed the way headers/footers are stored in the program to enable the ability to click any one of the 3 headers/footers to go to the dialog. While doing that I changed the pointer initialization to nullptr and converted some things to the new representation (which uses vectors).