Change image storage to use new ExtendedAttrs payload field

[PerBothner]

Adds a general-purpose payload hook to ExtendedAttrs. This allows us to get rid of the ExtendedAttrsImage logic, which can easily conflict with other extensions.

The PR avoids looking into BufferLine internals by instead using loadCell and setCell. The calls to loadCell may add slightly more overhead; if that is measurable, we can look into reducing the number of loadCell calls.

This is an implementation of this suggestion.

It might make sense for a future update to remove the urlId field from ExtendedAttrs and instead use the general payload hook.

[PerBothner]

An optional optimization is to replace imageId (which needs to be looked up in the _images map) with a direct reference to IImageSpec:

class ImageTileInfo {
  constructor(
    public imageSpec: IImageSpec | undefined = undefined,
    public tileId = -1) {
  }
}

A further possibility would be to remove the _images map. Since each IImageSpec has an associated Marker, when we need to iterate over all images, we can do that by iterating over markers. This assumes we add a payload field to Marker (as suggested in PR #5853), and that the payload field is used to point back to the IImageSpec.

[Tyriar]

It's a bit of a code smell that we touch private API here at all. If the image addon only needs to attach extended data, could we make a real API to allow attaching data using an arbitrary key?

interface IBufferCell {
  setCustomData(key: string, value: object);
  getCustomData(key: string): object | undefined
}

Even adding this public API to xterm.d.ts and keeping the internals access as-is would be preferable to bringing in loadCell/CellData imo.

@jerch does this look like the shape you would expect?

[PerBothner]

The setCustomData/getCustomData functionality is certainly nice and general - but it could be expensive. Conceptually we're adding a separate Map for each cell - or at least one for every cell with HAS_EXTENDED. Presumably we'd want keep ExtendedAttrs (or its replacement) immutable.

We can replace the loadCell with the public getCell method - but ImageStore._writeToCell need the ability to modify a cell, currently done with setCell. What is the public API equivalent? From what I understand, getCell returns a copy of the state of a cell. Calling setCustomData presumably only updates the copy - we need to write back to the BufferLine.

[PerBothner]

This comment below for a (slightly) more detailed suggestion. I've focused on the internal API; we can discuss the public API once the concepts are clear.

[PerBothner]

On further thought, I think it might be worthwhile to consider merging the Marker and ExtendedAttrs concepts. As you suggest, we could add a general API to add "custom" data (specified by a key and an arbitrary value) to a cell. Internally it probably make sense to attach the custom data to a range of cells in a line, rather than just a single cell. And the API should make it convenient to apply custom data to a whole range using a single method call.

I believe the changes to Marker in PR #5853 provide a good foundation for this model. That change attaches the Markers directly to a LogicalLine making it efficient to attach and query for custom data for a line. Reflow is handled with no extra work. PR #5853 adds a startColumn to a Marker; we would want to be able to also specify an end column. The PR allows a general payload property on a Marker; we might add a key property. The implementation represents the markers associated with a line as a simple linked list. That representation works well if there are at most a small number of Marker ranges associated with a line, which I think would almost always be the case.

With this change, I think we can gradually get rid of ExtendedAttrs. The HAS_EXTENDED flag would be set if there is marker whose range includes the current cell. We would add marker keys for "image", "link-url", "fancy-underlining", "input-prompt", "input-line", and so on, as needed. As an transitional step, we can keep store the ExtendedAttrs type as a payload (using "extended-attrs" as a key).

However, I do think it makes sense to merge in PR #5853 (which includes the changes in this PR) before going further with the "custom data" route.

[PerBothner]

I merged from upstream and all tests pass. Can we merge this? It is blocking BufferLine/LogicalLine changes. @Tyriar requested one small change (which I believe I've made), and potentially a more general API (which is a bigger project that should be considered in conjunction with other Marker/Decorator changes).