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

[wip] Feature/elastictabstops #1152

Merged
merged 16 commits into from
Feb 22, 2013
Merged

[wip] Feature/elastictabstops #1152

merged 16 commits into from
Feb 22, 2013

Conversation

gjtorikian
Copy link
Contributor

Addresses #215.

I became interested in elastic tabstops after taking a look at the issue above:

60RvQ

Since I'm a masochist (and I didn't feel like looking at Nick's Java code), I actually took a peak at this Python/Sublime text plugin, and did a port of that instead.

It's mostly working at this point. Remaining issues include:

  • Performance issues
  • Lack of multiple cursor support
  • A setting to enable / disable

The reason I'm opening this PR now is to ask @nightwing / @fjakobs questions on preferences for conventions.

This feature adds quite a few utility ($) functions. Is it preferred to stick them in a file somewhere, instead of littering editor.js with them ? On top of that, this only really works when a real \t is used. I'm not sure if that's a limitation or a "feature."

I would also like to make a better tab character, one that stretches across the width of the span. A "solution" I came up with was to use 2D transforms to stretch the arrow:

display: inline-block;
transform: scale(4,1);
-webkit-transform: scale(4,1);
-moz-transform: scale(4,1);
-ms-transform: scale(4,1);
-o-transform: scale(4,1);
transform-origin-x: 0;
-webkit-transform-origin-x: 0;
-moz-transform-origin-x: 0;
-ms-transform-origin-x: 0;
-o-transform-origin-x: 0;

Unfortunately, this causes interference with certain characters. The other route is to use a background image, but I am really lazy to create separate light and dark pictures. Plus, I would love to just be able to automatically use the same color as a comment. Is this possible, do you think?

As an added bonus, I am absolutely ashamed at the state of the API docs, there's quite a few missing or inaccurate info. I fixed a few of the ones I referred to but in a separate PR I'll need to fix it all up properly.

@gjtorikian
Copy link
Contributor Author

BTW I want to hook into the onDocumentChange method to better detect row changes. Is it ok if I extend Document's insertInLine / removeInLine methods to stop the emitting of the change event there? Otherwise the loop emits endlessly, because in editor.$adjustRow I need to call them. Or is there some other solution?

@nightwing
Copy link
Member

cool!

  • this shouldn't be in editor, it should be optional plugin which will add change listeners to the editor/session
  • to hook into document use change event
var inChange
editor.on("change",function(e){
    if (inChange) {
        // called because of changes from this function
        return
    }
    inChange = true
    editor.session.replace(range, value)
    inChange = false
})

another way is to collect change event and to call $adjustRow after a samll timeout

  • please put api doc fixes into separate pr, otherwise it will be hard to see what this pr actually does, (also this pr will take some time to be finished and api changes can e merged qickly)

@gjtorikian
Copy link
Contributor Author

Barev @nightwing . This is getting closer to finishing. The last thing to test is multiple cursors.

I have a strange bug with the UndoManager, seen in this video: http://screencast.com/t/4D2BAxmHhrCS

After hitting Cmd-Z the enter set of changes are selected. Is there some easy way to disable this? Or is it expected behavior?

Edit: I mean, I know I can manually deselect via the API. I suspect this is expected behavior, but I think I should change the code to prevent this.

@nightwing
Copy link
Member

it's an old bug in https://github.com/ajaxorg/ace/blob/master/lib/ace/edit_session.js#L1284
(related #939)

it definitely shouldn't merge not overlapping range
but i am not sure what is the right behavior, should it still select entered text or restore exact selection like sublime does?

selecting inserted text isn't useful after one character undo, but somewhat useful after deleting word

@nightwing nightwing closed this Dec 15, 2012
@nightwing nightwing reopened this Dec 15, 2012
@nick-gravgaard
Copy link

The Sublime Text package this is based on is not a proper implementation of my invention. See SublimeText/ElasticTabstops#4

@gjtorikian
Copy link
Contributor Author

Hi @nickgravgaard — I was about to continue our e-mail chain but this PR seems more appropriate.

Yeah, I hate to be the bearer of bad news, but I'm doing the same thing as the ST implementation--inserting and removing spaces: https://github.com/ajaxorg/ace/blob/7bbd0177314eddf8220ba0a92f10d0470c036f93/lib/ace/elastic_tabstops.js#L214-226

But, I'm committed to making this function the way you see fit. Another reason I wanted to do this was because I believe someone wanted (or someone was working on) proportional fonts, so I'd like this to work with that.

Here's how the Ace API works:

Screen Shot 2012-12-15 at 4 01 35 PM

Each tab area is actually an arrow character (that takes up one space), followed by a set of  s. You can set the tab space programmatically (for example, setTabSize(7) makes 7 &nbsps); there's also soft (just spaces) and hard (actual \ts) tab support. (Edit: if you see above in my original post, I also want to rework the fact that we're even using a as part of the implementation, but it's better suited for another PR.)

I suspect that, due to the crazy nature of tabs and websites, we're also out of luck in proper implementation. But I'd love your feedback nonetheless.

@nick-gravgaard
Copy link

Actually, from the look of that markup I think it may well be possible to do elastic tabstops properly. Let's consider line 19 from your screenshot and let's say the contents of the buffer for that line are "\tkey_t\tkey;\n". I think you would have a span containing "\t", followed by a span containing "key_t\t" and then a span containing "key;" (possibly with a "\n" on the end). You then just need to position the spans horizontally (and change their widths) using CSS so that they line up with matching "cells" on neighbouring lines.

Does that make sense?

@gjtorikian
Copy link
Contributor Author

Yeah, I see how that could work. So instead of modifying the buffer directly, add a CSS class to the spans in the rows and modify them that way. I'm trying to think if that will imply some performance loss but I'll try it out first and see how it behaves.

@gjtorikian
Copy link
Contributor Author

Unfortunately I am having a hell of a time doing this "properly". Any attempt I make to fake the tab stop spacing results in a horrendous mess, especially as you scroll out of view.

I think the current spaces implementation is good enough so @nightwing if you want to review and comment that would be great. I can fix up any issues you find.

@nick-gravgaard
Copy link

Ah, that's a shame :(

Could you rename this feature "elastic tabstops lite" to avoid any confusion with the real thing? See http://nickgravgaard.com/elastictabstops/news/elastic-tabstops-lite/

@@ -449,6 +449,21 @@ var Editor = function(renderer, session) {

// update cursor because tab characters can influence the cursor position
this.$cursorChange();

if (this.$useElasticTabstops) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this must go into separate function in elastic_tabstops.js
editor must know nothing about elastic tabstops,

insead of editor.setUseElasticTabstops(true) one should do

var ElasticTabstops = require("ace/elastic_tabstops").ElasticTabstops;
new ElasticTabstops(editor);

@nightwing
Copy link
Member

@gjtorikian sorry for such a long delay, i needed to finish pr for config.defineOptions first.
i've backed up your branch to feature/elastictabstops-unrebased and rebased it onto master
there is a small problem when editing text without tabs see d640d13,
And the problem with restoring selections after undo, which i'll fix soon.

nightwing added a commit that referenced this pull request Feb 22, 2013
@nightwing nightwing merged commit a7544b4 into master Feb 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants