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 Automation Point delete radius and size #3902

Merged
merged 2 commits into from Oct 24, 2017

Conversation

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Oct 22, 2017

Exchange defective algorithm for Automatin Point delete radius with a new function adapted from AutomationEditor::drawLine(),
Increase minimal Automation Point radius by 50%.

The sensitivity for grabbing/moving an Automation Point is still a bit off on the fines quantization levels.

Fixes #3896

@zonkmachine zonkmachine requested a review from Umcaruje Oct 22, 2017
@zonkmachine zonkmachine force-pushed the automationpointissues branch 2 times, most recently from 5d8cc8d to 6a20f29 Oct 22, 2017
@zonkmachine
Copy link
Member Author

@zonkmachine zonkmachine commented Oct 22, 2017

I found a much better way by adapting AutomationEditor::drawLine().

Loading

@@ -507,6 +507,7 @@ void AutomationEditor::mousePressEvent( QMouseEvent* mouseEvent )
// get tick in which the user clicked
int pos_ticks = x * MidiTime::ticksPerTact() / m_ppt +
m_currentPosition;
m_drawLastTick = pos_ticks;
Copy link
Member Author

@zonkmachine zonkmachine Oct 22, 2017

Choose a reason for hiding this comment

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

I just, cunningly, shot the line draw function to pieces with this line. 8P

Loading

Copy link
Member Author

@zonkmachine zonkmachine Oct 22, 2017

Choose a reason for hiding this comment

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

Fixed.

Loading

@zonkmachine zonkmachine force-pushed the automationpointissues branch from 6a20f29 to 6c98633 Oct 22, 2017
}
removePoints( m_drawLastTick, pos_ticks );
Engine::getSong()->setModified();
Copy link
Member

@PhysSong PhysSong Oct 22, 2017

Choose a reason for hiding this comment

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

It will mark the project as modified if no points are removed. It should be moved into removePoints().

Loading

Copy link
Member Author

@zonkmachine zonkmachine Oct 22, 2017

Choose a reason for hiding this comment

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

I remove points without checking if there are any there. So if I add bool pointsRemoved = false; and lets the delete loop set it to true, it is still going to set the project to changed as soon as the mouse moves just a bit without really removing anything.

Loading

Copy link
Member Author

@zonkmachine zonkmachine Oct 22, 2017

Choose a reason for hiding this comment

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

The behaviour right now is, deleting points with right click sets the project to changed but deleting by dragging over points doesn't.

Loading

Copy link
Member Author

@zonkmachine zonkmachine Oct 23, 2017

Choose a reason for hiding this comment

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

@PhysSong I don't know quite how to move Engine::getSong()->setModified(); to removePoints(). This is already an improvement from before when setModified() wasn't called at all. I suggest merging this 'as is', or I can have another look at it if someone can point me in the right direction but for now I think it's low priority.

Loading

Copy link
Member

@PhysSong PhysSong Oct 23, 2017

Choose a reason for hiding this comment

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

Engine::getSong()->setModified(); is an UX problem. So I think it's out of scope a little bit. Since the point of this PR is on GUI part, merging as-is looks fine. Dropping that will be fine, too.
I can create a PR which fixes the UX issue later. I think it will be the best way. 👍

Loading

Copy link
Member Author

@zonkmachine zonkmachine Oct 23, 2017

Choose a reason for hiding this comment

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

All right. I'll merge this tomorrow.

Loading

@zonkmachine zonkmachine force-pushed the automationpointissues branch from 5dfd6a3 to f8d4a8b Oct 23, 2017
return;
}

if( x0 < x1)
Copy link
Member

@PhysSong PhysSong Oct 23, 2017

Choose a reason for hiding this comment

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

This looks somewhat odd. Space after (, but no space before ).

Loading

Copy link
Member Author

@zonkmachine zonkmachine Oct 23, 2017

Choose a reason for hiding this comment

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

Fixed.

Loading

@zonkmachine zonkmachine force-pushed the automationpointissues branch from f8d4a8b to 61d8dd4 Oct 23, 2017
@zonkmachine zonkmachine merged commit f9ab2de into LMMS:stable-1.2 Oct 24, 2017
1 check passed
Loading
@zonkmachine zonkmachine deleted the automationpointissues branch Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants