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

Add auto-highlight scale and key selection #5196

Merged
merged 1 commit into from
Sep 30, 2020

Conversation

Veratil
Copy link
Contributor

@Veratil Veratil commented Sep 16, 2019

This adds a new key selection combobox to the Piano Roll next to the scale combobox. When a key+scale is selected, it will automatically highlight the scale on the selected key. If either is set back to "No key/scale" the highlights will be removed.

This doesn't remove any functionality already there with marking semitones, but is a QOL improvement saving steps when wanting to mark scales.

lmms-key-scale

lmms-key-selection

Additionally, as another small QOL improvement this links the toolbar icon with the combobox so when resized you don't get this:

lmms-toolbar-clip

@necrashter
Copy link
Contributor

Good idea but I think I found a bug:

  1. Open piano roll, mark a note.
  2. Right click on the marked note and select "Mark/Unmark all corresponding octave semitones"
    Program crashes with segfault. This doesn't happen in master branch.

Also, the combobox doesn't change if you mark a scale by right clicking on a note and selecting "Mark current scale". Is this the expected behavior?

@PhysSong
Copy link
Member

PhysSong commented Sep 20, 2019

@necrashter Because this branch doesn't contain the changes from #5137 which is merged into master. Should work now.

@Veratil Veratil force-pushed the autohighlight-scales branch 2 times, most recently from 2d58b29 to 8a98d2b Compare September 20, 2019 14:00
@Veratil Veratil added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Dec 7, 2019
@LmmsBot
Copy link

LmmsBot commented Sep 26, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://9083-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-715%2Bgc03e52c-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9083?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://9080-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-715%2Bgc03e52cec-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9080?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://9081-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-715%2Bgc03e52cec-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9081?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/ghcsojhhkysj5nwy/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35455326"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/4kbogs0h8tcg4ex2/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35455326"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://9079-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-715%2Bgc03e52cec-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9079?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "45a3b3e65803212580133ee83ad71618cf140aa9"}

include/PianoRoll.h Outdated Show resolved Hide resolved
Copy link
Member

@Spekular Spekular left a comment

Choose a reason for hiding this comment

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

No more complaints here, LGTM!

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
@Spekular
Copy link
Member

Worked fine for me when testing. I find that the key combobox looks a bit cramped when "No key" is displayed. Also, highlights only apply when I move my mouse after clicking, so if I select a different key and don't move my mouse the previous highlights stick around. However, this occurs for me with other builds as well (when manually highlighting notes) so it isn't an issue with your PR.

@Spekular
Copy link
Member

Actually, I did find one issue that I can't reproduce on builds of my own branch. The vertical zoom combobox is misplaced (on top of the transport controls) and missing it's icon.

@Veratil
Copy link
Contributor Author

Veratil commented Sep 27, 2020

I find that the key combobox looks a bit cramped when "No key" is displayed.

Ah you're right, I never noticed I guess since I know what it says. I'll see about expanding it a few pixels.

Also, highlights only apply when I move my mouse after clicking, so if I select a different key and don't move my mouse the previous highlights stick around. However, this occurs for me with other builds as well (when manually highlighting notes) so it isn't an issue with your PR.

I see this happens with the right-click menu. It could easily be adding an update() call after processing. I'll look into it for another PR.

@Veratil
Copy link
Contributor Author

Veratil commented Sep 27, 2020

Actually, I did find one issue that I can't reproduce on builds of my own branch. The vertical zoom combobox is misplaced (on top of the transport controls) and missing it's icon.

Ah that's because it was added after my PR. Fixing up now.

@Veratil
Copy link
Contributor Author

Veratil commented Sep 27, 2020

I added update(); at the end of markSemiTone so it properly redraws now. Squashed all commits into 1 with rebase.

@Spekular
Copy link
Member

Tested again after the recent changes, all my concerns are addressed and the feature still works as expected 👍

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

Only doxygen comments reviewed.

There are still open questions about matter of taste:

  • Maybe we should set JAVADOC_AUTOBRIEF = YES and QT_AUTOBRIEF = YES in doc/Doxyfile.in. This would simply make the first sentence (until the first dot) \brief (without use of the \brief command).
  • Do we want to use @param vs \param, //! vs ///, /** vs /*! , or should we just allow both?

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
@Veratil
Copy link
Contributor Author

Veratil commented Sep 29, 2020

Only doxygen comments reviewed.

There are still open questions about matter of taste:

* Maybe we should set `JAVADOC_AUTOBRIEF = YES` and `QT_AUTOBRIEF = YES` in `doc/Doxyfile.in`. This would simply make the first sentence (until the first dot) `\brief` (without use of the `\brief` command).

* Do we want to use `@param` vs `\param`, `//!` vs `///`, `/**` vs `/*!` , or should we just allow both?

I think this is a good discussion to move to an issue, rather than start it on this PR. I can remove the doc comments for now from this PR in favor of adding them in a future PR?

@Veratil Veratil merged commit 31f79a2 into LMMS:master Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants