phronmophobic / membrane.term

A terminal emulator in pure clojure
Eclipse Public License 1.0
58 stars 1 forks source link

Improve programmatic interface #26

Closed phronmophobic closed 2 years ago

phronmophobic commented 2 years ago

A pet peeve of mine is libraries that have their API dominated by their human interface without providing a solid programmatic interface. The API for run-term and screenshot should be oriented towards use by programs rather than the cli.

Currently, run-term and screenshot accept arguments that are convenient to produce from the cli and immediately convert them to a format that's used by the rest of the program. Instead, they should accept the same data format they use throughout the rest of the program and the conversion from cli friendly formats should happen elsewhere.

For example, the color-scheme argument is a readable value that has a plist of colors. For the rest of the program, the color scheme is a map of color names to rgb values. If someone wanted to programmatically produce a color scheme for use with run-term, they would have to do something goofy like serialize their color scheme and decide on a way to make it readable.

It's a little less obvious whether font-family and font-size are ok as is, but I think we should accept one font argument rather than font-family and font-size for the following reasons:

  1. font-family and font-size are immediately converted to a different format
  2. I don't think the API would be the same if it wasn't designed alongside the cli interface
phronmophobic commented 2 years ago

@lread , any thoughts on the above?

lread commented 2 years ago

Heya @phronmophobic!

Yes, I agree, the command line has been the driver, for sure.

I think you make an interesting point about color-scheme.
The Clojure API should accept an easy-to-use sensible Clojure structure. And I suppose the convertor from iterm2 plist format to that sensible format should be available for API use as well. What folks find easy-to-use might vary, many might be more familiar with html hex #rrggbbaa values than membrane's rgba encoding. Perhaps convertors from popular formats to membrane encoding might be interesting. Then again, membrane's rgba encoding is pretty easy to understand on its own.

I've less of an opinion on the font options. From a command line perspective, I think it is usable (what might be a good alternative?). From an API perspective, yeah... I think a font {:size x :family y} map might have been what I had come up with. Maybe. But nested option maps also have their drawbacks.

phronmophobic commented 2 years ago

I'm really happy with the command line interface. I just want to make sure the cli interface is built on top of the programmatic interface and I think run-term and screenshot should have a programmatic interface that is used by the cli interface.

phronmophobic commented 2 years ago

Unless there's a good reason, I think run-term and screenshot should accept data in the format that they use naturally (ie. the formats returned by load-*)

lread commented 2 years ago

Ah, I see.

This seems straightforward for color-scheme, and makes sense to me.

But for font, maybe less so? Right now, our load-terminal-font is enriching the font returned by membrane.ui/font with some membrane.term specific keys. Should the API expect a return value from membrane.ui/font? Doesn't that expose an implementation detail?

Also: tangental side note: looking at code, I think the API path has no default font.

phronmophobic commented 2 years ago

But for font, maybe less so?

Yea, I'm also less sure about how the font related arguments should be specified.

Should the API expect a return value from membrane.ui/font? Doesn't that expose an implementation detail?

At least in my head, it seems reasonable to think that someone who wanted programmatically call run-term might also want to use other features provided my membrane, but maybe the membrane part would be incidental. Using the model from membrane.ui provides other related functionality that might be useful (like getting font metrics). As you've seen, some parts of the membrane.ui model are sorta clunky and I would probably do things differently if starting from scratch, but it still is meant to provide a reasonable model.

If we were to use some other format for representing the font, it would be nice to use a common format rather than having an adhoc specification just for these two functions. I guess another possibility is to decide on a better model and incorporate that into membrane.

Also: tangental side note: looking at code, I think the API path has no default font.

😛 That shouldn't be too hard to fix.

lread commented 2 years ago

I think the current fruits of this discussion are:

  1. At the API level, color-scheme should be passed in as the current output of load-color-scheme
  2. When values are unspecified, the API should default to monospace for font-family and 12 for font-size.

TBD is what to do with font. There are many valid ways to craft a thing, so long a the rationale is rational, I think we are good. If membrane.term is considered an extension/add-on of membrane, I think providing a membrane.ui created font is fine. But if membrane is to be considered an implementation detail of membrane.term then requiring membrane created things as inputs is not so great. The fact that you've named it membrane.term does give some good weight to this be a membrane thing, yeah? But it does tie the user to the implementation specifics of membrane, and maybe even a specific version of membrane.

I think you are probably ok either way. I've no strong feeling here. Your call, but I'd maybe leave the font as-is for now and see how font loading from url and/or path plays out (if we plan to tackle that). Oh... there might also be emoji font options someday.

phronmophobic commented 2 years ago

Should the :play option for screenshot be a filename or a string? At the repl, it's much more convenient to do (screenshot {:play "ls -l"}). Currently, we slurp the whole string regardless.

lread commented 2 years ago

Should the :play option for screenshot be a filename or a string? At the repl, it's much more convenient to do (screenshot {:play "ls -l"}). Currently, we slurp the whole string regardless.

Hmm... yeah, probably a string is a better choice for the API. No real downside is coming to my mind, any you can think of?

phronmophobic commented 2 years ago

The only potential drawback is if someone wanted to have a very long script, then passing a file means the whole script wouldn't have to be in memory, but I think that's very speculative.

Ok, let's make screenshot's :play argument a string!

phronmophobic commented 2 years ago

Programmatic interface successfully improved!