Skip to content

Commit dfe7621

Browse files
Vladimir Amiorkovdtopuzov
Vladimir Amiorkov
authored and
dtopuzov
committed
fix: clear the resolvedPage when entry is being cleared, change the passed View to be a weak reference (#7327)
* fix: clear the `resolvedPage` when entry is being cleared fix: change the passed `View` to be a weak reference * chore: add trace logs when weak ref has been cleared but is continuing to be used chore: add condition to check if weak ref has not been cleared when it is being used * chore: refactor the way the `resolvedPage` is cleared * chore: add backward compatible property to avoid breaking changes * chore: refactor condition to check if WeakRef is not cleared chore: add tracelogs * chore: refactor condition to check if WeakRef is not cleared chore: add tracelogs * refactor: weakRef usages * chore: change the way WeakRef type check is done
1 parent 81e1f54 commit dfe7621

7 files changed

Lines changed: 239 additions & 51 deletions

File tree

‎tns-core-modules/ui/core/properties/properties.ts‎

Lines changed: 58 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,12 @@ import { ViewBase } from "../view-base";
44

55
// Types.
66
import { WrappedValue, PropertyChangeData } from "../../../data/observable";
7+
import {
8+
write as traceWrite,
9+
categories as traceCategories,
10+
messageType as traceMessageType,
11+
} from "../../../trace";
12+
713
import { Style } from "../../styling/style";
814

915
import { profile } from "../../../profiling";
@@ -125,7 +131,7 @@ export class Property<T extends ViewBase, U> implements TypedPropertyDescriptor<
125131
if (affectsLayout) {
126132
this.requestLayout();
127133
}
128-
134+
129135
if (reset) {
130136
delete this[key];
131137
if (valueChanged) {
@@ -466,6 +472,13 @@ export class CssProperty<T extends Style, U> implements definitions.CssProperty<
466472
const property = this;
467473

468474
function setLocalValue(this: T, newValue: U | string): void {
475+
const view = this.viewRef.get();
476+
if (!view) {
477+
traceWrite(`${newValue} not set to view because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
478+
479+
return;
480+
}
481+
469482
const reset = newValue === unsetValue || newValue === "";
470483
let value: U;
471484
if (reset) {
@@ -482,7 +495,6 @@ export class CssProperty<T extends Style, U> implements definitions.CssProperty<
482495
const changed: boolean = equalityComparer ? !equalityComparer(oldValue, value) : oldValue !== value;
483496

484497
if (changed) {
485-
const view = this.view;
486498
if (reset) {
487499
delete this[key];
488500
if (valueChanged) {
@@ -534,6 +546,13 @@ export class CssProperty<T extends Style, U> implements definitions.CssProperty<
534546
}
535547

536548
function setCssValue(this: T, newValue: U | string): void {
549+
const view = this.viewRef.get();
550+
if (!view) {
551+
traceWrite(`${newValue} not set to view because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
552+
553+
return;
554+
}
555+
537556
const currentValueSource: number = this[sourceKey] || ValueSource.Default;
538557

539558
// We have localValueSource - NOOP.
@@ -557,7 +576,6 @@ export class CssProperty<T extends Style, U> implements definitions.CssProperty<
557576
const changed: boolean = equalityComparer ? !equalityComparer(oldValue, value) : oldValue !== value;
558577

559578
if (changed) {
560-
const view = this.view;
561579
if (reset) {
562580
delete this[key];
563581
if (valueChanged) {
@@ -718,12 +736,18 @@ export class CssAnimationProperty<T extends Style, U> implements definitions.Css
718736
enumerable, configurable,
719737
get: getsComputed ? function (this: T) { return this[computedValue]; } : function (this: T) { return this[symbol]; },
720738
set(this: T, boxedValue: U | string) {
739+
const view = this.viewRef.get();
740+
if (!view) {
741+
traceWrite(`${boxedValue} not set to view because ".viewRef" is cleared`, traceCategories.Animation, traceMessageType.warn);
742+
743+
return;
744+
}
721745

722746
const oldValue = this[computedValue];
723747
const oldSource = this[computedSource];
724748
const wasSet = oldSource !== ValueSource.Default;
725749
const reset = boxedValue === unsetValue || boxedValue === "";
726-
750+
727751
if (reset) {
728752
this[symbol] = unsetValue;
729753
if (this[computedSource] === propertySource) {
@@ -760,7 +784,6 @@ export class CssAnimationProperty<T extends Style, U> implements definitions.Css
760784
valueChanged(this, oldValue, value);
761785
}
762786

763-
const view = this.view;
764787
if (view[setNative] && (computedValueChanged || isSet !== wasSet)) {
765788
if (view._suspendNativeUpdatesCount) {
766789
if (view._suspendedUpdates) {
@@ -816,10 +839,16 @@ export class CssAnimationProperty<T extends Style, U> implements definitions.Css
816839
}
817840

818841
public _initDefaultNativeValue(target: T): void {
842+
const view = target.viewRef.get();
843+
if (!view) {
844+
traceWrite(`_initDefaultNativeValue not executed to view because ".viewRef" is cleared`, traceCategories.Animation, traceMessageType.warn);
845+
846+
return;
847+
}
848+
819849
const defaultValueKey = this.defaultValueKey;
820850

821851
if (!(defaultValueKey in target)) {
822-
const view = target.view;
823852
const getDefault = this.getDefault;
824853
target[defaultValueKey] = view[getDefault] ? view[getDefault]() : this.defaultValue;
825854
}
@@ -862,6 +891,13 @@ export class InheritedCssProperty<T extends Style, U> extends CssProperty<T, U>
862891
const property = this;
863892

864893
const setFunc = (valueSource: ValueSource) => function (this: T, boxedValue: any): void {
894+
const view = this.viewRef.get();
895+
if (!view) {
896+
traceWrite(`${boxedValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
897+
898+
return;
899+
}
900+
865901
const reset = boxedValue === unsetValue || boxedValue === "";
866902
const currentValueSource: number = this[sourceKey] || ValueSource.Default;
867903
if (reset) {
@@ -876,7 +912,6 @@ export class InheritedCssProperty<T extends Style, U> extends CssProperty<T, U>
876912
}
877913

878914
const oldValue: U = key in this ? this[key] : defaultValue;
879-
const view = this.view;
880915
let value: U;
881916
let unsetNativeValue = false;
882917
if (reset) {
@@ -907,7 +942,6 @@ export class InheritedCssProperty<T extends Style, U> extends CssProperty<T, U>
907942
const changed: boolean = equalityComparer ? !equalityComparer(oldValue, value) : oldValue !== value;
908943

909944
if (changed) {
910-
const view = this.view;
911945
if (valueChanged) {
912946
valueChanged(this, oldValue, value);
913947
}
@@ -997,15 +1031,29 @@ export class ShorthandProperty<T extends Style, P> implements definitions.Shorth
9971031
const converter = options.converter;
9981032

9991033
function setLocalValue(this: T, value: string | P): void {
1000-
this.view._batchUpdate(() => {
1034+
const view = this.viewRef.get();
1035+
if (!view) {
1036+
traceWrite(`setLocalValue not executed to view because ".viewRef" is cleared`, traceCategories.Animation, traceMessageType.warn);
1037+
1038+
return;
1039+
}
1040+
1041+
view._batchUpdate(() => {
10011042
for (let [p, v] of converter(value)) {
10021043
this[p.name] = v;
10031044
}
10041045
});
10051046
}
10061047

10071048
function setCssValue(this: T, value: string): void {
1008-
this.view._batchUpdate(() => {
1049+
const view = this.viewRef.get();
1050+
if (!view) {
1051+
traceWrite(`setCssValue not executed to view because ".viewRef" is cleared`, traceCategories.Animation, traceMessageType.warn);
1052+
1053+
return;
1054+
}
1055+
1056+
view._batchUpdate(() => {
10091057
for (let [p, v] of converter(value)) {
10101058
this[p.cssName] = v;
10111059
}

‎tns-core-modules/ui/core/view-base/view-base.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ export abstract class ViewBase extends Observable implements ViewBaseDefinition
192192
public _domId: number;
193193
public _context: any;
194194
public _isAddedToNativeVisualTree: boolean;
195-
public _cssState: ssm.CssState = new ssm.CssState(this);
195+
public _cssState: ssm.CssState = new ssm.CssState(new WeakRef(this));
196196
public _styleScope: ssm.StyleScope;
197197
public _suspendedUpdates: { [propertyName: string]: Property<ViewBase, any> | CssProperty<Style, any> | CssAnimationProperty<Style, any> };
198198
public _suspendNativeUpdatesCount: SuspendType;
@@ -249,7 +249,7 @@ export abstract class ViewBase extends Observable implements ViewBaseDefinition
249249
constructor() {
250250
super();
251251
this._domId = viewIdCounter++;
252-
this._style = new Style(this);
252+
this._style = new Style(new WeakRef(this));
253253
}
254254

255255
// Used in Angular.

‎tns-core-modules/ui/frame/frame-common.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,8 @@ export class FrameBase extends CustomLayoutView implements FrameDefinition {
154154
} else {
155155
page._tearDownUI(true);
156156
}
157+
158+
removed.resolvedPage = null;
157159
}
158160

159161
// Attempts to implement https://github.com/NativeScript/NativeScript/issues/1311

‎tns-core-modules/ui/styling/style-properties.ts‎

Lines changed: 71 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ import {
2525
matrixArrayToCssMatrix,
2626
multiplyAffine2d,
2727
} from "../../matrix";
28+
import {
29+
write as traceWrite,
30+
categories as traceCategories,
31+
messageType as traceMessageType,
32+
} from "../../trace";
2833

2934
import * as parser from "../../css/parser";
3035
import { LinearGradient } from "./linear-gradient";
@@ -175,15 +180,25 @@ export const zeroLength: Length = { value: 0, unit: "px" };
175180
export const minWidthProperty = new CssProperty<Style, Length>({
176181
name: "minWidth", cssName: "min-width", defaultValue: zeroLength, affectsLayout: isIOS, equalityComparer: Length.equals,
177182
valueChanged: (target, oldValue, newValue) => {
178-
target.view.effectiveMinWidth = Length.toDevicePixels(newValue, 0);
183+
const view = target.viewRef.get();
184+
if (view) {
185+
view.effectiveMinWidth = Length.toDevicePixels(newValue, 0);
186+
} else {
187+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
188+
}
179189
}, valueConverter: Length.parse
180190
});
181191
minWidthProperty.register(Style);
182192

183193
export const minHeightProperty = new CssProperty<Style, Length>({
184194
name: "minHeight", cssName: "min-height", defaultValue: zeroLength, affectsLayout: isIOS, equalityComparer: Length.equals,
185195
valueChanged: (target, oldValue, newValue) => {
186-
target.view.effectiveMinHeight = Length.toDevicePixels(newValue, 0);
196+
const view = target.viewRef.get();
197+
if (view) {
198+
view.effectiveMinHeight = Length.toDevicePixels(newValue, 0);
199+
} else {
200+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
201+
}
187202
}, valueConverter: Length.parse
188203
});
189204
minHeightProperty.register(Style);
@@ -237,31 +252,51 @@ paddingProperty.register(Style);
237252
export const paddingLeftProperty = new CssProperty<Style, Length>({
238253
name: "paddingLeft", cssName: "padding-left", defaultValue: zeroLength, affectsLayout: isIOS, equalityComparer: Length.equals,
239254
valueChanged: (target, oldValue, newValue) => {
240-
target.view.effectivePaddingLeft = Length.toDevicePixels(newValue, 0);
255+
const view = target.viewRef.get();
256+
if (view) {
257+
view.effectivePaddingLeft = Length.toDevicePixels(newValue, 0);
258+
} else {
259+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
260+
}
241261
}, valueConverter: Length.parse
242262
});
243263
paddingLeftProperty.register(Style);
244264

245265
export const paddingRightProperty = new CssProperty<Style, Length>({
246266
name: "paddingRight", cssName: "padding-right", defaultValue: zeroLength, affectsLayout: isIOS, equalityComparer: Length.equals,
247267
valueChanged: (target, oldValue, newValue) => {
248-
target.view.effectivePaddingRight = Length.toDevicePixels(newValue, 0);
268+
const view = target.viewRef.get();
269+
if (view) {
270+
view.effectivePaddingRight = Length.toDevicePixels(newValue, 0);
271+
} else {
272+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
273+
}
249274
}, valueConverter: Length.parse
250275
});
251276
paddingRightProperty.register(Style);
252277

253278
export const paddingTopProperty = new CssProperty<Style, Length>({
254279
name: "paddingTop", cssName: "padding-top", defaultValue: zeroLength, affectsLayout: isIOS, equalityComparer: Length.equals,
255280
valueChanged: (target, oldValue, newValue) => {
256-
target.view.effectivePaddingTop = Length.toDevicePixels(newValue, 0);
281+
const view = target.viewRef.get();
282+
if (view) {
283+
view.effectivePaddingTop = Length.toDevicePixels(newValue, 0);
284+
} else {
285+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
286+
}
257287
}, valueConverter: Length.parse
258288
});
259289
paddingTopProperty.register(Style);
260290

261291
export const paddingBottomProperty = new CssProperty<Style, Length>({
262292
name: "paddingBottom", cssName: "padding-bottom", defaultValue: zeroLength, affectsLayout: isIOS, equalityComparer: Length.equals,
263293
valueChanged: (target, oldValue, newValue) => {
264-
target.view.effectivePaddingBottom = Length.toDevicePixels(newValue, 0);
294+
const view = target.viewRef.get();
295+
if (view) {
296+
view.effectivePaddingBottom = Length.toDevicePixels(newValue, 0);
297+
} else {
298+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
299+
}
265300
}, valueConverter: Length.parse
266301
});
267302
paddingBottomProperty.register(Style);
@@ -822,7 +857,12 @@ export const borderTopWidthProperty = new CssProperty<Style, Length>({
822857
throw new Error(`border-top-width should be Non-Negative Finite number. Value: ${value}`);
823858
}
824859

825-
target.view.effectiveBorderTopWidth = value;
860+
const view = target.viewRef.get();
861+
if (view) {
862+
view.effectiveBorderTopWidth = value;
863+
} else {
864+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
865+
}
826866
const background = target.backgroundInternal.withBorderTopWidth(value);
827867
target.backgroundInternal = background;
828868
}, valueConverter: Length.parse
@@ -837,7 +877,12 @@ export const borderRightWidthProperty = new CssProperty<Style, Length>({
837877
throw new Error(`border-right-width should be Non-Negative Finite number. Value: ${value}`);
838878
}
839879

840-
target.view.effectiveBorderRightWidth = value;
880+
const view = target.viewRef.get();
881+
if (view) {
882+
view.effectiveBorderRightWidth = value;
883+
} else {
884+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
885+
}
841886
const background = target.backgroundInternal.withBorderRightWidth(value);
842887
target.backgroundInternal = background;
843888
}, valueConverter: Length.parse
@@ -852,7 +897,12 @@ export const borderBottomWidthProperty = new CssProperty<Style, Length>({
852897
throw new Error(`border-bottom-width should be Non-Negative Finite number. Value: ${value}`);
853898
}
854899

855-
target.view.effectiveBorderBottomWidth = value;
900+
const view = target.viewRef.get();
901+
if (view) {
902+
view.effectiveBorderBottomWidth = value;
903+
} else {
904+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
905+
}
856906
const background = target.backgroundInternal.withBorderBottomWidth(value);
857907
target.backgroundInternal = background;
858908
}, valueConverter: Length.parse
@@ -867,7 +917,12 @@ export const borderLeftWidthProperty = new CssProperty<Style, Length>({
867917
throw new Error(`border-left-width should be Non-Negative Finite number. Value: ${value}`);
868918
}
869919

870-
target.view.effectiveBorderLeftWidth = value;
920+
const view = target.viewRef.get();
921+
if (view) {
922+
view.effectiveBorderLeftWidth = value;
923+
} else {
924+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
925+
}
871926
const background = target.backgroundInternal.withBorderLeftWidth(value);
872927
target.backgroundInternal = background;
873928
}, valueConverter: Length.parse
@@ -1095,7 +1150,12 @@ export namespace Visibility {
10951150

10961151
export const visibilityProperty = new CssProperty<Style, Visibility>({
10971152
name: "visibility", cssName: "visibility", defaultValue: Visibility.VISIBLE, affectsLayout: isIOS, valueConverter: Visibility.parse, valueChanged: (target, oldValue, newValue) => {
1098-
target.view.isCollapsed = (newValue === Visibility.COLLAPSE);
1153+
const view = target.viewRef.get();
1154+
if (view) {
1155+
view.isCollapsed = (newValue === Visibility.COLLAPSE);
1156+
} else {
1157+
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
1158+
}
10991159
}
11001160
});
11011161
visibilityProperty.register(Style);

0 commit comments

Comments
 (0)