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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `readonly` attribute to text editor element #16294

Merged
merged 6 commits into from Dec 6, 2017

Conversation

Projects
None yet
5 participants
@kuychaco
Member

kuychaco commented Nov 29, 2017

This PR allows for the use of a readonly attribute which disables the input for text editor elements:

<atom-text-editor readonly />

The read-only state is stored on the TextEditor model. As of now this state merely prevents text from being input into the editor. Future considerations include using the state to determine modified status, preventing saving, etc.

/cc @maxbrunsfeld @damieng for 馃挱s

damieng and others added some commits Dec 4, 2017

Ensure read-only text editors are not considered 'modified'. Also cle鈥
鈥r read-only flag on successful save.

@kuychaco kuychaco requested a review from maxbrunsfeld Dec 5, 2017

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Dec 5, 2017

Contributor

I might have missed some discussion here - why does saving the buffer clear the readonly status?

Other than that question, this looks great to me.

Contributor

maxbrunsfeld commented Dec 5, 2017

I might have missed some discussion here - why does saving the buffer clear the readonly status?

Other than that question, this looks great to me.

@kuychaco kuychaco merged commit bffb3bf into master Dec 6, 2017

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

@kuychaco kuychaco deleted the ku-add-readonly-editor-attribute branch Dec 6, 2017

@ykpythemind ykpythemind referenced this pull request Dec 13, 2017

Closed

vim mode cursor disappears in Atom 1.24-beta0 #998

4 of 7 tasks complete
@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 13, 2017

Contributor

I might have missed some discussion here - why does saving the buffer clear the readonly status?

Other than that question, this looks great to me.

I want to ask this too.
I'm maintainor of vim-mode-plus.
In vim-mode-plus,

  • normal-mode is achieved by calling this.editor.element.component.setInputEnabled(false).
    • So editor.component.isInputEnalbed() returns false
    • But once I saved buffer by cmd-s, editor.component.isInputEnalbed() returns true

This fundamental editor.component's capability( input enabled or not ) should not be affected by irrelevant action like cmd-s.

Contributor

t9md commented Dec 13, 2017

I might have missed some discussion here - why does saving the buffer clear the readonly status?

Other than that question, this looks great to me.

I want to ask this too.
I'm maintainor of vim-mode-plus.
In vim-mode-plus,

  • normal-mode is achieved by calling this.editor.element.component.setInputEnabled(false).
    • So editor.component.isInputEnalbed() returns false
    • But once I saved buffer by cmd-s, editor.component.isInputEnalbed() returns true

This fundamental editor.component's capability( input enabled or not ) should not be affected by irrelevant action like cmd-s.

@@ -819,7 +823,7 @@ class TextEditorComponent {
const oldClassList = this.classList
const newClassList = ['editor']
if (this.focused) newClassList.push('is-focused')
if (this.focused && this.isInputEnabled()) newClassList.push('is-focused')
if (model.isMini()) newClassList.push('mini')

This comment has been minimized.

@t9md

t9md Dec 13, 2017

Contributor

Why this code assume is-focused only need to set only when isInputEnabled() was true?

@t9md

t9md Dec 13, 2017

Contributor

Why this code assume is-focused only need to set only when isInputEnabled() was true?

This comment has been minimized.

@t9md

t9md Dec 14, 2017

Contributor

I need this unchanged, don't change meaning of is-focused.
Should allow readonly and is-focused buffer.
If some feature need exclude inputDisabled buffer from focused editor, do this by client side by checking state manually, don' bend core-meaning just because it's convenient.

@t9md

t9md Dec 14, 2017

Contributor

I need this unchanged, don't change meaning of is-focused.
Should allow readonly and is-focused buffer.
If some feature need exclude inputDisabled buffer from focused editor, do this by client side by checking state manually, don' bend core-meaning just because it's convenient.

@@ -1106,7 +1129,7 @@ class TextEditor {
setEncoding (encoding) { this.buffer.setEncoding(encoding) }
// Essential: Returns {Boolean} `true` if this editor has been modified.
isModified () { return this.buffer.isModified() }
isModified () { return this.isReadOnly() ? false : this.buffer.isModified() }

This comment has been minimized.

@t9md

t9md Dec 13, 2017

Contributor

I don't want this unnecessary guard. If readonly really disallow buffer modification, just returning buffer.isModified() should be enough.
This method should be just proxy of this.buffer.isModified() for clarity, and remove possibility of inconsistency when it become this.isReadonly !== this.buffer.isModifed().
Also this breaks https://github.com/t9md/atom-narrow pkg's important feature.

@t9md

t9md Dec 13, 2017

Contributor

I don't want this unnecessary guard. If readonly really disallow buffer modification, just returning buffer.isModified() should be enough.
This method should be just proxy of this.buffer.isModified() for clarity, and remove possibility of inconsistency when it become this.isReadonly !== this.buffer.isModifed().
Also this breaks https://github.com/t9md/atom-narrow pkg's important feature.

This comment has been minimized.

@damieng

damieng Dec 13, 2017

Contributor

This is necessary otherwise it is not possible to create a read-only text editor from some in-memory data without it showing as modified on the tab.

@damieng

damieng Dec 13, 2017

Contributor

This is necessary otherwise it is not possible to create a read-only text editor from some in-memory data without it showing as modified on the tab.

This comment has been minimized.

@t9md

t9md Dec 13, 2017

Contributor

then, I want to say hack this.buffer.isModified by overriding buffer.isModified = () => true.
I feel some anxiety when seeing this fundamental return different answer based on isReadOnly status, and that status is unstable(changed by just saving buffer).
I can't believe such method, so just trying to relay on buffer.isModified directly.

@t9md

t9md Dec 13, 2017

Contributor

then, I want to say hack this.buffer.isModified by overriding buffer.isModified = () => true.
I feel some anxiety when seeing this fundamental return different answer based on isReadOnly status, and that status is unstable(changed by just saving buffer).
I can't believe such method, so just trying to relay on buffer.isModified directly.

This comment has been minimized.

@t9md

t9md Dec 14, 2017

Contributor

Oh, I wanted to say use buffer.isModified = () => false to hide modified icon on tab.

@t9md

t9md Dec 14, 2017

Contributor

Oh, I wanted to say use buffer.isModified = () => false to hide modified icon on tab.

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Dec 13, 2017

Contributor

One of our scenarios for use of a read-only text editor is when we want to show you and editor but you can't edit the content because of where the text comes from - e.g. source that lives on a https:// url or inside a .jar file. If however they chose to save that content to disk then by the very nature it is now editable as it lives somewhere the content can be changed.

Contributor

damieng commented Dec 13, 2017

One of our scenarios for use of a read-only text editor is when we want to show you and editor but you can't edit the content because of where the text comes from - e.g. source that lives on a https:// url or inside a .jar file. If however they chose to save that content to disk then by the very nature it is now editable as it lives somewhere the content can be changed.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 13, 2017

Contributor

@damieng I'm totally OK of that use case, but that feature is achievable without breaking existing setInputEnabled and isReadonly function.

Once vim-mode was maintained by github team.
But I forked and released it because of it's inactivity.
The component.setInputEnabled feature have been used from very early stage of vim-mode.
When I saw this PR and your response, I feel github team have no interest how this capability is used in other pkgs, that make me really reluctant to be active user of Atom and Atom pkgs.

Contributor

t9md commented Dec 13, 2017

@damieng I'm totally OK of that use case, but that feature is achievable without breaking existing setInputEnabled and isReadonly function.

Once vim-mode was maintained by github team.
But I forked and released it because of it's inactivity.
The component.setInputEnabled feature have been used from very early stage of vim-mode.
When I saw this PR and your response, I feel github team have no interest how this capability is used in other pkgs, that make me really reluctant to be active user of Atom and Atom pkgs.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 13, 2017

Contributor

Sorry, I was really nervous for now, I know I should not write any comment with such mental state.
I won't respond for a while.

Contributor

t9md commented Dec 13, 2017

Sorry, I was really nervous for now, I know I should not write any comment with such mental state.
I won't respond for a while.

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Dec 13, 2017

Contributor

No code is perfect :) We did consider whether saving should clear the flag or not after @maxbrunsfeld also brought it up. If it causes you difficulty we can remove that.

Contributor

damieng commented Dec 13, 2017

No code is perfect :) We did consider whether saving should clear the flag or not after @maxbrunsfeld also brought it up. If it causes you difficulty we can remove that.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Dec 13, 2017

Contributor

We should remove it. We need to not break vim-mode-plus, and they are orthogonal concepts anyway.

Same thing with isModified. We can address the issue of opening files from .jars in another way that doesn鈥檛 bake assumptions into core.

Contributor

maxbrunsfeld commented Dec 13, 2017

We should remove it. We need to not break vim-mode-plus, and they are orthogonal concepts anyway.

Same thing with isModified. We can address the issue of opening files from .jars in another way that doesn鈥檛 bake assumptions into core.

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Dec 13, 2017

Contributor

Removed the isModified/clear-on-save code ca53cf9

Contributor

damieng commented Dec 13, 2017

Removed the isModified/clear-on-save code ca53cf9

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 14, 2017

Contributor

I need is-focused breaking reverted too.
This is direct breaking where style based on is-focused scope become not working.
Readonly state is completely irrelevant to focused state.
Should not be coupled that way.

Contributor

t9md commented Dec 14, 2017

I need is-focused breaking reverted too.
This is direct breaking where style based on is-focused scope become not working.
Readonly state is completely irrelevant to focused state.
Should not be coupled that way.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 14, 2017

Contributor

Basically want revert whole this PR and rethink to achieve it without breaking existing setInputEnabled capability.
I鈥檓 not insisting to not break existing feature in every case of new enhancements, but I say should not break existing feature this time, since readonly feature should not be coupled/blended to these feature.
This is very fundamental feature vim-mode-plus depends on.
And original vim-mode was created by GitHub.
Please communicate each other if your team conside vmp as part of Atom鈥檚 goodness(I want say so).

Contributor

t9md commented Dec 14, 2017

Basically want revert whole this PR and rethink to achieve it without breaking existing setInputEnabled capability.
I鈥檓 not insisting to not break existing feature in every case of new enhancements, but I say should not break existing feature this time, since readonly feature should not be coupled/blended to these feature.
This is very fundamental feature vim-mode-plus depends on.
And original vim-mode was created by GitHub.
Please communicate each other if your team conside vmp as part of Atom鈥檚 goodness(I want say so).

if (this.component != null) {
this.component.scheduleUpdate()
}
this.buffer.emitModifiedStatusChanged(this.isModified())

This comment has been minimized.

@t9md

t9md Dec 14, 2017

Contributor

Should not emit "modified state changed" event when it's really not changed.
If some feature need hook for this, fire distinct event like did-set-read-only and do manual work in this.

@t9md

t9md Dec 14, 2017

Contributor

Should not emit "modified state changed" event when it's really not changed.
If some feature need hook for this, fire distinct event like did-set-read-only and do manual work in this.

This comment has been minimized.

@t9md

t9md Dec 14, 2017

Contributor

I noticed this part is reverted in ca53cf9. sorry.

@t9md

t9md Dec 14, 2017

Contributor

I noticed this part is reverted in ca53cf9. sorry.

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Dec 14, 2017

Contributor
Contributor

damieng commented Dec 14, 2017

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 14, 2017

Contributor

@damieng

it's needed elsewhere.

I want to know where.

Contributor

t9md commented Dec 14, 2017

@damieng

it's needed elsewhere.

I want to know where.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 14, 2017

Contributor

@damieng @maxbrunsfeld

Confirmed with current master that I can maintain vmp if is-focused meaning is reverted to original.

diff --git a/src/text-editor-component.js b/src/text-editor-component.js
index 08af5ada1..263556ee0 100644
--- a/src/text-editor-component.js
+++ b/src/text-editor-component.js
@@ -823,7 +823,7 @@ class TextEditorComponent {

     const oldClassList = this.classList
     const newClassList = ['editor']
-    if (this.focused && this.isInputEnabled()) newClassList.push('is-focused')
+    if (this.focused) newClassList.push('is-focused')
     if (model.isMini()) newClassList.push('mini')
     for (var i = 0; i < model.selections.length; i++) {
       if (!model.selections[i].isEmpty()) {

I think this request is reasonable, in vmp's normal-mode, it's readonly as long as it's focused.
The is-focused and readonly states should not be mutually exclusive.

Contributor

t9md commented Dec 14, 2017

@damieng @maxbrunsfeld

Confirmed with current master that I can maintain vmp if is-focused meaning is reverted to original.

diff --git a/src/text-editor-component.js b/src/text-editor-component.js
index 08af5ada1..263556ee0 100644
--- a/src/text-editor-component.js
+++ b/src/text-editor-component.js
@@ -823,7 +823,7 @@ class TextEditorComponent {

     const oldClassList = this.classList
     const newClassList = ['editor']
-    if (this.focused && this.isInputEnabled()) newClassList.push('is-focused')
+    if (this.focused) newClassList.push('is-focused')
     if (model.isMini()) newClassList.push('mini')
     for (var i = 0; i < model.selections.length; i++) {
       if (!model.selections[i].isEmpty()) {

I think this request is reasonable, in vmp's normal-mode, it's readonly as long as it's focused.
The is-focused and readonly states should not be mutually exclusive.

@kuychaco

This comment has been minimized.

Show comment
Hide comment
@kuychaco

kuychaco Dec 14, 2017

Member

@t9md the change has been reverted in 853b9a4. We'll get beta hotfixed later today.

Member

kuychaco commented Dec 14, 2017

@t9md the change has been reverted in 853b9a4. We'll get beta hotfixed later today.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 14, 2017

Contributor

@kuychaco Thank you for hearing.

Contributor

t9md commented Dec 14, 2017

@kuychaco Thank you for hearing.

@fabianfiorotto

This comment has been minimized.

Show comment
Hide comment
@fabianfiorotto

fabianfiorotto Jan 2, 2018

Readonly editors can be modified using middle click paste on Linux. I tested it on Linux Mint 17.3 with atom 1.24.0-beta1

fabianfiorotto commented Jan 2, 2018

Readonly editors can be modified using middle click paste on Linux. I tested it on Linux Mint 17.3 with atom 1.24.0-beta1

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Jan 2, 2018

Contributor

@fabianfiorotto Nice find, that should be raised as a separate issue - comments on PR's can't be tracked as work items.

Contributor

damieng commented Jan 2, 2018

@fabianfiorotto Nice find, that should be raised as a separate issue - comments on PR's can't be tracked as work items.

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