IGlk changes

[curiousdannii]

• [ ] Remove unused functions?
• [x] Remove [MarshalAs(UnmanagedType.LPArray, ArraySubType = UnmanagedType.U1)] from glk_put_buffer etc
• Return tuples rather than using out parameters? - only in GlkUtil
• Could we distinguish between arrays which are temporary and can be on the stack, and those which must be kept around for later? My understanding is that Span and Memory are for this purpose. So glk_put_buffer would accept a Span, but glk_stream_open_memory would accept Memory.
• We could make glk_request_line_event also take Memory, or go one step further: change it completely from the Glk API, to accept an initial string, and then return a string. (Unless you want to use the partial result if glk_cancel_line_event is called...)
  • Actually a pointer is the clearest way to indicate that an array needs to be pinned in place.
• [x] glkunix_fileref_get_name should just return a string. It doesn't make sense in the WASM/JS implementation to return a pointer.
• [x] Change to distinct reference types so that we can't mix them up, rather than putting streams, windows etc all under IntPtr.
• [x] streamResult needs to be a struct. And functions like glk_select could return a new struct, rather than modifying one that is passed in.
• [ ] Several functions should have nullable arguments (particularly for out arguments or streamResult)
• [ ] Sort functions

---

Would there be any issue cleaning up unused IGlk functions?

While most of them don't matter, glk_stream_open_memory in particular seems to be wrong, as it takes a IntPtr buf rather than byte* buf. Or maybe that doesn't really matter for the C interpreters, but I just can't work out how to convert it into an ArraySegment for JS marshalling ;)

For the rest, it would make it slightly smaller an API for us to keep track of. No need for glk_schannel_play if we always use glk_schannel_play_ext instead. Things like that.

[curiousdannii]

Actually some further ideas for IGlk:

Could we make it more of a natural C# API. Things like:

1. Removing [MarshalAs(UnmanagedType.LPArray, ArraySubType = UnmanagedType.U1)] from glk_put_buffer etc
2. I think the len parameter is not actually necessary in glk_put_buffer etc,
3. Return tuples rather than using out parameters?

Basically expanding the use of higher level functions like those in GlkUtil rather than low level ones.

But if we do any of these, should we rename any functions which have a different number of arguments than the actual Glk function, to avoid confusion? And if we do make changes like this, it would be more beneficial to do a DEFAULT_DLL_NAME implementation so that the Garglk/Winglk versions don't have to duplicate the code which turns our modified API into the true Glk API.

Could we distinguish between arrays which are temporary and can be on the stack, and those which must be kept around for later? My understanding is that Span and Memory are for this purpose. So glk_put_buffer would accept a Span, but glk_stream_open_memory would accept Memory.

We could make glk_request_line_event also take Memory, or go one step further: change it completely from the Glk API, to accept an initial string, and then return a string. (Unless you want to use the partial result if glk_cancel_line_event is called...)

glkunix_fileref_get_name should just return a string. It doesn't make sense in the WASM/JS implementation to return a pointer.

Change to distinct reference types so that we can't mix them up, rather than putting streams, windows etc all under IntPtr.

streamResult needs to be a struct. And functions like glk_select could return a new struct, rather than modifying one that is passed in.

[awlck]

I agree with removing the MarshalAs annotations on IGlk, they are totally redundant there.

As for making IGlk a high(er) level API in general, I'm reluctant. This would mean potentially duplicating the mapping code for each Glk implementation, or adding yet another layer of indirection (a NativeGlkRunner library that handles the translation). I'd rather add more higher-level stuff to GlkUtil and keep IGlk close to the native definitions.

glkunix_fileref_get_name should just return a string. It doesn't make sense in the WASM/JS implementation to return a pointer.

If that's a necessary change then I can do that.

Change to distinct reference types so that we can't mix them up

Agreed, I'll see to that.

streamResult needs to be a struct.

Well, my entire initial implementation was super jank. I didn't need the streamResult, so I only ever pass IntPtr.Zero for it.

As for the entire Span/Memory thing... probably a good idea, but I skimmed through the docs, and most of it went straight over my head, I'm afraid.

glk_stream_open_memory can probably just be removed for now, it's not currently used anywhere. (As discussed here, Adrift5 Blorb files need to be tweaked in order to be valid. I tried to use a memory stream, but Gargoyle doesn't support setting the Blorb map from a memory stream, so now I'm writing it out to a temporary file instead.)

[curiousdannii]

As for making IGlk a high(er) level API in general, I'm reluctant. This would mean potentially duplicating the mapping code for each Glk implementation, or adding yet another layer of indirection (a NativeGlkRunner library that handles the translation). I'd rather add more higher-level stuff to GlkUtil and keep IGlk close to the native definitions.

Sounds good, let's do this.

Well, my entire initial implementation was super jank. I didn't need the streamResult, so I only ever pass IntPtr.Zero for it.

Maybe right for now it could assert/throw if a non-zero pointer is passed in? Just to be safe.

As for the entire Span/Memory thing... probably a good idea, but I skimmed through the docs, and most of it went straight over my head, I'm afraid.

It's a bit over my head too... I'll see if I can get it working.

I tried to use a memory stream, but Gargoyle doesn't support setting the Blorb map from a memory stream, so now I'm writing it out to a temporary file instead.

I assume that's this part:

https://github.com/garglk/garglk/blob/e5802018ae13a4142c78f618c2430c7716d10fa9/garglk/cheapglk/cgblorb.cpp#L52-L58

@cspiegel Could this be changed to accept memory streams?

Or could we just call giblorb_create_map instead?

[cspiegel]

@cspiegel Could this be changed to accept memory streams?

Yes, I don't think this would be too difficult to implement. I'll give it a look.

[cspiegel]

I've got a pull request at garglk/garglk#759 for memory Blorbs. I'll continue to test a bit, but I'm making it available for any other testing/reviews for those who are interested.

[cspiegel]

OK, I've merged it in, so the next major Gargoyle release will support Blorb loading from memory streams.