Conversation
JohannesLorenz
left a comment
JohannesLorenz
left a comment
There was a problem hiding this comment.
Checked:
- Style
- Functionality
Open points:
- Testing
- Checking if the feature is wanted
|
Ah yes, I apologize for that. I changed it on my PC when I realized the errors, but forgot to push. :/ |
|
Seems to be a great idea, but what about themes? Maybe that's the least of all problems, since it is just an extra button, but, who knows? |
Sounds great, I would love my LMMS in purple, but if you want to talk about that, maybe open up a discussion on the forums or an “enhancement” tag issue on Github. |
Can you clarify what you are concerned about? What scenario are you concerned about specifically? |
|
This shouldn't affect custom themes. The icon is unchanged, and it is just added to the edit mode button group, so if that's themed, the knife button will also be themed. |
|
See also my combo button idea in #6497. If you don't want to copy pasta that, I'm fine with merging this as-is for the moment. |
| QAction* eraseAction = editModeGroup->addAction( embed::getIconPixmap( "edit_erase" ), tr("Erase mode (Shift+E)" ) ); | ||
| QAction* selectAction = editModeGroup->addAction( embed::getIconPixmap( "edit_select" ), tr( "Select mode (Shift+S)" ) ); | ||
| QAction* pitchBendAction = editModeGroup->addAction( embed::getIconPixmap( "automation" ), tr("Pitch Bend mode (Shift+T)" ) ); | ||
| QAction* knifeAction = editModeGroup->addAction(embed::getIconPixmap("edit_knife"), tr("Knife")); |
There was a problem hiding this comment.
The others have written out the shortcut explicitly in their names, it might be necessary for these kinds of buttons, I haven't actually tested the code.
There was a problem hiding this comment.
I didn't change that bit of code ; The tr("Knife") is exactly the same as it was originally.
I could modify it if that would be best, but I'm not sure whether people think that would overstep the scope set by the issue...
|
Sorry for spamming, just wanted to say this could perhaps be added to #7953 or the other way around |
|
I'd be willing to add into the other PR ; See my message there. |
6 participants
This pull request attempts to resolve #7814 by moving the knife tool to the piano roll toolbar, and adding a hint for its usage.
The changes are :
I'm not too pleased with this to be honest, but it works alright and makes other code cleaner.
NOTE : I'm not sure whether it would be a good idea to only show the hint once - If the user misses it, that could be problematic. Therefore, I've left it as displaying every time for now. Maybe a 'Don't show again' button would solve this ? I don't know how I would program that though \(°v°)/