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

Differentiated MSCZ and MSCX Document Icons #5421

Closed
wants to merge 271 commits into from
Closed

Differentiated MSCZ and MSCX Document Icons #5421

wants to merge 271 commits into from

Conversation

liamrosenfeld
Copy link
Contributor

@liamrosenfeld liamrosenfeld commented Oct 25, 2019

This brings the separate MSCZ and MSCX document icons currently found on Linux targets over to macOS. I would be happy to also add them on Windows if someone could point me in the right direction.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Oct 28, 2019

I believe for Windows the icons get set in build/packaging/WIX.template.in, lines 117-132, which in turn refererences mscore/data/mscorefile.ico in mscore/data/mscore.rc, line 5

@liamrosenfeld
Copy link
Contributor Author

@Jojo-Schmitz, thank you! I updated the PR with those files modified.

@anatoly-os anatoly-os added this to the MuseScore 3.5 milestone Dec 16, 2019
@anatoly-os anatoly-os self-requested a review December 16, 2019 17:17
@Jojo-Schmitz
Copy link
Contributor

This needs a rebase too

@liamrosenfeld
Copy link
Contributor Author

@Jojo-Schmitz, I rebased. 👍

@anatoly-os, I need similar guidance on this PR on to add a new icon to the Windows target?

@@ -652,7 +652,12 @@ else (MINGW)
PROPERTIES
LINK_FLAGS "-stdlib=libc++"
)
xcode_pch(mscoreapp all)
xcode_pch(mscore all)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should rather stay mscoreapp

Comment on lines +656 to +659
install (TARGETS mscore BUNDLE DESTINATION ${CMAKE_INSTALL_PREFIX})
install (FILES data/mscore.icns DESTINATION ${Mscore_SHARE_NAME}${Mscore_INSTALL_NAME})
install (FILES data/msczDocument.icns DESTINATION ${Mscore_SHARE_NAME}${Mscore_INSTALL_NAME})
install (FILES data/mscxDocument.icns DESTINATION ${Mscore_SHARE_NAME}${Mscore_INSTALL_NAME})
Copy link
Contributor

Choose a reason for hiding this comment

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

install statements are better now to go to main/CMakeLists.txt

dmitrio95 and others added 22 commits March 2, 2020 10:03
- Move functions of `BendCanvas` class to `bendcanvas.cpp`
- Move existing definition and functions of `InspectorBend` class to `inspectorBend.h` and `inspectorBend.cpp`
- Move processing of bend properties to `inspectorBend.cpp`
- Remove Bend Properties dialogue
Resolves: https://musescore.org/node/270589.

A "Custom" option is added to get the right behaviour of bend type.
+ move definition of `FretCanvas` functions to `fretcanvas.cpp`
 - Remove unused EditData::init() function
 - Remove unused EditData::duration field
 - Prevent memory leaks with ElementEditData descendants
dmitrio95 and others added 17 commits March 2, 2020 10:04
Resolves: https://musescore.org/en/node/301436

The accessibility navigation commands
(Alt+Left/Right, also Ctrl+Alt+Shift+Left/Right)
were not properly checking for mmrests,
resulting in selection of elements in the underlying measures
that were not valid in the current layout.
This adds the necessary checks.
Mostly just a matter of adding "MM" to various function calls.
In a couple of places, the appropriate function did not exist,
so I added it.
Also corrected errors in Ctrl+Alt+Shift+Left/Right
that occurs when going past the end of a staff,
the code to wrap around to the next staff this case well.
In part this is because the implementation of barlines changed
since the code was written.
Barlines are per-staff now even when spanned,
so the use and management of prevTrack is no longer appropriate.
Fixed a problem that caused the “Selection Filter” item in the “View Menu” to be unchecked at application startup when the “Selection Filter” panel was in fact visible.
…cores imported from 2.x

Resolves: https://musescore.org/en/node/301478.

When importing scores from 1.x or 2.x, the property flags for styled properties must be set to UNSTYLED. Otherwise, the properties will not be written when the score is saved. ScoreElement::readProperty() should be used wherever possible, since it takes care of setting the property flags correctly. When it is not possible to use ScoreElement::readProperty(), ScoreElement::setPropertyFlags() must be called after setting the property.
Resolves: https://musescore.org/en/node/301605.

Prior to this commit, ScoreView::cmdAddSlur() could be called multiple times from within ScoreView::addSlur(), resulting in the need for multiple "undo" commands to completely undo a single "add-slur" command. This commit swaps the names of these two functions, and, more importantly, only calls Score::startCmd() and Score::endCmd() once for the entire operation.
…additional tie

Resolves: https://musescore.org/en/node/301414.

During paste, if a note has a tie, that tie is remembered as a pending connector which is later added to the score when XmlReader::checkConnectors() is called. If a note is too long to fit in the measure, it is split up into a series of tied notes, but the pending tie needs to be adjusted to begin at the last tied note in the series, instead of at the original note.
…pitch

to make it consistent with "Guitar (Treble Clef)" and
"Acoustic Guitar (Treble Clef)"
Also adjust gpx mtest ref-files.
Resolves: https://musescore.org/en/node/301496

Alt+Left/Right commands were skipping voltas because
we were checking start element and checking against the active staff,
but the start element is actually the measure for voltas.
The main change here is to go ahead and visit the volta
if the active staff if you are navigating the top staff.
Arguably, it could make sense to check for the top *visible* staff,
since that is what the volta at least appears to be attached to.
So I have code here to that.
But I disabled it because in practice,
neither the navigation commands themsevles nor the screen reader
treat invisible staves specially.
So a blind user navigating would have no way of knowing
the top staff is not visible.
So they would likely continue to see it as relevant.

I would not the same issue occurs for system text,
which we always treat as attached to the top staff only.
I added a TODO to indicate where this code would need updating.

Eventually we could consider coming up with some way
of presenting information about hidden staves.
Perhaps in conjunction with a facility allow user
to hide staves on specific systems only,
which seems to be a fairly common request.
@liamrosenfeld
Copy link
Contributor Author

My branch got messed up and all the files got overridden, so I'm just going to close this

@liamrosenfeld liamrosenfeld deleted the matching-mscx branch March 2, 2020 15:29
@Jojo-Schmitz
Copy link
Contributor

Try git pull --rebase musescore master; git push -f

@liamrosenfeld liamrosenfeld restored the matching-mscx branch March 2, 2020 15:41
@liamrosenfeld liamrosenfeld deleted the matching-mscx branch March 2, 2020 15:43
@liamrosenfeld
Copy link
Contributor Author

liamrosenfeld commented Mar 2, 2020

Thank you for the speedy response—I appreciate it. I did but github desktop discarded my stash when I changed branches so all the icon files were lost.

@dmitrio95
Copy link
Contributor

My review comments seem to belong to this commit: ee0b4be. Try to check it out locally (it should probably be still preserved in your local repo) and new icons should be restored. Actually you can try to cherry-pick that commit on top of current master branch, that should effectively do the same as rebase, as this branch is intended to have one extra commit compared to master anyway.

@liamrosenfeld liamrosenfeld restored the matching-mscx branch March 2, 2020 20:34
@liamrosenfeld
Copy link
Contributor Author

liamrosenfeld commented Mar 2, 2020

Thank you so much, @dmitrio95! Github won't allow me to reopen this PR because of the force push that was required (and a bit more—see below) so I opened up a new one at #5779. I implemented your code review changes on that one too.

@Jojo-Schmitz
Copy link
Contributor

Sorry, I don't buy that. I'm using git push -f all the time, never had to close and redo PR because of it

@liamrosenfeld
Copy link
Contributor Author

liamrosenfeld commented Mar 2, 2020

The button was grayed out and said something along the lines that I couldn't because I force pushed onto a deleted branch and then republished that branch. It is now grayed out for a different reason because there is now a different open PR for that branch.

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.

None yet