-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add scrollback option to config #3038
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🙌 Yes, we should add this to the docs. Can you open a PR with this in hyper-site? I think it's basically just copy this block and add scrollback in the same way.
@jaridmargolin Thank you for this PR 🙏 You're totally right. We need more unit tests and more functional tests with |
Never too late to start. At least xterm is decently tested (see e.g. https://github.com/xtermjs/xterm.js/tree/master/src)... looking into tests. At work I'm spoiled by TDD using jest and jsdom with mocking of the API data (which is tested separately) - would like to be able to have a similar TDD flow. |
Yay! I've been wanting to scroll higher. Have you tested how high this value can be before performance becomes an issue? |
I just added the scollback option in v2.1.1 and im not seeing it. It looks like this change was merged sometime ago so it should be there. Am i missing something here? |
@AhmedBM I can confirm it works in 2.1.2, though I don't know how to set it to infinite. For now I went with an absurdly large value
|
I tried this and don't really get that much more when scrolling back up. It's for sure not close to being infinite. Any ideas? |
I still don't have additional scrollback after setting: {
scrollback: 100000000
} What do I do? |
Agree with @rjurney, scrollback appears to not actually do anything to the buffer length. Seems a bit crazy this isn't something of higher priority. I have no idea how people are using this with only an 1000 buffer length. |
@ddimico This makes the thing unusable. I abandoned it. If it is fixed I'll look again. |
where i set this value? in .hyper.js? |
hyper prolly solely relies on the scrollback handling done by xterm.js and has no additional infinite scroll shim. If so, you should not try to set it to some arbitrary high value or the app will crash with OOM. Some calculations - JS engines typically dont allow more than ~2GB to be claimed. A cell takes 12 bytes, thus:
and so on... |
Echoing this does not work - using config.json |
Just wanted to touch base on this. Did somebody ever find a way to get this working? It still doesn't work for me even on the latest hyper version. |
I would like to confirm that this works. You will need to add the setting under config: {...}, it is the same as adding lineHeight or letterSpacing. Follow the same format as any general setting under the config block :-) |
The The option does work, you just have to add it yourself. |
As I was preparing this PR I was surprised to find that there was very little test coverage. Perhaps I am missing something?
It'd be great to get this pushed through and I am fairly confident the change is working as expected, but it feels strange to add functionality without writing/running tests 🤔
... Additionally, if there is interest in merging this PR, documentation should be updated accordingly.