Skip to content

Implement configurable tab stop width #497

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.

Sign up for GitHub

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

Jump to bottom
Merged
parisk merged 3 commits into from
Jan 20, 2017
Merged

Implement configurable tab stop width #497

parisk merged 3 commits into from
Jan 20, 2017

Conversation

@parisk
Copy link
Contributor

@parisk parisk commented Jan 20, 2017 •
edited
Loading

This PR introduces configurable tab stop width.

Current issues

  • No way to call term.setupStops() after setting the option, without a hack into the private API. A private _optionChanged event could definitely solve this. Ideas @Tyriar?
  • There is no easy way to re-render already printed tab stops. In order to achieve this we will have to separate the data from the presentation layer and re-render the whole thing, which is a big issue.

Documentation PR: xtermjs/xtermjs.org#13

Closes #488.

@parisk parisk added this to the 2.3.0 milestone Jan 20, 2017
@parisk parisk self-assigned this Jan 20, 2017
@parisk parisk requested a review from Tyriar January 20, 2017 14:52
@parisk parisk mentioned this pull request Jan 20, 2017
Closed
@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2017

What does setupStops do exactly? We can call it from within getOption like is done to fix scrollback?

@parisk
Copy link
Contributor Author

parisk commented Jan 20, 2017

setupStops defines the position in the terminal where tab stops are located.

I added it into setOption just like we do with cursorBlink, although I wasn't quite fond of the idea at first.

@parisk parisk force-pushed the issue-#488-tabstopwidth branch from a56e992 to 8cb46f2 Compare January 20, 2017 16:19
@parisk
Copy link
Contributor Author

parisk commented Jan 20, 2017

@Tyriar I rebased with master and resolved conflicts. This is ready for review.

Tyriar
Tyriar requested changes Jan 20, 2017
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

There is no easy way to re-render already printed tab stops. In order to achieve this we will have to separate the data from the presentation layer and re-render the whole thing, which is a big issue.

Do we need to worry about this and/or file a follow up?

demo/index.html Outdated
<h2>Options</h2>
<p>
<label><input type="checkbox" id="option-cursor-blink"> cursorBlink</label>
<label>cursorBlink <input type="checkbox" id="option-cursor-blink"></label>
Copy link
Member

@Tyriar Tyriar Jan 20, 2017

Choose a reason for hiding this comment

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

Normally checkboxes are at the start of the line?

Copy link
Contributor Author

@parisk parisk Jan 20, 2017

Choose a reason for hiding this comment

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

Did this to keep uniformity with the rest. Will revert.

demo/index.html Outdated
<label>scrollback <input type="number" id="option-scrollback" value="1000" /></label>
</p>
<p>
<label>tabStopWidth <input type="number" id="option-tabstopwidth" value="4" /></label>
Copy link
Member

@Tyriar Tyriar Jan 20, 2017

Choose a reason for hiding this comment

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

value=8 as the default?

Copy link
Contributor Author

@parisk parisk Jan 20, 2017

Choose a reason for hiding this comment

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

Nah, forgot it from testing.

demo/main.js Outdated
term.setOption('scrollback', parseInt(optionElements.scrollback.value, 10));
});
optionElements.tabstopwidth.addEventListener('change', function () {
term.setOption('tabStopWidth', parseInt(optionElements.tabstopwidth.value));
Copy link
Member

@Tyriar Tyriar Jan 20, 2017

Choose a reason for hiding this comment

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

Add radix of 10 as second arg as it's a mandatory arg https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt

demo/main.js Outdated
cursorBlink: optionElements.cursorBlink.checked,
scrollback: parseInt(optionElements.scrollback.value, 10)
scrollback: parseInt(optionElements.scrollback.value, 10),
tabStopWidth: parseInt(optionElements.tabstopwidth.value)
Copy link
Member

@Tyriar Tyriar Jan 20, 2017

Choose a reason for hiding this comment

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

Radix arg

src/xterm.js
this.element.classList.toggle(`xterm-cursor-style-underline`, value === 'underline');
this.element.classList.toggle(`xterm-cursor-style-bar`, value === 'bar');
break;
case 'tabStopWidth': this.setupStops(); break;
Copy link
Member

@Tyriar Tyriar Jan 20, 2017

Choose a reason for hiding this comment

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

Eventually we could move this to event listeners which would probably be a better structure.

Copy link
Contributor Author

@parisk parisk Jan 20, 2017

Choose a reason for hiding this comment

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

Yep. It does the job for now, so let's keep this for another PR.

@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2017 •
edited
Loading

Changing tab will likely break ls output? How does ls fetch the terminal's tab size? Related: #259

@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2017

Looking at the tab stuff for #259, I don't think re-rendering would actually be desirable. There is the issue of how programs get the tab size though.

@parisk
Copy link
Contributor Author

parisk commented Jan 20, 2017

Everything seems to be working fine. I think that re-rendering is an open question and that we can revisit it in the future.

ls looks to be working just fine. I am pretty sure though that it is using spaces and not tabs.

Screnshot

image

@parisk
Copy link
Contributor Author

parisk commented Jan 20, 2017

I also fixed comments in demo, so @Tyriar feel free to take another look.

@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2017

It was certainly tabs on my Macbook, I'll grab your branch and verify.

@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2017

This is what I see on a dir with a bunch of files of varying lengths. The first is default (8), the second is 4 via setOption:

screen shot 2017-01-20 at 2 49 47 pm

@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2017

This also happens on Terminal.app, maybe this is just a price you pay?

@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2017

You can fix this on linux by doing something like:

alias ls=ls --tabsize=4

I guess that's just how it is 😄

@parisk
Copy link
Contributor Author

parisk commented Jan 20, 2017

Well, while this view sucks, technically there is nothing wrong about it. Take a close look and you will see that each file name is being printed at the first available tab stop.

The tab stop width though is not available somehow to ls in order to take care of properly aligning everything. I guess this is where you have to use ls --tabsize=4.

My opinion is that this is the price you have to pay eventually for such a feature and that you should be careful with using it.

I guess it has it's use cases though.

@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2017

Yeah, this is the behavior of other terminal emulators that set via calling tabs so 👍

@parisk parisk merged commit 7b87e78 into master Jan 20, 2017
@vincentwoo
Copy link
Contributor

vincentwoo commented Jan 21, 2017

Sorry was late to the party. I agree that tab stop width changes should not rerender previous content. Otherwise, I have no comments!

@Tyriar Tyriar deleted the issue-#488-tabstopwidth branch January 21, 2017 02:23
@Tyriar Tyriar modified the milestone: 2.3.0 Feb 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tyriar Tyriar Tyriar approved these changes

Assignees

@parisk parisk

Labels

None yet

Projects

None yet

Milestone

2.3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@parisk @Tyriar @vincentwoo