Community
Participate
Working Groups
Build Identifier: TextView currently has support for changing the tab size - the number of spaces used to render the Tab character. We need an option to expand Tab characters to spaces based on the given tabSize. Reproducible: Always
Just sent a pull request: https://github.com/eclipse/orion.client/pull/4 I have added expandTab option and made it work such that when you press a Tab you get the number of spaces you have configured (tabSize, default 8 spaces). The Backspace key removes the number of tab spaces when you are at a tab stop, so you can press tab, tab, backspace, backspace, naturally, as if real tabs are used. This implements the common and expected behavior of a programmers editor - for example vim. This is something we need for the Mozilla codebase integration. Our previous simple textarea implementation allowed the choice of tabs or expanded tabs, and had configurable tab size. This patch just brings that feature to Orion. Please let me know if further changes are needed. Thank you! And the usual statement: I confirm that I wrote all this code and have the rights to contribute it to Eclipse under the eclipse.org web site terms of use.
To be honest I don't know what a vim user expects when the expand tab option is on. This patch it just replaces newly insert tabs by (the correct number of) whitespaces. The backspace keys, deletes the right number of whitespaces in some cases (when the caret is at tab stop and no letter between caret and line start). Nothing is done for the delete key. Nothing is done for \t that are already in the text buffer (added to the butter by textView#setText) Nothing is done for \t that are inserted in the buffer by clipboard operation (paste). It means that there will still be \t in the view even when expand tab option is on. Mihai, is that the right behavior ?
(In reply to comment #2) > To be honest I don't know what a vim user expects when the expand tab option is > on. > > This patch it just replaces newly insert tabs by (the correct number of) > whitespaces. Yes. > The backspace keys, deletes the right number of whitespaces in some cases (when > the caret is at tab stop and no letter between caret and line start). Yes. > Nothing is done for the delete key. Yes. > Nothing is done for \t that are already in the text buffer (added to the butter > by textView#setText) Correct. It is not desired to expand existing tabs to spaces. Such operations are done only on user requests, like in vim we can ask for "retab". > Nothing is done for \t that are inserted in the buffer by clipboard operation > (paste). Correct. Text from clipboard is taken as-is. > It means that there will still be \t in the view even when expand tab option is > on. Yes. > Mihai, is that the right behavior ? Yes. If others believe further changes will be needed, sure, I am open to stand corrected. However, for the moment, this patch makes expanded tabs work much better than they would otherwise do (in current Orion builds). I would also note that for Mozilla we want expanded tabs on, by default. This is why I submitted this patch, so we can do that. Thank you!
Hi Mihai, Note that I can't release new features to master till the summer release is out. (another week and it will be okay). I'm not sure if textview is the right place for the expand tab features, it would be implemented on top of the textview: /* Expand Tab */ view.setAction("tab", function() { var selection = view.getSelection(); var offset = selection.start; var model = view.getModel(); var lineIndex = model.getLineAtOffset(offset); var lineStart = model.getLineStart(lineIndex); var tabSize = options.tabSize || 8; var spaces = tabSize - ((offset - lineStart) % tabSize); var text = (new Array(spaces + 1)).join(" "); view.setText(text, selection.start, selection.end); return true; }); view.setAction("deletePrevious", function() { var selection = view.getSelection(); if (selection.start === selection.end) { var model = view.getModel(); var lineIndex = model.getLineAtOffset(selection.start); var lineStart = model.getLineStart(lineIndex); var offset = selection.start - lineStart; var tabSize = options.tabSize || 8; if (offset >= tabSize && offset % tabSize === 0) { var text = view.getText(lineStart, lineStart + offset); if (!/[^ ]/.test(text)) { view.setText("", selection.start - tabSize, selection.end); return true; } } } return false; }); The problem I see adding this feature to textView is that other user might needed a slightly different behaviour for expand tabs (for example, other user might need \t in clipboard operations to be converted to spaces). Other problem is that we have already other implementation of tab (and backspace). The editor re-implements the "tab" action (it changes it so that when the selection spans over several lines all the lines are added a tab), it also adds a shift-tab action. See editorFeatures.js. If we decided to add expandTab to textView we would need to add API (to return expandTab and tabSize) so that editorFeatures can be changed to honor expandTab too. If we decided to add expandTab to the editor we will need to changed the current "tab" and "deletePrevious" function. What do you think ?
forgot to mention, IMO there is small bug in your code. Set the selection to start at the start offset of a line and to end at second offset of the following line, hit tab. Only two white spaces are added. I would expected 4. This happens because you used caret offset instead of selection.start.
Hello Felipe! Thanks for your input. You have good points, but expanded tabs to spaces seems like a textview feature, like tabsize. I mean we can put everything, then, in editorFeatures, and move all actions there. TextView needs to grow as well, there's not a clear distinction between editor features and textview features. The way I see it, this is something that should be part of the textview features, it's the base stuff, everyone should be given the option to expand tabs to spaces. I haven't yet studied editorFeatures, but I presume that automatic indentation based on the previous line indentation is also something I would expect in TextView. The editorFeatures.js distinction is somewhat artificial. Nonetheless, if you want, I will move/merge my implementation into editorFeatures.js. The bug you found is indeed something I noticed after making the pull request. Will fix it. Thanks for your heads up!
(In reply to comment #4) > Hi Mihai, > > Note that I can't release new features to master till the summer release is > out. > (another week and it will be okay). No worries! > The problem I see adding this feature to textView is that other user might > needed a slightly different behaviour for expand tabs (for example, other user > might need \t in clipboard operations to be converted to spaces). Sure, but they can do their implementation then, for special purposes. Basic stuff is ... basic. :) > Other problem is that we have already other implementation of tab (and > backspace). The editor re-implements the "tab" action (it changes it so that > when the selection spans over several lines all the lines are added a tab), it > also adds a shift-tab action. See editorFeatures.js. Nice! Will check it. > If we decided to add expandTab to textView we would need to add API (to return > expandTab and tabSize) so that editorFeatures can be changed to honor expandTab > too. Thought of it. I almost wanted to add this for my API needs, but said "no" because I did not need it right now. Do you want it? > If we decided to add expandTab to the editor we will need to changed the > current "tab" and "deletePrevious" function. Sure, but this points out a flaw. I mean, we add new features to TextView and editorFeatures fails to use them. Shouldn't these things work a bit differently? Editors shouldn't overwrite existing functionality, they should build on top of it.
(In reply to comment #7) > > If we decided to add expandTab to textView we would need to add API (to return > > expandTab and tabSize) so that editorFeatures can be changed to honor expandTab > > too. > > Thought of it. I almost wanted to add this for my API needs, but said "no" > because I did not need it right now. Do you want it? Not now, the real solution should allow the application not only to get a option used when the textview was created but also to set them. (for example, the only way the change tabSize now is to create the textview). > > > If we decided to add expandTab to the editor we will need to changed the > > current "tab" and "deletePrevious" function. > > Sure, but this points out a flaw. I mean, we add new features to TextView and > editorFeatures fails to use them. Shouldn't these things work a bit > differently? Editors shouldn't overwrite existing functionality, they should > build on top of it. There valid cases where a specialized editor might want to overwrite a textview action. For example, a code editor might want to overwrite cursor next/previous to implement camel case. I will talk to Silenio and see if he has any suggestion for us in this area.
Mihai, in your project are using only textview or editor+textview ? Sorry this it taking longer to be fixed but I think it is okay cause I don't think you are stuck (you can use the code in comment 4).
(In reply to comment #9) > Mihai, in your project are using only textview or editor+textview ? I use the TextView only. > Sorry this it taking longer to be fixed but I think it is okay cause I don't > think you are stuck (you can use the code in comment 4). That's correct. I am going to make my own "editorFeatures.js" into which I will put code for tab, deletePrevious and enter, and perhaps some more actions.
Added expandTab option to TextView and implemented editorFeatures.js tab action taking into consideration the new option. Fixed http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=4db96cc92961fb34b09f92d9aa47908ed74a88e4
Thanks Mihai for the changes
(In reply to comment #12) > Thanks Mihai for the changes Thank you for landing these changes! This helps me trim our Orion integration code.
