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

use jansi's windows support #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

use jansi's windows support #44

wants to merge 1 commit into from

Conversation

amaranth
Copy link
Contributor

Change jansi shading to include JNI package for Windows support and enable
it in AnsiWindowsTerminal so Windows users can have working ansi support
without installing extra programs.

Change jansi shading to include JNI package for Windows support and enable
it in AnsiWindowsTerminal so Windows users can have working ansi support
without installing extra programs.
@scgray
Copy link

scgray commented May 23, 2012

+1

@@ -64,6 +64,8 @@ private static OutputStream wrapOutputStream(final OutputStream stream) {
}

private static boolean detectAnsiSupport() {
AnsiConsole.systemInstall();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is valid, detection of ANI support should not install ANSI support, which is what this change does.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, this should be part of the software using jline imho.

@gnodet
Copy link
Member

gnodet commented May 30, 2012

I don't think we should do that. AnsiConsole.systemInstall() messes with the system streams and should be in control of the caller, not a side effect of detecting if it will work. The main reason is that some software want to do more with system streams, such as redirecting or having a thread based streams like Karaf.

@jdillon
Copy link
Member

jdillon commented May 30, 2012

I don't really get this pull at all... windows should work w/o these changes presently, is this not the case?

@amaranth
Copy link
Contributor Author

amaranth commented Jun 1, 2012

Without these changes you have to use ansicon to get ansi support on windows. Otherwise it just spits the ansi codes out in to the terminal which looks like garbage to people who don't understand it.

@gnodet
Copy link
Member

gnodet commented Jun 11, 2012

Don't you embed jline in your application ? You can easily call AnsiConsole.systemInstall() in your application before starting the console.

@jonatzin
Copy link

What is the status of this change can i use this change in my code?

@gnodet
Copy link
Member

gnodet commented Jun 27, 2012

@jonatzin: not sure to understand your question. AFAIU calling AnsiConsole.systemInstall() before creating the console allows you to cleanly support Ansi on windows.
Fwiw, I think we can include that call, but there has to be a way to disable it so that people that have more complex setup wrt to system streams can still have control over what happens.

@jonatzin
Copy link

@gnodet Sorry I'll rephrase. Currently the issue mentioned above is a problem in my code (which embeds jline 2.6), can I apply the above code changes? Should I just use AnsiConsole.systemInstall(); in my code? Should I change the pom.xml?

@gnodet
Copy link
Member

gnodet commented Jun 27, 2012

I'd go for calling AnsiConsole.systemInstall() before creating the console ... and I don't think the pom needs any change afaik.

@jonatzin
Copy link

OK, so just to make sure I shouldn't change the source of jline?

@jonatzin
Copy link

Also when should I call AnsiConsole.systemInstall()? Wouldn't calling it on a linux machine cause a problem?

@gnodet
Copy link
Member

gnodet commented Jun 27, 2012

Well, I guess you should try it, but I'm calling it myself without any problems ;-)
Just try with the 2.7 released available on maven central and report if you hit any problems.
http://central.maven.org/maven2/jline/jline/2.7/

@jonatzin
Copy link

Hello everyone, So I regret to say that simply calling AnsiConsole.systemInstall() doesn't solve the issue. My code works fine on unix machines, Win7 however it doesn't work on Win2008R2SP1. Any suggestions?

@gnodet
Copy link
Member

gnodet commented Jun 28, 2012

Yeah, it seems there's an issue on Win 2008, but I don't think it's related to jline.
Users have reported success after having installed Microsoft Visual C++ 2008 SP1 Redistributable Package (x64) http://www.microsoft.com/en-us/download/details.aspx?id=29

But I don't think there's anything to do in jline, or maybe a graceful degraded mode, but we can't really install the system DLLs.

@gnodet
Copy link
Member

gnodet commented Jun 28, 2012

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.

5 participants