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

Separate keyboard enablement from read-only editor state #17124

Merged
merged 8 commits into from Apr 13, 2018

Conversation

Projects
None yet
3 participants
@smashwilson
Member

smashwilson commented Apr 12, 2018

Add an enableKeyboardInput() configuration to TextEditor that allows a package to explicitly take control of the editor's input without making the editor read-only. I'm also changing TextEditorComponent.setInputEnabled() to modify the editor's keyboard enablement rather than read-onliness to restore the behavior of calling it directly.

Fixes #17121.

smashwilson added some commits Apr 12, 2018

@daviwil

Looks great!

@t9md t9md referenced this pull request Apr 12, 2018

Closed

idea to fix #1058 #1060

@t9md

This comment has been minimized.

Contributor

t9md commented Apr 12, 2018

Doesn't work, I'm now investigating this.
At least this part should be removed.
https://github.com/atom/atom/blob/master/src/text-editor-component.js#L480-L482

But after that, I still cant' make vim-mode-plus work in master.
Why don't you simply manage state at component level like I did in closed #1060 which seem to much simpler and explicit for me.

Will update after I found the cause.

@t9md

This comment has been minimized.

Contributor

t9md commented Apr 12, 2018

I need to go to bed now.
Still couldn't make it work.
And also noticed at least user need to invoke atom by --clear-wind-sate since old setInputEnabled have make editor readonly and that prop is restored from editorElement's state(which is not we want this time).

Again I want make ignoreTextInput explicitly separate, mean no shared-logic with read-only-ness.
It make us confuse when following code and changing code.
I bet IgnoreTextInput or setInputEnabled is used only by vim-mode-plus, why this has to be so general?

I want this stupidly simple so that no confusion, no breaking in future read-only behavior modification.

@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 12, 2018

No, it doesn't work - I hadn't actually had a chance to try it out myself yet. I'm working on it presently.

smashwilson added some commits Apr 12, 2018

@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 12, 2018

Okay, I have vim-mode-plus' tests on master passing now, and editors behaving correctly as I toggle .enableInputEnablement() and .setReadOnly() in the dev console. I think all I need to do now is address the TextEditor serialization issue.

@daviwil

This comment has been minimized.

Member

daviwil commented Apr 12, 2018

👍

@smashwilson

This comment has been minimized.

Member

smashwilson commented Apr 12, 2018

Okay, I've bumped the serialization key for readOnly to readOnly2. Any editors previously serialized with TextEditorComponent.setInputEnabled(false) will now be deserialized as read-write, not read-only.

There is a risk of us restoring an editor that used to be read-only as read-write now, but I feel like that's better than breaking everyone's serialized vim-mode editors, especially because readOnly is a relatively recent addition.

@t9md

This comment has been minimized.

Contributor

t9md commented Apr 13, 2018

Thanks @smashwilson , me too confirmed that vim-mode-plus worked properly with this PR.
Thanks!!

@smashwilson smashwilson merged commit 9c19b83 into master Apr 13, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the aw/keyboard-enablement branch Apr 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment