Support more graphics backends

phronmophobic commented 2 years ago

Currently, work in progress.

lread commented 2 years ago

Interesting!

phronmophobic commented 2 years ago

This patch relies on some WIP improvements to membrane, but it mostly works with swing. There's a few differences between how membrane.skia and membrane.java2d fire events, but they shouldn't be too hard to sort out.

phronmophobic commented 2 years ago

@lread

I think I'm pretty close to being able to merge these changes in, but I wanted to get your thoughts on a couple things:

I was planning on adding a command line arg and was thinking --graphics with skia and swing as options.
What do you think the default should be? Generally, the skia backend is "better", but the swing backend doesn't require the extra skialib dependency and swing should theoretically also work on windows. For those reasons, I usually make swing the default backend.
This change might conflict with any font changes you're making. I'm happy to wait until those get pulled in so you don't have deal with conflicts, but if you haven't made many changes, then it might be easier to push them first.

lread commented 2 years ago

Cool!

Unless you have some reason you want to promote the use of skia, I think a default of swing is fine. We can touch on the pros and cons in the README.

And, very thoughtful of you to think of my WIP, but please do go ahead!

phronmophobic commented 2 years ago

I've added cli args for --toolkit so that either skia or java2d (ie. swing) can be used. Right now, I'm stuck on the fact that java2d doesn't recognize "monospace" as a font, but does recognize "monospaced". I think just having membrane ensure that "monospace" works for any toolkit is probably the right way to go, but I'm going to sleep on it.

I feel like the interface of run-term and screenshot is bending away from programmatic usage towards cli usage. I'll create a separate issue with my thoughts.

lread commented 2 years ago

The changes are much smaller than I was expecting! Very cool. Haven't run this at all yet, (can do sometime tomorrow), but the code looks very nice to me.

lread commented 2 years ago

I assume this is known and expected, but when I run using the java2d toolkit under jdk11, I see reflective access warnings in the terminal from which I launch membrane.term:

$ clojure -M:membrane.term run-term                                                                                                                                                                                                                                                                                     
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by clojure.lang.InjectedInvoker/0x0000000800229c40 (file:/Users/lee/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar) to method sun.font.StandardGlyphVector.getGlyphMetrics(int)
WARNING: Please consider reporting this to the maintainers of clojure.lang.InjectedInvoker/0x0000000800229c40
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

lread commented 2 years ago

Small weirdness: When I request --help when using java2d toolkit, a java app is briefly launched and closes (as you can see in my macOS dock): 2021-11-22_14-59-00

phronmophobic commented 2 years ago

I assume this is known and expected, but when I run using the java2d toolkit under jdk11, I see reflective access warnings in the terminal from which I launch membrane.term:

I thought that was a clojure issue. For other reasons, I just made an update to membrane.java2d to fix reflection warnings. I wonder if updating to the latest version of membrane fixes it?

com.phronemophobic/membrane {:mvn/version "0.9.31.5-beta"}

phronmophobic commented 2 years ago

Regarding the dock icon, if we really want to prevent the dock icon from appearing, we can either disable the dock icon altogether (I think it's useful when actually running the term), see https://github.com/phronmophobic/treemap-clj#fixing-the-mac-osx-dock-icon-appearing. Or we can wait to load the toolkit until after we determine if the cli command is run-term.

lread commented 2 years ago

dock icon

Yeah, I think it is generally useful. Just found it mildly odd that it showed up when I only requested help. I only mentioned it because I noticed it. Unless it bothers you, might not be worth an issue.

lread commented 2 years ago

Yup, your guess was good! Upgrading to membrane to version 0.9.31.5-beta gets rid of the reflection warnings.

phronmophobic commented 2 years ago

The dock thing seems like it’s worth an issue. There’s no harm in documenting it.

lread commented 2 years ago

Another very minor one: when I do a run-term under skia, I see "Membrane" as my window title but when I use java2d I see "title" as my window title.

Example from macOS: skia:

java2d:

Oh, and I guess the title bar background is a different color too.

phronmophobic commented 2 years ago

100% compatibility between toolkits is a non goal, but using the same title for the window makes sense. How about “membrane.term”? Using the same background color for the title bar also makes sense, but I’m not actually sure how to that. 🙈

lread commented 2 years ago

Yeah membrane.term would be the perfect title, I think! The color for the title bar could simply be a separate git issue that might or might not be addressed someday?

lread commented 2 years ago

I thought I'd give this a whirl under Windows.
I've not had any success yet. Any tips?

Membrane.term uses Pty4J which uses winpty... my brain is not understanding what I need to do. Do I need to be launching from Cygwin/MSYS?

phronmophobic commented 2 years ago

Unfortunately, my Windows experience is pretty useless. I did use Pty4J rather than wrapping fork_pty myself in hopes that it would automagically work 🤞 .

lread commented 2 years ago

Unfortunately, my Windows experience is pretty useless. I did use Pty4J rather than wrapping fork_pty myself in hopes that it would automagically work 🤞 .

Maybe I just need a tweak or two. I see that Pty4J includes Windows-specific tests. And I just successfully did a gradlew test from a cmd shell and they seemed to run successfully. I'll poke around a bit.

lread commented 2 years ago

Doh! Membrane.term runs /bin/bash and that's not right for Windows (what is? not sure yet). But if I, for test purposes, have membrane.term instead run ls -l, I see the following on Windows 10 for:

clojure -M:membrane.term run-term

lread commented 2 years ago

Windows support probably deserves its own issue.

phronmophobic commented 2 years ago

whoops, https://github.com/phronmophobic/membrane/blob/master/src/membrane/java2d.clj#L918 🙈 . I guess I'll have to make quick update to membrane to fix the title issue.

phronmophobic commented 2 years ago

Ok, I think I addressed all the comments that don't have their own separate issues. Everything look good now 👍 ?

lread commented 2 years ago

The font rendering on Linux for java2d is rough compared to skia.

(this could certainly be extracted to a separate issue)

Here are a couple of samples from Linux Mint using a 20pt font size to zoom in a bit.

clojure -M:membrane.term:skia run-term --toolkit skia --font-size 20

clojure -M:membrane.term run-term --toolkit java2d --font-size 20

Java version:

$java --version
openjdk 11.0.13 2021-10-19
OpenJDK Runtime Environment Temurin-11.0.13+8 (build 11.0.13+8)
OpenJDK 64-Bit Server VM Temurin-11.0.13+8 (build 11.0.13+8, mixed mode)

When using the same version of Java on macOS I don't see the rough font rendering on for java2d:

lread commented 2 years ago

My personal take? Merge it! I can extract my last finding to an issue if you agree that it is worth an issue.

phronmophobic commented 2 years ago

oof. The linux java2d rendering is pretty rough. I'll go ahead and merge for now, but an extracted issue would be great! It looks like anti-aliasing isn't turned on. Maybe it's just a simple config issue 🤞 .

Edit: I just created the issue at https://github.com/phronmophobic/membrane.term/issues/32.

phronmophobic / membrane.term

Support more graphics backends #14