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

Cancel track rename by pressing Escape key #3676

Merged
merged 1 commit into from Jul 14, 2017
Merged

Conversation

@M374LX
Copy link
Contributor

@M374LX M374LX commented Jul 1, 2017

With this pull request, the user is able to cancel a track rename action by pressing the Escape key.

Also renamed rename_dlg to renameDlg in TrackLabelButton::rename() so it complies to the naming conventions.

Fixes #3675.

/*
* TrackRenameLineEdit.h - class TrackRenameLineEdit
*
* Copyright (c) 2004-2008 Tobias Doerffel <tobydox/at/users.sourceforge.net>

This comment has been minimized.

@Umcaruje

Umcaruje Jul 1, 2017
Member

You should put your name here

This comment has been minimized.

@M374LX

M374LX Jul 1, 2017
Author Contributor

* represents the text field that appears when one
* double-clicks a track's label to rename it
*
* Copyright (c) 2004-2008 Tobias Doerffel <tobydox/at/users.sourceforge.net>

This comment has been minimized.

@Umcaruje

Umcaruje Jul 1, 2017
Member

Same here, your name

This comment has been minimized.

@M374LX

M374LX Jul 1, 2017
Author Contributor

Same as above.

@tresf
Copy link
Member

@tresf tresf commented Jul 1, 2017

stable-1.2 is on a feature freeze, right?

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jul 2, 2017

stable-1.2 is on a feature freeze, right?

@M374LX As @tresf said, stable-1.2 is on a feature freeze. You shouldn't introduce new feature into stable-1.2 unless it is related to a bug fix.
See #3662 (comment).

If many people agrees 1.2.0 needs this, however, it would be fine.

@@ -2,6 +2,7 @@
* TrackRenameLineEdit.h - class TrackRenameLineEdit
*
* Copyright (c) 2004-2008 Tobias Doerffel <tobydox/at/users.sourceforge.net>
* Copyright (c) 2007 Alexandre Almeida <http://m374lx.users.sourceforge.net/>

This comment has been minimized.

@Umcaruje

Umcaruje Jul 2, 2017
Member

You mistyped the date, it's 2017 now 😉

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jul 2, 2017

stable-1.2 is on a feature freeze, right?
You shouldn't introduce new feature into stable-1.2 unless it is related to a bug fix.

Well, I just checked and when renaming in LMMS 1.1.3 the escape key would close the rename dialog. To that extent, this can be considered a bug fix cause it brings back previous behaviour that was lost with the new feature. I vote this gets into stable-1.2

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jul 2, 2017

Well, I just checked and when renaming in LMMS 1.1.3 the escape key would close the rename dialog.

If it was in previous version of LMMS, I think it could a bug fix. I agree with @Umcaruje.

@M374LX
Copy link
Contributor Author

@M374LX M374LX commented Jul 2, 2017

Also note that the rename dialog still exists in 1.2.0-rc3 when using compact track buttons and the Escape key also cancels it.




TrackRenameLineEdit::TrackRenameLineEdit( QWidget * _parent ) :

This comment has been minimized.

@Umcaruje

Umcaruje Jul 2, 2017
Member

Per the coding conventions: Function parameters are not prefixed with "_" anymore

So this should be parent




void TrackRenameLineEdit::keyPressEvent( QKeyEvent * _ke )

This comment has been minimized.

@Umcaruje

Umcaruje Jul 2, 2017
Member

Same as the previous comment: this should be ke

{
Q_OBJECT
public:
TrackRenameLineEdit( QWidget * _parent );

This comment has been minimized.

@Umcaruje

Umcaruje Jul 2, 2017
Member

These should be updated as well.

Copy link
Member

@Umcaruje Umcaruje left a comment

Tested this out, looks like a charm, and the code looks good as well. Green light to merge from me 👍


void TrackRenameLineEdit::keyPressEvent( QKeyEvent * ke )
{
if( ke->key() == Qt::Key_Escape ) {

This comment has been minimized.

@BaraMGB

BaraMGB Jul 2, 2017
Contributor

The brace should be in the next line

@tresf
Copy link
Member

@tresf tresf commented Jul 3, 2017

in LMMS 1.1.3 the escape key would close the rename dialog. To that extent, this can be considered a bug fix

Valid point.

Copy link
Member

@zonkmachine zonkmachine left a comment

Tests fine and looks good.

PS. When this is merged, please edit the commit message.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jul 7, 2017

PS. When this is merged, please edit the commit message.

What do you mean by this?

@tresf
Copy link
Member

@tresf tresf commented Jul 7, 2017

PS. When this is merged, please edit the commit message.

What do you mean by this?

When a PR is squashed and merged it leaves a bunch of unnecessary comments in the history. This one will read:

* Create TrackRenameLineEdit.cpp

* Update TrackLabelButton.cpp

* Update TrackLabelButton.h

* Create TrackRenameLineEdit.h

* Update CMakeLists.txt

* Update TrackRenameLineEdit.cpp

* Update TrackRenameLineEdit.cpp

* Update TrackLabelButton.h

* Update TrackLabelButton.h

* Update TrackRenameLineEdit.h

* Update TrackRenameLineEdit.h

* Update TrackRenameLineEdit.cpp

* Update TrackRenameLineEdit.cpp

* Update TrackRenameLineEdit.h

* Update TrackRenameLineEdit.cpp

He's asking that the person that merges it doesn't leave the default text in there and instead cleans up the commit message on merge. This should be standard action on the merger's part but sometimes we forget to do it due to the small preview window.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jul 7, 2017

Oh, thanks for explaining @tresf

@M374LX M374LX force-pushed the M374LX:stable-1.2 branch from 39018b2 to aa6d528 Jul 8, 2017
@tresf tresf merged commit 3dfda61 into LMMS:stable-1.2 Jul 14, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Sawuare
Copy link
Member

@Sawuare Sawuare commented Jul 15, 2017

Instrument plug-ins are the only tracks that can be re-named from their own window, will this pull request enable the user to cancel the re-name action from the window by pressing the Esc key?
I mean from here:
image

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Jul 15, 2017

@Hussam-Eddin-Alhomsi No, change from plug-in windows are applied immediately, so can't be canceled by Esc key.

@Sawuare
Copy link
Member

@Sawuare Sawuare commented Jul 15, 2017

@PhysSong You're right. Didn't think about that.

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

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