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 Esc key to close Cel and Layer Properties windows #1302

Merged
merged 2 commits into from Oct 25, 2016

Conversation

Projects
None yet
2 participants
@klw0
Contributor

klw0 commented Oct 23, 2016

Addresses #964, and does the same for the Layer Properties window.

This mostly follows the key handling style used in menu.cpp.

Show outdated Hide outdated src/app/commands/cmd_cel_properties.cpp
break;
case kKeyEsc:
closeWindow(this);

This comment has been minimized.

@dacap

dacap Oct 24, 2016

Member

I think the kKeyEsc should call onCommitChange() too, because it acts like a shortcut to close the dialog, but not to cancel the changes in the dialog (e.g. we quickly change the layer name and press Esc doesn't mean that we want to discard the layer name change).

I think esc should act exactly like kKeyEnter. So this could be just:

case kKeyEnter:
case kKeyEsc:
   onCommitChange();
   closeWindow(this)...
@dacap

dacap Oct 24, 2016

Member

I think the kKeyEsc should call onCommitChange() too, because it acts like a shortcut to close the dialog, but not to cancel the changes in the dialog (e.g. we quickly change the layer name and press Esc doesn't mean that we want to discard the layer name change).

I think esc should act exactly like kKeyEnter. So this could be just:

case kKeyEnter:
case kKeyEsc:
   onCommitChange();
   closeWindow(this)...

This comment has been minimized.

@klw0

klw0 Oct 24, 2016

Contributor

Sounds good to me, I'll update this tonight.

@klw0

klw0 Oct 24, 2016

Contributor

Sounds good to me, I'll update this tonight.

Show outdated Hide outdated src/app/commands/cmd_layer_properties.cpp
break;
case kKeyEsc:
closeWindow(this);

This comment has been minimized.

@dacap

dacap Oct 24, 2016

Member

Same here.

@dacap

dacap Oct 24, 2016

Member

Same here.

@klw0

This comment has been minimized.

Show comment
Hide comment
@klw0

klw0 Oct 25, 2016

Contributor

@dacap updated.

Contributor

klw0 commented Oct 25, 2016

@dacap updated.

@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Oct 25, 2016

Member

Thanks for your contribution @klw0!

Member

dacap commented Oct 25, 2016

Thanks for your contribution @klw0!

@dacap

dacap approved these changes Oct 25, 2016

@dacap dacap merged commit 674d1d7 into aseprite:master Oct 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment