Skip to content

Conversation

@stephan-gh
Copy link
Contributor

@stephan-gh stephan-gh commented Jul 8, 2019

Fixes #1734. Fixes #1690 (?)

There were a few larger changes (new JLine/JNA version, changes in implementation of colors in TCA), so I am looking for some testing before I release the new version of TCA. @zachbr asked for a new release in Minecrell/TerminalConsoleAppender#7, so maybe you'd like to help with testing on Paper. :)

It looks fine to me in IDE+Terminal on Arch GNU/Linux, but I cannot test on macOS or Windows at the moment.

@zachbr
Copy link
Contributor

zachbr commented Jul 8, 2019

The JNA change is breaking, this will need to be held until 1.14.4 if that is required.

@stephan-gh
Copy link
Contributor Author

stephan-gh commented Jul 8, 2019

The JNA change is breaking, this will need to be held until 1.14.4 if that is required.

It seems so: jline/jline3@9d24f65

Edit: Urgh, this is going to cause me headaches for Forge...

@stephan-gh
Copy link
Contributor Author

@zachbr Do you use JNA for anything except TCA on Paper?
Given that the new JNA version is going to cause me headaches for Forge, I've been thinking about using Jansi again. There is no functional difference for JLine; both are equally supported. And Jansi is actually much smaller (280 KB vs 1400 KB for JNA).

I think I stopped using Jansi back then because I felt it was old and somewhat messy. But it has been continuously updated together with JLine, so it's probably fine.

@astei
Copy link
Contributor

astei commented Jul 9, 2019

@DemonWav has a branch for his experimental paperd daemon which uses JNA.

@astei
Copy link
Contributor

astei commented Jul 9, 2019

I can test this new TCA release in Velocity if you want.

@stephan-gh
Copy link
Contributor Author

@astei Sure. Any help with testing is appreciated!

@stephan-gh
Copy link
Contributor Author

@DemonWav has a branch for his experimental paperd daemon which uses JNA.

Okay. Either way is fine really. I think I'll just make it clear in the TCA documentation that both can be used. Right now it strongly recommends JNA for no real reason.

@zachbr
Copy link
Contributor

zachbr commented Jul 14, 2019

Looks good on macOS 10.15 and Windows 10 20H1

@DenWav
Copy link
Member

DenWav commented Jul 15, 2019

I'm not doing anything fancy with JNA in feature/daemon that needs any one feature over another, as long as we don't downgrade.

It's also certainly possible for me to switch to a JNI approach. JNA doesn't require any other native code which is why I went with it, but we'd only need a small amount of C code to use JNI instead which could be a separate project where the prebuilt binary is simply downloaded at build time for Paper.

Another option that would be even better if I could get it to work is the JNI code could be part of the paperd binary itself, which could be used in daemon mode as well...

So don't worry about me, if removing JNA is the best way forward for this, go for it, using JNI won't be that hard (and will have a lot less overhead too).

@stephan-gh
Copy link
Contributor Author

So don't worry about me, if removing JNA is the best way forward for this, go for it, using JNI won't be that hard (and will have a lot less overhead too).

I have decided that I will stop recommending one of them, so you will need to decide what to use in Paper. You could keep JNA now and switch to Jansi later once an alternative has been written for paperd.
Or whatever you like :) The functional difference is so minor that it does not matter which one is used.

@zachbr
Copy link
Contributor

zachbr commented Jul 17, 2019

I wouldn't mind not using JNA because it would let us backport this change to older versions. I will have to try and find some time to test 1.12.2 and 1.13.2 with the jansi dependency instead.

@stephan-gh stephan-gh changed the title Update TerminalConsoleAppender + JLine + JNA Update TerminalConsoleAppender + JLine, use Jansi instead of JNA Jul 17, 2019
@stephan-gh
Copy link
Contributor Author

I've pushed a commit to TCA to stop depending on JNA explicitly. Therefore each project using TCA now has to add an explicit dependency on either jline-terminal-jansi or jline-terminal-jna.

I've changed this PR to depend on Jansi instead (and keep the JNA dependency as-is), so it is no longer breaking :)

@DenWav
Copy link
Member

DenWav commented Jul 17, 2019

I'm testing out a new JNI implementation for paperd, should have it working today if all goes well. Should be cool to remove the JNA dep.

@DenWav
Copy link
Member

DenWav commented Jul 18, 2019

paperd and the feature/daemon branch are now moved off JNA and onto JNI.

@stephan-gh
Copy link
Contributor Author

Yay for conflicts! Now fixed :)
/me glances at #2324

@zachbr
Copy link
Contributor

zachbr commented Jul 20, 2019

I've added a commit to this PR to remove the JNA runtime dependency.
Jansi version confirmed as expected on Arch Linux, macOS 10.15, and Windows Server 2016

@stephan-gh
Copy link
Contributor Author

I've added a commit to this PR to remove JNA.
Jansi version confirmed as expected on Arch Linux, macOS 10.15, and Windows Server 2016

Great, thank you! I will release the new version of TCA soon and then we can merge this. 👍

@stephan-gh
Copy link
Contributor Author

Updated TerminalConsoleAppender to the 1.2.0 release, ready for merge now!

@zachbr I squashed your commit into mine. I hope you don't mind :)

Remove dependency on JNA since it is no longer needed.
@zachbr zachbr merged commit fa8b499 into PaperMC:ver/1.14 Jul 24, 2019
@zachbr
Copy link
Contributor

zachbr commented Jul 24, 2019

Thanks for your continued support and assistance with TCA 👍

@HexedHero
Copy link
Contributor

Since this update me and a lot of other owners have had this issue with their console:
https://cdn.hexedhero.com/u/uc2OBgyS7N.jpg

Using Pterodactyl 0.7.14
OpenJDK Runtime Environment (build 1.8.0_212-8u212-b01-1~deb9u1-b01)

Using -Dterminal.jline=false -Dterminal.ansi=true startup flags completely removed the >.... issue

@clrxbl
Copy link
Member

clrxbl commented Jul 25, 2019

@HexedHero Yeah I asked Minecrell about this earlier, I've made an issue for the Pterodactyl panel & the workaround PR with those arguments has been merged into parkervcp's eggs. (so this should be built in with the next daemon update) pelican-eggs/eggs#261

@stephan-gh
Copy link
Contributor Author

I will need to reproduce this myself with some debugging code to be able to say more. The problem is likely a mixture of many things. JLine prints >.... in weird situations (see jline/jline3#411 for a similar problem that was already fixed), but Pterodactyl likely provides an incomplete console implementation if it is visible.

Unfortunately, Pterodactyl does not quite look like it can be installed in a few minutes....

@stephan-gh stephan-gh deleted the tca-1.2.0 branch July 25, 2019 09:05
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.

6 participants