feat(hints): allow custom hint generator functions

[peff]

Following up on the discussion in #58 (reply in thread), this adds a config option to provide a custom hint generator function. And since my goal there was sequential numeric hints, it also provides a stock function to do that.

This is more flexible than just a string-based option name (like "prefix-free" vs "numeric"), since you can provide an arbitrary function. But my hope is that by giving stock functions for those, it ends up being about equally easy for a user who wants to pick one of them.

It did create some headaches for me in the code, though. Where should these stock functions live? I stuck them in glide.hints.*, but that might not be appropriate. It also creates a challenge as that is not available when we are initializing the glide.o options (so we carry a null when the function is unset and fill in the default dynamically at the point of use).

I don't usually write typescript or javascript, so everything here is largely cargo-culted from surrounding code. Apologies in advance for any egregious style or idiom violations.

[RobertCraigie]

This is very cool, thank you!

My only concern with the current API shape is that it limits the sources for pulling in labels. As in, this doesn't let you define labels based on specific Element attributes, e.g. using the text of the element as a label.

My current thinking is that this could just happen in the pick function, where we'd add support for filling in a label property and then respecting that, e.g. along the lines of

  glide.hints.show({
    pick(hints) {
      hints[0].label = "a";
      return hints;
    },
  });

But this feels pretty hacky and does complicate the internal label generator process a lot, as we'd need to make sure the labels the user set don't overlap with the generated labels. (or just require all hints to be given a label, if you want this behaviour)

So I've been trying to think about a better approach but haven't come up with anything yet... do you have any ideas for how this could be integrated?

I'm asking as figuring this out may result in a breaking change to the API introduced in this PR, so ideally we'd figure out the design for content-process label generation before shipping this.

[RobertCraigie]

Ah to add a new glide.o option, you need to add it to glide.Options type as well as the internal runtime version.

[RobertCraigie]

One option is to use a getter/setter like the above option, so that it's lazily initialised.

e.g.

#hint_label_generator: glide.Options['hint_label_generator'] | null = null;

get hint_label_generator() {
  return this.#hint_label_generator ?? GlideBrowser.api.hints.label_prefix_free;
}
set hint_size(value: glide.Options['hint_label_generator']) {
  this.#hint_label_generator = value;
}

But defining the default label generator in hinting.mts and then just referencing it here would also be a fine approach.

[peff]

I think that's much nicer than having to do the defaulting at the point of use. For now there's only one such point of use, but it's possible we could grow more. And even without that, having the default defined here alongside the Options object is a much more sensible place.

[RobertCraigie]

Ah this file is auto-generated, so you should move this description to glide.Options and then run pnpm build:docs.

I also just added a header to this file so it should be more obvious that it's auto-generated :)

[peff]

Ah, thanks. I was fooled by the commit for hint_chars touching this file. Is there a particular reason it needs to be included in the repository if it's auto-generated? Certainly that would have resolved my confusion.

[RobertCraigie]

Ah good point, I personally like having generated docs files in the source code so it's easy to see how it'll change / has changed over time.

That said, I didn't really consider that it could be confusing for new contributors, so yeah for that reason it probably does make sense to exclude it from the repository.

[RobertCraigie]

fyi that this type doesn't match the implementations as here the arguments are (alphabet: string[], len: number) but the default generator just accepts len, label_prefix_free(len: number): string[].

[peff]

Ah, whoops. As you can probably guess, I played around with a few different possible interfaces. One was to take the alphabet (from hint_chars) automatically. But it felt odd since a generator may use its own alphabet (and the numeric one does so). So this is just left-over crud. Of course it worked anyway since (to my understanding) all of the type annotations are stripped out as the code is integrated into firefox.

[RobertCraigie]

Of course it worked anyway since (to my understanding) all of the type annotations are stripped out as the code is integrated into firefox.

Yup that is how it works :)

If you're curious, you can see the JS code that is actually being ran by navigating to the corresponding dist/$file.js file or opening the Browser Toolbox debugger and navigating to it.

[peff]

@robertcragie Thanks for all your feedback, and sorry it took so long to get back to this.

My only concern with the current API shape is that it limits the sources for pulling in labels. As in, this doesn't let you define labels based on specific Element attributes, e.g. using the text of the element as a label.

Good point. That's not something I want to do in my generator, but it seems like a reasonable thing for somebody to eventually try. My first instinct was to just pass in the set of hint objects themselves, but I worried that we'd be exposing too many internal details. However, it looks like the pick function already does this, so it makes sense to follow along here.

One curiosity is that we have an array of GlideResolvedHint here. Which are really just GlideHintIPC under the hood. Whereas the pick function is seeing real glide.ContentHint objects. I'm not entirely clear on how these IPC bits work or what is running where, but from the comment at

glide/src/glide/browser/actors/GlideHandlerChild.sys.mts

Lines 697 to 704 in 1d56ead

	actor.send_async_message("Glide::ResolvedHints", {
	location: props.location,
	auto_activate: props.auto_activate,
	// strip out the `target` as we cannot / don't need to send it
	hints: hints.map(({ element, ...rest }): GlideHintIPC =>
	props.debug ? { ...rest, element_id: element.id || undefined } : rest
	),
	});

it sounds like it might be hard to get the whole element into the generator. Would we need to be running the generator earlier, on the sending side of the IPC, then?

My current thinking is that this could just happen in the pick function, where we'd add support for filling in a label property and then respecting that, e.g. along the lines of

  glide.hints.show({
    pick(hints) {
      hints[0].label = "a";
      return hints;
    },
  });

But this feels pretty hacky and does complicate the internal label generator process a lot, as we'd need to make sure the labels the user set don't overlap with the generated labels. (or just require all hints to be given a label, if you want this behaviour)

Yeah, I agree that opens up some complications. I also suspect that there are many cases where you might want to orthogonally replace one function but not the other. For example, a custom keybind to trigger hinting with a specific pick matcher, but which you still want to label in the usual way (I have ;a in tridactyl bound to match only images with alt-text, and then show the selected alt-text in an alert box).

So I've been trying to think about a better approach but haven't come up with anything yet... do you have any ideas for how this could be integrated?

If we're OK exposing glide.ContentHint (and pick already does), then I think a function mapping glide.ContentHint[] into string[] is the best interface. We just need to resolve the IPC issues above. As I said, I'm a bit clueless about the IPC split, but I can't think of a good reason why the generator couldn't run in resolve_hints just after the pick function, pre-label all of the items, and then pass them through the IPC message.

I'm asking as figuring this out may result in a breaking change to the API introduced in this PR, so ideally we'd figure out the design for content-process label generation before shipping this.

Agreed.

I've pushed up another iteration. This one passes GlideResolvedHints, but as above, I don't think that's quite what we want.

Also having looked at the pick function, I notice this hinting.pickers namespace in

glide/src/glide/browser/base/content/hinting.mts

Line 178 in 1d56ead

export const pickers = {

Should there be a labelers namespace at the same spot with prefix_free and numeric entries (rather than shoving them into glide.hints)? I couldn't figure out how users are supposed to access those names, though. I.e., how would they do the equivalent of the keymap setup in

glide/src/glide/browser/base/content/plugins/keymaps.mts

Lines 44 to 49 in 1d56ead

	glide.keymaps.set(
	"normal",
	"gI",
	() => glide.hints.show({ auto_activate: true, editable: true, pick: hinting.pickers.biggest_area }),
	{ description: "Focus the largest editable element on the page" },
	);

[RobertCraigie]

FYI that the lint errors are because parameters in inline functions still need names, e.g.

Suggested change

	hint_label_generator: (GlideResolvedHint[]) => string[];
	hint_label_generator: (hints: GlideResolvedHint[]) => string[];

[RobertCraigie]

note: you'll need to define this function, and the one below, in glide.d.ts as well

[RobertCraigie]

Thanks for all your feedback, and sorry it took so long to get back to this.

No worries, thank you for working on this at all :)

it sounds like it might be hard to get the whole element into the generator. Would we need to be running the generator earlier, on the sending side of the IPC, then?

yes, that's the main difficulty here; we can't just provide a single API that let's you access the Element and also allows access to more sensitive glide APIs because they're only accessible in different processes.

I also suspect that there are many cases where you might want to orthogonally replace one function but not the other.

ah that is a great point! that completely discards that solution for me then.

If we're OK exposing glide.ContentHint (and pick already does), then I think a function mapping glide.ContentHint[] into string[] is the best interface. We just need to resolve the IPC issues above. As I said, I'm a bit clueless about the IPC split, but I can't think of a good reason why the generator couldn't run in resolve_hints just after the pick function, pre-label all of the items, and then pass them through the IPC message.

Yeah it could run in the content process. However I think we'll want to support this API in both the content process and the main process. Reasoning being that if it's executed in the main process, you can access the glide API, while in the content process you cannot.

Because of that, the current idea I'm actually leaning towards is continuing to execute the label generator in the main process, and providing a wrapper function to indicate it should be executed in the content process, for example:

glide.o.hint_label_generator = glide.content.fn((hints) => {
  hints[0]?.element;
  // ...
  return ["first", "second", "third"];
});

I think the biggest downside with this approach is the inconsistency with pick, where pick is always executed in the content process, which makes the behaviour here surprising...

Apart from that, it seems pretty nice, TypeScript can even infer the type of the hints argument which is crazy.

( also to be clear, adding something like glide.content.fn() should not happen in this PR, we can just figure out if this is a good enough solution to unblock this API :) )

Also having looked at the pick function, I notice this hinting.pickers namespace in
Should there be a labelers namespace at the same spot with prefix_free and numeric entries (rather than shoving them into glide.hints)? I couldn't figure out how users are supposed to access those names, though. I.e., how would they do the equivalent of the keymap setup in

Yeah right now users cannot access hinting.pickers.biggest_area. I like your proposed solution with a separate namespace:

glide.hints.pickers.biggest_area;
glide.hints.labelers.prefix_free;
glide.hints.labelers.numeric

Although labelers feels a little awkward? Can't come up with much better off the top of my head though...

[RobertCraigie]

nit: I think this implementation should just be in label_prefix_free in browser.mts instead of here

[RobertCraigie]

also note that GlideResolvedHint is an internal type, meaning it won't be included in the glide.d.ts that is shipped to users.

for this change we'll want to split glide.ContentHint into two types, like so

    export type Hint = {
      id: number;
      x: number;
      y: number;
      width: number;
      height: number;
    };
    export type ContentHint = glide.Hint & { element: HTMLElement };

and then use glide.Hint[] here.

---

tip! you can verify this by running pnpm bundle, it should output something like this

+ pnpm dts-bundle-generator src/glide/browser/base/content/bundled.d.ts -o src/glide/browser/base/content/dist/bundled.compiled.d.ts --inline-declare-global --project tsconfig.bundled.json
Compiling input files...
src/glide/browser/base/content/glide.d.ts(941,37): error TS2304: Cannot find name 'GlideResolvedHint'.