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

Blessed xterm fixes #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

johnpoth
Copy link

In this PR I propose to

  1. remove the cloning of the options as it may result in errors when using the parent:screen option.
  2. make blessed-xterm work in the browser as the global.window is read only.

Thanks!

@rse
Copy link
Owner

rse commented Jan 16, 2018

In my code comment for the clone() usage, I wrote: "clone options or all widget instances will show at least the same style, etc" How do you think this issue is now adresses by just removing the clone() call? I do not see what you are doing instead?

@johnpoth
Copy link
Author

Hi @rse , thanks for reviewing! I saw the comment. Could you please elaborate the problem of widgets having the same style? I tried reproducing using the sample.js file by applying different styles to the two terminals and it worked without the clone() call. Thanks!

@rse
Copy link
Owner

rse commented Jan 17, 2018

I've currently no time to reproduce, but AFAIK the issue was that the style of a window being rendered active, or scrolling (in visual mode) or in error state was broken without this. One of those three states was not rendered correctly AFAIK.

@johnpoth
Copy link
Author

@rse no worries I'll try to reproduce on my side too.

From a design standpoint I don't think it should be needed to clone the options. So if it is needed I think it would be more of a bug in blessed/blessed-xterm. My 2 cents ofc :)

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.

2 participants