Skip to content

Commit

Permalink
fix(core): drop support for plural event/gesture names (#10539)
Browse files Browse the repository at this point in the history
  • Loading branch information
@shirakaba
shirakaba committed May 7, 2024
1 parent d323672 commit 9be392f
Show file tree
Hide file tree
Showing 8 changed files with 323 additions and 269 deletions.
20 changes: 12 additions & 8 deletions apps/automated/src/data/observable-tests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export var test_Observable_addEventListener_MultipleEvents = function () {
obj.addEventListener(events, callback);
obj.set('testName', 1);
obj.test();
TKUnit.assert(receivedCount === 2, 'Callbacks not raised properly.');
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange,tested', as we have dropped support for listening to plural event names.");
};

export var test_Observable_addEventListener_MultipleEvents_ShouldTrim = function () {
Expand All @@ -176,13 +176,14 @@ export var test_Observable_addEventListener_MultipleEvents_ShouldTrim = function

var events = Observable.propertyChangeEvent + ' , ' + TESTED_NAME;
obj.addEventListener(events, callback);
TKUnit.assert(obj.hasListeners(Observable.propertyChangeEvent), 'Observable.addEventListener for multiple events should trim each event name.');
TKUnit.assert(obj.hasListeners(TESTED_NAME), 'Observable.addEventListener for multiple events should trim each event name.');
TKUnit.assert(obj.hasListeners(events), "Expected a listener to be present for event name 'propertyChange , tested', as we have dropped support for splitting plural event names.");
TKUnit.assert(!obj.hasListeners(Observable.propertyChangeEvent), "Expected no listeners to be present for event name 'propertyChange', as we have dropped support for splitting plural event names.");
TKUnit.assert(!obj.hasListeners(TESTED_NAME), "Expected no listeners to be present for event name 'tested', as we have dropped support for splitting plural event names.");

obj.set('testName', 1);
obj.test();

TKUnit.assert(receivedCount === 2, 'Callbacks not raised properly.');
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange , tested', as we have dropped support for listening to plural event names (and trimming whitespace in event names).");
};

export var test_Observable_addEventListener_MultipleCallbacks = function () {
Expand Down Expand Up @@ -223,7 +224,7 @@ export var test_Observable_addEventListener_MultipleCallbacks_MultipleEvents = f
obj.set('testName', 1);
obj.test();

TKUnit.assert(receivedCount === 4, 'The propertyChanged notification should be raised twice.');
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange , tested' with two different callbacks, as we have dropped support for listening to plural event names (and trimming whitespace in event names).");
};

export var test_Observable_removeEventListener_SingleEvent_SingleCallback = function () {
Expand Down Expand Up @@ -341,19 +342,22 @@ export var test_Observable_removeEventListener_MultipleEvents_SingleCallback = f

var events = Observable.propertyChangeEvent + ' , ' + TESTED_NAME;
obj.addEventListener(events, callback);
TKUnit.assert(obj.hasListeners(events), "Expected a listener to be present for event name 'propertyChange , tested', as we have dropped support for splitting plural event names.");
TKUnit.assert(!obj.hasListeners(Observable.propertyChangeEvent), "Expected no listeners to be present for event name 'propertyChange', as we have dropped support for splitting plural event names.");
TKUnit.assert(!obj.hasListeners(TESTED_NAME), "Expected no listeners to be present for event name 'tested', as we have dropped support for splitting plural event names.");
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange , tested', as we have dropped support for listening to plural event names (and trimming whitespace in event names).");

obj.set('testName', 1);
obj.test();

obj.removeEventListener(events, callback);

TKUnit.assert(!obj.hasListeners(Observable.propertyChangeEvent), 'Expected result for hasObservers is false');
TKUnit.assert(!obj.hasListeners(TESTED_NAME), 'Expected result for hasObservers is false.');
TKUnit.assert(!obj.hasListeners(events), "Expected the listener for event name 'propertyChange , tested' to have been removed, as we have dropped support for splitting plural event names.");

obj.set('testName', 2);
obj.test();

TKUnit.assert(receivedCount === 2, 'Expected receive count is 2');
TKUnit.assert(receivedCount === 0, "Expected no event handlers to fire upon the 'propertyChange' event when listening for event name 'propertyChange , tested', as we have dropped support for listening to plural event names (and trimming whitespace in event names).");
};

export var test_Observable_removeEventListener_SingleEvent_NoCallbackSpecified = function () {
Expand Down
136 changes: 62 additions & 74 deletions packages/core/data/observable/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,6 @@ const _globalEventHandlers: {
};
} = {};

const eventNamesRegex = /\s*,\s*/;

/**
* Observable is used when you want to be notified when a change occurs. Use on/off methods to add/remove listener.
* Please note that should you be using the `new Observable({})` constructor, it is **obsolete** since v3.0,
Expand Down Expand Up @@ -153,93 +151,89 @@ export class Observable {

/**
* A basic method signature to hook an event listener (shortcut alias to the addEventListener method).
* @param eventNames - String corresponding to events (e.g. "propertyChange"). Optionally could be used more events separated by `,` (e.g. "propertyChange", "change").
* @param eventName Name of the event to attach to.
* @param callback - Callback function which will be executed when event is raised.
* @param thisArg - An optional parameter which will be used as `this` context for callback execution.
*/
public on(eventNames: string, callback: (data: EventData) => void, thisArg?: any): void {
this.addEventListener(eventNames, callback, thisArg);
public on(eventName: string, callback: (data: EventData) => void, thisArg?: any): void {
this.addEventListener(eventName, callback, thisArg);
}

/**
* Adds one-time listener function for the event named `event`.
* @param event Name of the event to attach to.
* @param eventName Name of the event to attach to.
* @param callback A function to be called when the specified event is raised.
* @param thisArg An optional parameter which when set will be used as "this" in callback method call.
*/
public once(event: string, callback: (data: EventData) => void, thisArg?: any): void {
this.addEventListener(event, callback, thisArg, true);
public once(eventName: string, callback: (data: EventData) => void, thisArg?: any): void {
this.addEventListener(eventName, callback, thisArg, true);
}

/**
* Shortcut alias to the removeEventListener method.
*/
public off(eventNames: string, callback?: (data: EventData) => void, thisArg?: any): void {
this.removeEventListener(eventNames, callback, thisArg);
public off(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
this.removeEventListener(eventName, callback, thisArg);
}

/**
* Adds a listener for the specified event name.
* @param eventNames Comma delimited names of the events to attach the listener to.
* @param eventName Name of the event to attach to.
* @param callback A function to be called when some of the specified event(s) is raised.
* @param thisArg An optional parameter which when set will be used as "this" in callback method call.
*/
public addEventListener(eventNames: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
public addEventListener(eventName: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
once = once || undefined;
thisArg = thisArg || undefined;

if (typeof eventNames !== 'string') {
throw new TypeError('Event name(s) must be a string.');
if (typeof eventName !== 'string') {
throw new TypeError('Event name must be a string.');
}

if (typeof callback !== 'function') {
throw new TypeError('Callback, if provided, must be a function.');
}

for (const eventName of eventNames.trim().split(eventNamesRegex)) {
const list = this._getEventList(eventName, true);
if (Observable._indexOfListener(list, callback, thisArg) !== -1) {
// Already added.
continue;
}

list.push({
callback,
thisArg,
once,
});
const list = this._getEventList(eventName, true);
if (Observable._indexOfListener(list, callback, thisArg) !== -1) {
// Already added.
return;
}

list.push({
callback,
thisArg,
once,
});
}

/**
* Removes listener(s) for the specified event name.
* @param eventNames Comma delimited names of the events the specified listener is associated with.
* @param eventName Name of the event to attach to.
* @param callback An optional parameter pointing to a specific listener. If not defined, all listeners for the event names will be removed.
* @param thisArg An optional parameter which when set will be used to refine search of the correct callback which will be removed as event listener.
*/
public removeEventListener(eventNames: string, callback?: (data: EventData) => void, thisArg?: any): void {
public removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
thisArg = thisArg || undefined;

if (typeof eventNames !== 'string') {
if (typeof eventName !== 'string') {
throw new TypeError('Events name(s) must be string.');
}

if (callback && typeof callback !== 'function') {
throw new TypeError('callback must be function.');
}

for (const eventName of eventNames.trim().split(eventNamesRegex)) {
const entries = this._observers[eventName];
if (!entries) {
continue;
}
const entries = this._observers[eventName];
if (!entries) {
return;
}

Observable.innerRemoveEventListener(entries, callback, thisArg);
Observable.innerRemoveEventListener(entries, callback, thisArg);

if (!entries.length) {
// Clear all entries of this type
delete this._observers[eventName];
}
if (!entries.length) {
// Clear all entries of this type
delete this._observers[eventName];
}
}

Expand Down Expand Up @@ -297,11 +291,11 @@ export class Observable {
* in future.
* @deprecated
*/
public static removeEventListener(eventNames: string, callback?: (data: EventData) => void, thisArg?: any): void {
public static removeEventListener(eventName: string, callback?: (data: EventData) => void, thisArg?: any): void {
thisArg = thisArg || undefined;

if (typeof eventNames !== 'string') {
throw new TypeError('Event name(s) must be a string.');
if (typeof eventName !== 'string') {
throw new TypeError('Event name must be a string.');
}

if (callback && typeof callback !== 'function') {
Expand All @@ -310,24 +304,22 @@ export class Observable {

const eventClass = this.name === 'Observable' ? '*' : this.name;

for (const eventName of eventNames.trim().split(eventNamesRegex)) {
const entries = _globalEventHandlers?.[eventClass]?.[eventName];
if (!entries) {
continue;
}
const entries = _globalEventHandlers?.[eventClass]?.[eventName];
if (!entries) {
return;
}

Observable.innerRemoveEventListener(entries, callback, thisArg);
Observable.innerRemoveEventListener(entries, callback, thisArg);

if (!entries.length) {
// Clear all entries of this type
delete _globalEventHandlers[eventClass][eventName];
}
if (!entries.length) {
// Clear all entries of this type
delete _globalEventHandlers[eventClass][eventName];
}

// Clear the primary class grouping if no list are left
const keys = Object.keys(_globalEventHandlers[eventClass]);
if (keys.length === 0) {
delete _globalEventHandlers[eventClass];
}
// Clear the primary class grouping if no list are left
const keys = Object.keys(_globalEventHandlers[eventClass]);
if (keys.length === 0) {
delete _globalEventHandlers[eventClass];
}
}

Expand All @@ -336,12 +328,12 @@ export class Observable {
* in future.
* @deprecated
*/
public static addEventListener(eventNames: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
public static addEventListener(eventName: string, callback: (data: EventData) => void, thisArg?: any, once?: boolean): void {
once = once || undefined;
thisArg = thisArg || undefined;

if (typeof eventNames !== 'string') {
throw new TypeError('Event name(s) must be a string.');
if (typeof eventName !== 'string') {
throw new TypeError('Event name must be a string.');
}

if (typeof callback !== 'function') {
Expand All @@ -353,17 +345,15 @@ export class Observable {
_globalEventHandlers[eventClass] = {};
}

for (const eventName of eventNames.trim().split(eventNamesRegex)) {
if (!_globalEventHandlers[eventClass][eventName]) {
_globalEventHandlers[eventClass][eventName] = [];
}
if (Observable._indexOfListener(_globalEventHandlers[eventClass][eventName], callback, thisArg) !== -1) {
// Already added.
return;
}

_globalEventHandlers[eventClass][eventName].push({ callback, thisArg, once });
if (!_globalEventHandlers[eventClass][eventName]) {
_globalEventHandlers[eventClass][eventName] = [];
}
if (Observable._indexOfListener(_globalEventHandlers[eventClass][eventName], callback, thisArg) !== -1) {
// Already added.
return;
}

_globalEventHandlers[eventClass][eventName].push({ callback, thisArg, once });
}

private _globalNotify<T extends EventData>(eventClass: string, eventType: string, data: T): void {
Expand Down Expand Up @@ -464,10 +454,8 @@ export class Observable {
};
}

public _emit(eventNames: string): void {
for (const eventName of eventNames.trim().split(eventNamesRegex)) {
this.notify({ eventName, object: this });
}
public _emit(eventName: string): void {
this.notify({ eventName, object: this });
}

private _getEventList(eventName: string, createIfNeeded?: boolean): Array<ListenerEntry> | undefined {
Expand Down
29 changes: 25 additions & 4 deletions packages/core/ui/core/bindable/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ViewBase } from '../view-base';
// Requires
import { unsetValue } from '../properties';
import { Observable, PropertyChangeData } from '../../../data/observable';
import { fromString as gestureFromString } from '../../../ui/gestures/gestures-common';
import { addWeakEventListener, removeWeakEventListener } from '../weak-event-listener';
import { bindingConstants, parentsRegex } from '../../builder/binding-builder';
import { escapeRegexSymbols } from '../../../utils';
Expand Down Expand Up @@ -87,25 +88,45 @@ export interface ValueConverter {
toView: (...params: any[]) => any;
}

/**
* Normalizes "ontap" to "tap", and "ondoubletap" to "ondoubletap".
*
* Removes the leading "on" from an event gesture name, for example:
* - "ontap" -> "tap"
* - "ondoubletap" -> "doubletap"
* - "onTap" -> "Tap"
*
* Be warned that, as event/gesture names in NativeScript are case-sensitive,
* this may produce an invalid event/gesture name (i.e. "doubletap" would fail
* to match the "doubleTap" gesture name), and so it is up to the consumer to
* handle the output properly.
*/
export function getEventOrGestureName(name: string): string {
return name.indexOf('on') === 0 ? name.substr(2, name.length - 2) : name;
return name.indexOf('on') === 0 ? name.slice(2) : name;
}

// NOTE: method fromString from "ui/gestures";
export function isGesture(eventOrGestureName: string): boolean {
// Not sure whether this trimming and lowercasing is still needed in practice
// (all Core tests pass without it), so worth revisiting in future. I think
// this is used exclusively by the XML flavour, and my best guess is that
// maybe it's to handle how getEventOrGestureName("onTap") might pass "Tap"
// into this.
const t = eventOrGestureName.trim().toLowerCase();

// Would be nice to have a convenience function for getting all GestureState
// names in `gestures-common.ts`, but when I tried introducing it, it created
// a circular dependency that crashed the automated tests app.
return t === 'tap' || t === 'doubletap' || t === 'pinch' || t === 'pan' || t === 'swipe' || t === 'rotation' || t === 'longpress' || t === 'touch';
}

// TODO: Make this instance function so that we dont need public statc tapEvent = "tap"
// TODO: Make this instance function so that we dont need public static tapEvent = "tap"
// in controls. They will just override this one and provide their own event support.
export function isEventOrGesture(name: string, view: ViewBase): boolean {
if (typeof name === 'string') {
const eventOrGestureName = getEventOrGestureName(name);
const evt = `${eventOrGestureName}Event`;

return (view.constructor && evt in view.constructor) || isGesture(eventOrGestureName.toLowerCase());
return (view.constructor && evt in view.constructor) || isGesture(eventOrGestureName);
}

return false;
Expand Down
12 changes: 6 additions & 6 deletions packages/core/ui/core/view/view-common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,11 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {

// Coerce "tap" -> GestureTypes.tap
// Coerce "loaded" -> undefined
const gesture: GestureTypes | undefined = gestureFromString(normalizedName);
const gestureType: GestureTypes | undefined = gestureFromString(normalizedName);

// If it's a gesture (and this Observable declares e.g. `static tapEvent`)
if (gesture && !this._isEvent(normalizedName)) {
this._observe(gesture, callback, thisArg);
if (gestureType && !this._isEvent(normalizedName)) {
this._observe(gestureType, callback, thisArg);
return;
}

Expand All @@ -324,11 +324,11 @@ export abstract class ViewCommon extends ViewBase implements ViewDefinition {

// Coerce "tap" -> GestureTypes.tap
// Coerce "loaded" -> undefined
const gesture: GestureTypes | undefined = gestureFromString(normalizedName);
const gestureType: GestureTypes | undefined = gestureFromString(normalizedName);

// If it's a gesture (and this Observable declares e.g. `static tapEvent`)
if (gesture && !this._isEvent(normalizedName)) {
this._disconnectGestureObservers(gesture, callback, thisArg);
if (gestureType && !this._isEvent(normalizedName)) {
this._disconnectGestureObservers(gestureType, callback, thisArg);
return;
}

Expand Down

0 comments on commit 9be392f

Please sign in to comment.