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

Fix #3005 #5262

Merged
merged 2 commits into from
Nov 1, 2019
Merged

Fix #3005 #5262

merged 2 commits into from
Nov 1, 2019

Conversation

Veratil
Copy link
Contributor

@Veratil Veratil commented Oct 20, 2019

mouseDoubleClickEvent wasn't forwarding to mousePressEvent like the default implementation does. This adds that check.

I don't believe this should cause any problems double clicking elsewhere (I can't seem to find any problems with a basic test), but just in case we can restrict this to just the piano key area.

@LmmsBot
Copy link

LmmsBot commented Oct 20, 2019

🤖 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

🤖
{"commit_sha": "b416186bc886df9062c9f6a463940b3ac5061213", "platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://4915-15778896-gh.circle-artifacts.com/0/lmms-1.2.0.554-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/4915?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://4917-15778896-gh.circle-artifacts.com/0/lmms-1.2.0.554-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/4917?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://4918-15778896-gh.circle-artifacts.com/0/lmms-1.2.0.554-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/4918?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}}

@Veratil Veratil added the needs testing This pull request needs more testing label Oct 22, 2019
@PhysSong
Copy link
Member

mouseDoubleClickEvent wasn't forwarding to mousePressEvent like the default implementation does.

Then why don't you call the default implementation instead of calling mousePressEvent directly? I understand both give the same result, but this will make it easy to see what it does.

@Veratil
Copy link
Contributor Author

Veratil commented Oct 25, 2019

mouseDoubleClickEvent wasn't forwarding to mousePressEvent like the default implementation does.

Then why don't you call the default implementation instead of calling mousePressEvent directly? I understand both give the same result, but this will make it easy to see what it does.

I don't understand, is this not easy to see what it does?

@zonkmachine
Copy link
Member

Tests fine!

Copy link
Member

@lukas-w lukas-w left a comment

Choose a reason for hiding this comment

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

@PhysSong's suggestion is the standard way of doing this, see https://doc.qt.io/qt-5/eventsandfilters.html#event-handlers for reference. We don't act on the event, so we should let the base class handle it.

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
@lukas-w lukas-w removed the needs testing This pull request needs more testing label Oct 31, 2019
Co-Authored-By: Lukas W <lukaswhl@gmail.com>
@Veratil
Copy link
Contributor Author

Veratil commented Oct 31, 2019

@PhysSong's suggestion is the standard way of doing this, see https://doc.qt.io/qt-5/eventsandfilters.html#event-handlers for reference. We don't act on the event, so we should let the base class handle it.

Ah! I understand now. 😄 Thanks!

@lukas-w lukas-w merged commit ebf7100 into LMMS:master Nov 1, 2019
@lukas-w
Copy link
Member

lukas-w commented Nov 1, 2019

Merged and cherry-picked to stable-1.2 via a8d91b1.

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.

5 participants