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

Right Mouse Button To Erase Nodes In Automation Editor Cursor Icon... #3414

Merged
merged 4 commits into from Mar 13, 2017

Conversation

@uro5h
Copy link
Contributor

@uro5h uro5h commented Mar 7, 2017

Right Mouse Button To Erase Nodes In Automation Editor Cursor Icon Does Not Change #3310

@uro5h uro5h force-pushed the uro5h:master branch from 2342249 to 0ca637a Mar 7, 2017
@uro5h uro5h changed the title #3310 Right Mouse Button To Erase Nodes In Automation Editor Cursor Icon... Right Mouse Button To Erase Nodes In Automation Editor Cursor Icon... Mar 7, 2017
@@ -240,6 +240,8 @@ protected slots:

EditModes m_editMode;

bool m_mouseDownLeft; //true if left click is being held down

This comment has been minimized.

@Umcaruje

Umcaruje Mar 8, 2017
Member

Since you end up only using m_mouseDownRight you should remove the m_mouseDownLeft as it's not used anywhere.

This comment has been minimized.

@uro5h

uro5h Mar 9, 2017
Author Contributor

Agreed, will remove this one. Also, suggested change(s) might be applicable to PianoRoll as it is never used also.

m_mouseDownLeft = false;

(Only line where it is used)

{
cursor = s_toolErase;
}
else

This comment has been minimized.

@Umcaruje

Umcaruje Mar 8, 2017
Member

You could also add a change for the cursor when automation points are moved, like in the piano roll:

case ModeDraw:
if( m_mouseDownRight )
{
cursor = s_toolErase;
}
else if( m_action == ActionMoveNote )
{
cursor = s_toolMove;
}
else
{
cursor = s_toolDraw;
}
break;
case ModeErase: cursor = s_toolErase; break;
case ModeSelect: cursor = s_toolSelect; break;
case ModeEditDetuning: cursor = s_toolOpen; break;

So we keep consitency.

This comment has been minimized.

@uro5h

uro5h Mar 9, 2017
Author Contributor

👍

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Mar 8, 2017

Tested out the PR, works nice, but some changes are needed.

P.S. Lepo je videti da postoje ljudi u Beogradu koji se zanimaju za LMMS :)
(English translation: Nice to see that there are people in Belgrade that take interest in LMMS)

@uro5h
Copy link
Contributor Author

@uro5h uro5h commented Mar 9, 2017

Hvala (Thanks), @Umcaruje .

🍻

@uro5h
Copy link
Contributor Author

@uro5h uro5h commented Mar 9, 2017

@Umcaruje,

I have made suggested changes, also removed unused member from PianoRoll class. Do you require squashed pull requests?

Also there is an issue with triggering MousePressEvent, then MouseReleaseEvent on automation point (without MouseMoveEvent), which doesn't delete the point, but I guess I can file a new issue for it, as I believe (based on a quick look) it is related to state handling (m_action) instead of cursor switching, therefore irrelevant for this pull request. What do you think?

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Mar 9, 2017

Do you require squashed pull requests?

We just squash on merge with Github, so there's no need for you to do it manually.

Also there is an issue with triggering MousePressEvent, then MouseReleaseEvent on automation point (without MouseMoveEvent), which doesn't delete the point, but I guess I can file a new issue for it, as I believe (based on a quick look) it is related to state handling (m_action) instead of cursor switching, therefore irrelevant for this pull request. What do you think?

Yeah, if that issue is outside of the scope of this PR, feel free to open a new issue for it. Better to have several small PR's than one big one :)

I'll test this tommorow, the code LGTM, if all goes well in testing, I'll merge 👍

@Umcaruje Umcaruje merged commit e879fad into LMMS:master Mar 13, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.