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 renaming of FX mixer channels with the F2 and enter keys. #4348

Merged
merged 4 commits into from
May 13, 2018

Conversation

SecondFlight
Copy link
Member

Will resolve #3157. Currently the enter key isn't being hit, and I have no idea why.

case Qt::Key_F2:
	renameChannel( m_currentFxLine->channelIndex() );
case Qt::Key_Enter:
	renameChannel( m_currentFxLine->channelIndex() );

I don't like the code duplication here, but I don't believe there's a way to capture multiple cases in one case statement. It could be reasonable to change to an if/else block just to avoid that.

@Wallacoloo
Copy link
Member

C++ case statements are "fall through", meaning that once you jump into the body of a case, the program runs sequentially until you reach the end brace of the switch block. break will exit the switch block. So you can do something like this:

switch(e->key()) {
case Qt::Key_F2:
        // does nothing; falls through to the case below.
case Qt::Key_Enter:
	renameChannel( m_currentFxLine->channelIndex() );
        break; // exit the switch statement so as not to run into the cases below.
// [more cases below]
}

This is a common enough idiom that you'd usually write it without the comments.

@SecondFlight
Copy link
Member Author

Alright, I'll be sure to use that. Figured there must be a way.

@SecondFlight
Copy link
Member Author

Now there's an issue where pressing enter to confirm the name doesn't work at all. I'm guessing this is because pressing enter causes the rename to trigger again. I'll look at this more tomorrow.

@@ -100,6 +100,9 @@ class EXPORT FxMixerView : public QWidget, public ModelView,
void moveChannelLeft(int index, int focusIndex);
void moveChannelRight(int index);

// rename the channel
Copy link
Member

Choose a reason for hiding this comment

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

This comment is useless and should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I see what you're saying. I was following the rest of the code there, it looks more out of place without the comment IMO. I could remove all these comments as they are all pretty redundant, but for now I'll just remove the one I added.

image

@SecondFlight SecondFlight changed the title [WIP] Allow renaming of FX mixer channels with the F2 and enter keys. Allow renaming of FX mixer channels with the F2 and enter keys. May 10, 2018
@SecondFlight
Copy link
Member Author

I've run into a snag here, and I'd like to reach out for suggestions before I go off and subclass QLineEdit.


The short version

Pressing enter after you've finished making your changes to the channel name does save your changes, but the event is not killed and it propagates after the edit is completed. My code that handles the enter key is called, causing it to go right back into edit mode.


The longer version

editingFinished() on the QLineEdit for the channel name is connected to renameFinished() in FxLine.cpp via this line:

connect( m_renameLineEdit, SIGNAL( editingFinished() ), this, SLOT( renameFinished() ) );

editingFinished() is emitted when the Return or Enter key is pressed or the line edit loses focus. (source from docs)

Based on this forum post, along with other sources, it would seem that editingFinished() does not call ->accept() on the event, meaning that the event will still propagate through and trigger other event listeners further up the chain. This means that somewhere along the line, keyPressEvent in FxMixerView.cpp gets called after the edit is finished with return as the pressed key, which causes it to go right back into edit mode.

I could be mistaken about something, but assuming all this is correct, it would seem that the only way to fix it is to subclass from QLineEdit to change the behavior. This is such a simple change though, and it really feels like there should be a simpler way to do it.

Or I could use a global variable, but that seems like a bad idea 😝

@Wallacoloo
Copy link
Member

@SecondFlight Are you familiar with event filters (scroll down half way)? I'm not sure they do what you want - I haven't really studied Qt much - but maybe they'd provide a way around your problem?

@SecondFlight
Copy link
Member Author

@Wallacoloo I tried using those, but I think I may have been using them wrong. I have an idea to try with them that I think might work.

@SecondFlight
Copy link
Member Author

Yep, I was blocking the event, but I wasn't actually handling it. This is definitely the right way to do it.

@SecondFlight SecondFlight merged commit 4585a07 into LMMS:master May 13, 2018
@SecondFlight SecondFlight deleted the fx-mixer-rename-shortcut branch May 13, 2018 21:16
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
…#4348)

* Add f2 as a FX mixer rename shortcut. Enter doesn't work yet.

* Add both enter keys, remove code duplication

* Fix renaming with enter/return

* Clean up
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