Automation point mouse wheel and double click (old) #5232

tecknixia · 2019-10-08T04:47:45Z

Edit: Replaced by #5291 and #5292

Related to issue #5225

Allows user to place cursor over an automation point in the Automation Editor, and use scroll wheel to adjust the y level for that point.

https://cdn.discordapp.com/attachments/332258319228207114/629248936099708928/AutomationEditor2-2019-10-03_04.21.20.mp4 (example, not current)

I didn't know where to put the variable, so I put it where I thought it might fit. I'm still getting familiar with the codebase. Also, I was wondering if "point" is a good term for this or not. So please let me know if I should change something.

LmmsBot · 2019-10-08T05:22:22Z

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request!

Linux

Linux (AppImage): lmms-1.2.0.565-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.2.0.565-win32.exe (build link)
Windows 64-bit: lmms-1.2.0.565-win64.exe (build link)
Windows 32-bit: lmms-1.2.0-win32.exe (build link)
Windows 64-bit: lmms-1.2.0-win64.exe (build link)

🤖

{"commit_sha": "4bc5a7f26c67726e5bf70302656437c09872a9b8", "platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://4500-15778896-gh.circle-artifacts.com/0/lmms-1.2.0.565-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/4500?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://4499-15778896-gh.circle-artifacts.com/0/lmms-1.2.0.565-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/4499?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://4501-15778896-gh.circle-artifacts.com/0/lmms-1.2.0.565-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/4501?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/qe8xltdvsjx1tm2l/artifacts/build/lmms-1.2.0-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/28167958"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/l69mqsaff35st731/artifacts/build/lmms-1.2.0-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/28167958"}]}}

tecknixia · 2019-10-08T05:34:52Z

I just noticed that I didn't account for different number scales. So, even if the increments are whole numbers, it still adjusts in increments of three decimal places.

Spekular · 2019-10-08T08:57:17Z

I used "numPoints" in the automation pattern flipping PR, so "points" sounds consistent to me.

…

On Tue, Oct 8, 2019, 07:34 tecknixia ***@***.***> wrote: I just noticed that I didn't account for different number scales. So, even if the increments are whole numbers, it still adjusts in increments of three decimal places. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5232?email_source=notifications&email_token=ACEBLGWUTGWPG65DSBAL3Q3QNQLYFA5CNFSM4I6M4GD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAS2AOQ#issuecomment-539336762>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACEBLGWJPXXULHFQEWJNZ63QNQLYFANCNFSM4I6M4GDQ> .

tecknixia · 2019-10-10T14:37:23Z

I believe I have created a fix for the decimal point step changes using:

m_pattern->firstObject()->step<float>()

Is there a way to update the pull request? Do I close and create a new one?
Edit: nevermind

tecknixia · 2019-10-10T14:44:35Z

I figured it out.

Updating forked repo tecknixia/lmms

tecknixia · 2019-10-13T03:46:55Z

Seems to work fine.

Only quirky behavior that I've noticed so far is that it does not lock the Y axis while over a point. So if the scale for zoom for Y is larger than the window, the mouse will adjust the Y value for the point, and scroll the information in the window up or down at the same time. Slightly confusing at first, but still practical.

ghost

I'm done with style review.

src/gui/editors/AutomationEditor.cpp

tecknixia · 2019-10-15T01:28:31Z

@cyberdevilnl Thank you for the style review.

ghost · 2019-10-15T14:20:20Z

For updating the tooltip while wheeling you might want to add something like this:

Add to header:

	ulong m_pointYLevelTimestamp;

Change mouseMoveEvent:

	if (mouseEvent->timestamp() > m_pointYLevelTimestamp)
	{
		m_pointYLevel = 0;
	}

Your wheelEvent code:

	m_pointYLevelTimestamp = we->timestamp();

I don't know other solution for that.

tecknixia · 2019-10-15T16:35:20Z

Could you please explain? What is the reason for doing that? Is it better?

It already updates the tooltip when scrolling, and shows mouse pointer position when mouse is moved, which seems fine to me.

What it does not do is show the automation point's y level when mouse is over the automation point before scrolling, it just shows the pointer y position until the wheel is scrolled.

After that is solved, two other things can be done:

The window should not be able to scroll up and down while mouse pointer is over an automation point.
Double-clicking over an automation point should allow the user to enter in the y value for the automation point with the keyboard.

Since these two enhancements are not completely necessary, I feel this PR could be used for now (since it makes inputting a specific value much easier) and an issue for the additional enhancements could be opened after that. However, I leave that up for debate.

ghost · 2019-10-15T17:01:39Z

While wheeling over a point the tooltip doesn't get updated here, it's because wheelEvent also triggers a mouseMoveEvent here and then m_pointYLevel is reset to 0. I'm on Qt version 5.13.1-2 btw.

tecknixia · 2019-10-15T17:26:39Z

Ok, I understand now.

I was not familiar with the interaction between wheelEvent and mouseMove. It works for me (Qt 5.5.1 on Linux Mint 18.3 64-bit).

Maybe I can just set m_pointYLevel = 0 when the mouse x or y value changes... I'll play around with it and try your method. Thanks for the suggestion.

ghost · 2019-10-15T17:33:56Z

I also didn't know that a wheelEvent did trigger a mouseMoveEvent, I haven't found it in the Qt docs either. Maybe it's a Qt bug?

tecknixia · 2019-10-15T18:05:28Z

A condition in mouseMoveEvent to identify that the mouse pointer is over an automation point would solve this problem, and lay the foundation for resolving the other two issues. It may be worth the rewrite. However, it might be better to patch up this PR, because I don't know how long it'll take to rewrite. I'll do that first.

tecknixia · 2019-10-15T19:40:46Z

I think I figured out the reason this was never completed in the first place. I believe that this line:

else if( mouseEvent->buttons() & Qt::NoButton && m_editMode == DRAW )

Inside mouseMoveEvent was never true. First, I replaced the detection code with the one I was using for the scroll wheel. Then, since I wasn't getting a result, I moved the contained code to the end of the if else as an else.

Now I'm getting a cursor change when hovering over an automation point and the test value is showing up on the tooltip.

tecknixia · 2019-10-16T00:21:44Z

I fixed both the point level on hover and window scroll issues. Left a //TODO: for the future mouseDoubleClick enhancement.

A condition in mouseMoveEvent to identify that the mouse pointer is over an automation point would solve this problem, and lay the foundation for resolving the other two issues.

Well, it didn't quite go like I had planned. There are multiple mouse functions checking the time map for automation points. So the code could probably use some clever refactoring. I'm still not comfortable enough with C++ to take on such a task.

About to push a BIG commit...

It works well for me, so let me know if there are any problems.

Also, the scroll wheel engagement points go all the way up and down the Y axis. This can enable the user to retain the ability to change the level even when the mouse pointer is no longer on the automation point. However, it can also get in the way of the window scrolling up and down if the cursor happens to land in the wrong spot.

This is an easy two line modification of the if statement, and the size of the scroll wheel engagement point can be modified. Let me know how this works for you, or if I should reduce the size of the scroll wheel engagement point.

src/gui/editors/AutomationEditor.cpp

tecknixia · 2019-10-16T21:20:12Z

Ok, things should be good at this point.

I would like some feedback. Let me know if there is anything wrong, code or function.

Is this is functioning properly on other systems, and what do you think about user experience?

tecknixia · 2019-10-17T00:48:13Z

Two enhancements:

Added the ability to enter a value when double clicking on an automation point
Adjusted the area to only include functionality near the automation points.

My previous comment still stands. I would like some feedback.
Let me know if there is anything wrong, code or function.

stevphie · 2019-10-17T01:43:29Z

Two enhancements:
1. Added the ability to enter a value when double clicking on an automation point

2. Adjusted the area to only include functionality near the automation points.
My previous comment still stands. I would like some feedback.
Let me know if there is anything wrong, code or function.

I think the tittle does not reflect to what you just did (pointing at point No. 1)

EDIT: By that I mean your pull request title @tecknixia should be changed too maybe?

PhysSong · 2019-10-30T00:21:58Z

@tecknixia I'm trying to review this pull request, but it's not easy due to some irrelevant reformattings and reorderings. Could you revert those changes to make the review easier?

tecknixia · 2019-10-30T16:06:20Z

I'm trying to review this pull request, but it's not easy due to some irrelevant reformattings and reorderings

@PhysSong Sorry about that, I was thinking that might complicate things. Unfortunately, I mixed necessary changes with unnecessary ones in some commits. I've been thinking about starting over with a new master clone, and re-create the PR cleanly in another branch, but I'm not sure if I need to close and start a new one, or if I'm able to modify this one to that extent somehow?

ghost · 2019-10-30T16:22:49Z

Creating a new branch will require a new PR, that is fine; alternative you could reset you master branch to the LMMS upstream master branch and start from there.

This will make your master branch equal to the LMMS master branch. You will lose your changes!

git remote add upstream https://github.com/lmms/lmms
git fetch upstream
git checkout master # checkout your master branch.
git reset --hard upstream/master # reset to upstream master branch
git push origin master --force # force push your master branch.

tecknixia · 2019-10-31T01:38:30Z

@PhysSong I started over. I've closed this one and created two new ones to replace it: #5291 and #5292

PhysSong · 2019-10-31T01:39:37Z

@tecknixia Thanks. I will review them soon.

tecknixia added 3 commits October 7, 2019 20:34

Update AutomationEditor.h

8730b21

Update AutomationEditor.cpp

21336ea

Update AutomationEditor.h

d81e77d

Update AutomationEditor.cpp

df886b0

Merge remote-tracking branch 'upstream/master'

0a9d6d9

Updating forked repo tecknixia/lmms

tecknixia mentioned this pull request Oct 13, 2019

Automation point adjustment mouse wheel and double click #5225

Closed

PhysSong added needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR labels Oct 13, 2019

ghost suggested changes Oct 14, 2019

View reviewed changes

tecknixia added 3 commits October 14, 2019 21:21

Style review fixes

2bb7a3e

Style Review Addition

453cad5

Fixed Indentation Inconsistencies

9c5aaca

Removed Spaces Within Parameters

fd2fab0

Tooltip Display Fix

b6b6aac

tecknixia added 2 commits October 15, 2019 18:26

Rewrite of Pull Request

12dc2d3

Merge remote-tracking branch 'upstream/master'

e9611f2

tecknixia commented Oct 16, 2019

View reviewed changes

src/gui/editors/AutomationEditor.cpp Outdated Show resolved Hide resolved

Whitespace fixes

41085b8

tecknixia added 2 commits October 16, 2019 18:47

Added Double Click Input Dialog Functionality

5e93d74

Engagement Area Enhancement

b6da78d

tecknixia added 2 commits October 16, 2019 21:15

Mouse Control Enhancements

05dbcb7

Merge remote-tracking branch 'upstream/master'

4bc5a7f

tecknixia changed the title ~~Automation Editor point adjustment with mouse wheel~~ Automation Editor point manipulation enhancements Oct 17, 2019

tecknixia changed the title ~~Automation Editor point manipulation enhancements~~ Automation point manipulation enhancements Oct 17, 2019

tecknixia changed the title ~~Automation point manipulation enhancements~~ Automation point mouse wheel and double click Oct 17, 2019

This was referenced Oct 18, 2019

Automation: points deleted while moving other points can be restored abnormally #4671

Open

Automation Editor Nodes Don't Always Delete #3301

Open

tecknixia changed the title ~~Automation point mouse wheel and double click~~ Automation point mouse wheel and double click (old) Oct 30, 2019

tecknixia closed this Oct 30, 2019

This was referenced Oct 30, 2019

Automation Editor point fine tuning with wheel scroll #5291

Closed

Automation Editor point fine tuning with double click #5292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automation point mouse wheel and double click (old) #5232

Automation point mouse wheel and double click (old) #5232

tecknixia commented Oct 8, 2019 •

edited

Loading

LmmsBot commented Oct 8, 2019 •

edited

Loading

Linux

Windows

tecknixia commented Oct 8, 2019

Spekular commented Oct 8, 2019 via email

tecknixia commented Oct 10, 2019 •

edited

Loading

tecknixia commented Oct 10, 2019

tecknixia commented Oct 13, 2019

ghost left a comment

tecknixia commented Oct 15, 2019

ghost commented Oct 15, 2019

tecknixia commented Oct 15, 2019 •

edited

Loading

ghost commented Oct 15, 2019

tecknixia commented Oct 15, 2019 •

edited

Loading

ghost commented Oct 15, 2019

tecknixia commented Oct 15, 2019

tecknixia commented Oct 15, 2019

tecknixia commented Oct 16, 2019 •

edited

Loading

tecknixia commented Oct 16, 2019

tecknixia commented Oct 17, 2019

stevphie commented Oct 17, 2019 •

edited

Loading

PhysSong commented Oct 30, 2019

tecknixia commented Oct 30, 2019

ghost commented Oct 30, 2019

tecknixia commented Oct 31, 2019

PhysSong commented Oct 31, 2019

Automation point mouse wheel and double click (old) #5232

Automation point mouse wheel and double click (old) #5232

Conversation

tecknixia commented Oct 8, 2019 • edited Loading

Edit: Replaced by #5291 and #5292

LmmsBot commented Oct 8, 2019 • edited Loading

Linux

Windows

tecknixia commented Oct 8, 2019

Spekular commented Oct 8, 2019 via email

tecknixia commented Oct 10, 2019 • edited Loading

tecknixia commented Oct 10, 2019

tecknixia commented Oct 13, 2019

ghost left a comment

Choose a reason for hiding this comment

tecknixia commented Oct 15, 2019

ghost commented Oct 15, 2019

tecknixia commented Oct 15, 2019 • edited Loading

ghost commented Oct 15, 2019

tecknixia commented Oct 15, 2019 • edited Loading

ghost commented Oct 15, 2019

tecknixia commented Oct 15, 2019

tecknixia commented Oct 15, 2019

tecknixia commented Oct 16, 2019 • edited Loading

tecknixia commented Oct 16, 2019

tecknixia commented Oct 17, 2019

stevphie commented Oct 17, 2019 • edited Loading

PhysSong commented Oct 30, 2019

tecknixia commented Oct 30, 2019

ghost commented Oct 30, 2019

tecknixia commented Oct 31, 2019

PhysSong commented Oct 31, 2019

tecknixia commented Oct 8, 2019 •

edited

Loading

LmmsBot commented Oct 8, 2019 •

edited

Loading

tecknixia commented Oct 10, 2019 •

edited

Loading

tecknixia commented Oct 15, 2019 •

edited

Loading

tecknixia commented Oct 15, 2019 •

edited

Loading

tecknixia commented Oct 16, 2019 •

edited

Loading

stevphie commented Oct 17, 2019 •

edited

Loading