refactor(web): rename Activator.activate → Activator.canActivate

[ermshiperete]

This change tries to improve the naming of the property since it denotes a state and doesn't cause an action to happen. Naming it canActivate might make this clearer.

Build-bot: skip build:web
Test-bot: skip

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

[keymanapp-test-bot]

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

• Web (no artifacts on BuildLevel "build")
  • KeymanWeb Test Home - build : ✅ all tests passed

[SabineSIL]

LGTM

[mcdurdin]

I don't think this is correct. I think it is representing 'activated' rather than 'canActivate'. The abstract Activator has no concept of setActivate but the subclasses do (why?). But I am not sure. It's awfully complex. The term activator is also changed to activationModel in some places, making it frustratingly difficult to trace and understand usage.

It is hard to understand why the config.activator property is not something that's entirely controlled within the osk view. Seems like that would be cleaner.

activationModel is used in only two places outside the OskView class hierarchy (one of which is explicitly cast to TwoStateActivator, breaking the abstraction). activator is used in only 1 place -- in initialization. This suggests that this entire abstraction should be refactored to something internal, and the Activator classes could probably just disappear.

This all feels too abstracted and complicated right now.

Looking at my original review of this, this was added when OSK component was refactored out of the original class hierarchy, where I wrote:

This is huge. I am unable to review this thoroughly as there are too many changes.

So, I guess we live and learn?

[mcdurdin]

This emit value seems like it still misleading?

[mcdurdin]

Not sure why the internal property is named flag and not _canActivate? (Avoid adding unnecessarily different identifiers)

[jahorton]

I don't think this is correct. I think it is representing 'activated' rather than 'canActivate'. The abstract Activator has no concept of setActivate but the subclasses do (why?). But I am not sure. It's awfully complex. The term activator is also changed to activationModel in some places, making it frustratingly difficult to trace and understand usage.

As the one who did the original conversion - it is meant to encapsulate the logic used to determine when the OSK should be activated and deactivated. This is often determined by whether or not there is an active context - a focused element ready to receive input. The logic is naturally more complex for the "floating", desktop-style OSK - users may opt to hide it, after all.

It is hard to understand why the config.activator property is not something that's entirely controlled within the osk view. Seems like that would be cleaner.

As currently implemented, it is essentially controlled by each specialized OSKView type's constructor. There is a case setting it externally... to its default for that mode:

keyman/web/src/app/webview/src/keymanEngine.ts

Lines 76 to 87 in 00a9b66

	const oskConfig: ViewConfiguration = {
	hostDevice: this.config.hostDevice,
	pathConfig: this.config.paths,
	// When hosted in a WebView, we never hide the Web OSK without hiding the hosting WebView.
	activator: new StaticActivator(),
	embeddedGestureConfig: buildEmbeddedGestureConfig(this.config.softDevice),
	doCacheBusting: true,
	predictionContextManager: this.contextManager.predictionContext,
	heightOverride: this.getOskHeight,
	widthOverride: this.getOskWidth,
	isEmbedded: true
	};

The Activator types (serving as a "model" of when each OSKView type should activate) are meant to control the OSK view's activation state - not the other way around. A default is set in each OSKView's constructor, but it's meant to be configurable to other patterns should we desire. In particular, it's meant to encapsulate the behavior logic for the OSKView superclass so that it doesn't need to worry about the logic details - it can just ask what this OSK mode + engine mode (app/webview, app/browser) specifies.

But this way, the OSKView base class itself, which implements the lion's share of OSK management - especially its activation and visibility, doesn't have to worry about the details.

activationModel is used in only two places outside the OskView class hierarchy (one of which is explicitly cast to TwoStateActivator, breaking the abstraction).

This is so that the main engine can properly report whether or not there is an active context that may receive input.

keyman/web/src/app/browser/src/keymanEngine.ts

Lines 81 to 83 in 00a9b66

	if(this.osk) {
	(this.osk.activationModel as TwoStateActivator<HTMLElement>).activationTrigger = e;
	}

Admittedly, it'd probably be better to also check instanceof and verify the correct activator type if maintaining the abstraction.