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

Allow individual velocity/pan changes with alt #3923

Merged
merged 1 commit into from
Dec 22, 2017

Conversation

gandalf3
Copy link
Contributor

This commit changes the behavior introduced in 6e3d4f4 to allow using
alt to change the velocity/panning of multiple selected notes by dragging.

Screencapture showing piano roll velocity/pan click+drag behavior with and without Alt held

I believe this was the originally intended behavior, but it never worked for me.

Fixes #3914

@tresf
Copy link
Member

tresf commented Oct 30, 2017

@gandalf3 thanks for the PR! This reads like a bug and should be targeted against stable-1.2 so that it makes it into the stable release.

// Under the cursor, when there is no selection
// Selected, and alt is not pressed
// Under the cursor, selected, and alt is pressed
if ( ( isUnderPosition && !isSelection() ) || \
Copy link
Member

Choose a reason for hiding this comment

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

Cosmetic but the \ before the newline is not needed since this line is not part of a preprocessor statement. It's also inconsistent with the formatting of this file.

This commit changes the behavior introduced in 6e3d4f4 to allow using
alt to change the velocity of multiple *selected* notes by dragging.

I believe this was the originally intended behavior, but it never worked for me.
@gandalf3 gandalf3 changed the base branch from master to stable-1.2 October 31, 2017 03:47
@gandalf3
Copy link
Contributor Author

@tresf Thanks, I've removed the \s and switched to stable-1.2

if ( ( isUnderPosition && !isSelection() ) ||
( n->selected() && !altPressed ) ||
( isUnderPosition && n->selected() && altPressed )
)
Copy link
Member

Choose a reason for hiding this comment

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

On Linux <Alt> + <Left Mouse Button> by default moves the whole application window. I think this is true on all desktops. This is nothing you've missed here but I had to change
bool altPressed = me->modifiers() & Qt::AltModifier; to
bool altPressed = me->modifiers() & Qt::ControlModifier;
in order to test this. It works as intended on Linux with this change applied. My suggestion is to simply change <Alt> for <Ctrl> . Two issues fixed.

@gandalf3
Copy link
Contributor Author

@zonkmachine Isn't Ctrl drag used for selection? When Ctrl+dragging here I get a box selection.

I use Super in place of Alt for window manipulations, as many other programs (e.g. Blender, Inkscape) use Alt + drag for stuff. I don't see a problem with programs using Alt.

In fact, <rant> I've never met a single program which binds anything to Super by default, which seems to be conventionally reserved for window managers. Why distros bind Alt to the window manager by default (instead of Super), I do not understand. </rant>

Regardless, that's a complaint for distribution maintainers. In my opinion, the solution is to choose shortcuts which make sense without worrying about Alt and permit the user to rebind keys as they see fit. Obviously that's a much bigger task, though I see it already exists: #1475

Anyway, I just wanted to put my two cents in. If I'm mistaken and Ctrl doesn't conflict, I'll happily update the PR.

@Umcaruje
Copy link
Member

I agree with @gandalf3, Alt seems to be the right modifier for this. my distro also doesn't have any shortcuts with Alt, they're all with Super.

@zonkmachine
Copy link
Member

It looks like the Alt + Move Window is a Gnome thing and one that many ask how to disable.

Isn't Ctrl drag used for selection?

It indeed is.

@zonkmachine zonkmachine dismissed their stale review November 13, 2017 00:15

Wrong. Review dismissed.

@zonkmachine
Copy link
Member

Merge?

@zonkmachine zonkmachine added this to the 1.2.0 milestone Dec 22, 2017
@tresf
Copy link
Member

tresf commented Dec 22, 2017

Merge?

Yes. I can confirm that on Ubuntu 14.04 (and likely most other Linux desktops), the ALT key will drag the window, but a valid workaround is to simply add the SHIFT modifier and this feature works as expected. So, hold SHIFT + ALT, then keep holding and click.

untitled

Side note... there's a bit of order-of-operation issues with modifiers and mouse clicks, but those are existing throughout most of LMMS. i.e. If you click first and then add the modifier afterward, the behavior is different. Fixing these other UX issues should be discussed separately but wanted to mention this incase someone testing this gets hung up. :)

@zonkmachine
Copy link
Member

There are supposedly ways to turn this feature off but so far it hasn't worked for me. It's an upstream anyway. I think this should be merged.

@tresf tresf merged commit 2c3df22 into LMMS:stable-1.2 Dec 22, 2017
@zonkmachine
Copy link
Member

<Alt>+<Left click> issue (drag window) on some Linux versions discussed here: #1810

@gandalf3 gandalf3 deleted the selective-velocity-editing branch January 31, 2018 05:51
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Fix regression caused by 6e3d4f4, allow ALT to change the velocity of multiple selected notes.
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.

4 participants