Skip to content

Commit

Permalink
fix: clear the resolvedPage when entry is being cleared, change the…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
@dtopuzov
Vladimir Amiorkov authored and dtopuzov committed Jun 21, 2019
1 parent 81e1f54 commit dfe7621
Show file tree
Hide file tree
Showing 7 changed files with 239 additions and 51 deletions.
68 changes: 58 additions & 10 deletions tns-core-modules/ui/core/properties/properties.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ import { ViewBase } from "../view-base";

// Types.
import { WrappedValue, PropertyChangeData } from "../../../data/observable";
import {
write as traceWrite,
categories as traceCategories,
messageType as traceMessageType,
} from "../../../trace";

import { Style } from "../../styling/style";

import { profile } from "../../../profiling";
Expand Down Expand Up @@ -125,7 +131,7 @@ export class Property<T extends ViewBase, U> implements TypedPropertyDescriptor<
if (affectsLayout) {
this.requestLayout();
}

if (reset) {
delete this[key];
if (valueChanged) {
Expand Down Expand Up @@ -466,6 +472,13 @@ export class CssProperty<T extends Style, U> implements definitions.CssProperty<
const property = this;

function setLocalValue(this: T, newValue: U | string): void {
const view = this.viewRef.get();
if (!view) {
traceWrite(`${newValue} not set to view because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);

return;
}

const reset = newValue === unsetValue || newValue === "";
let value: U;
if (reset) {
Expand All @@ -482,7 +495,6 @@ export class CssProperty<T extends Style, U> implements definitions.CssProperty<
const changed: boolean = equalityComparer ? !equalityComparer(oldValue, value) : oldValue !== value;

if (changed) {
const view = this.view;
if (reset) {
delete this[key];
if (valueChanged) {
Expand Down Expand Up @@ -534,6 +546,13 @@ export class CssProperty<T extends Style, U> implements definitions.CssProperty<
}

function setCssValue(this: T, newValue: U | string): void {
const view = this.viewRef.get();
if (!view) {
traceWrite(`${newValue} not set to view because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);

return;
}

const currentValueSource: number = this[sourceKey] || ValueSource.Default;

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

if (changed) {
const view = this.view;
if (reset) {
delete this[key];
if (valueChanged) {
Expand Down Expand Up @@ -718,12 +736,18 @@ export class CssAnimationProperty<T extends Style, U> implements definitions.Css
enumerable, configurable,
get: getsComputed ? function (this: T) { return this[computedValue]; } : function (this: T) { return this[symbol]; },
set(this: T, boxedValue: U | string) {
const view = this.viewRef.get();
if (!view) {
traceWrite(`${boxedValue} not set to view because ".viewRef" is cleared`, traceCategories.Animation, traceMessageType.warn);

return;
}

const oldValue = this[computedValue];
const oldSource = this[computedSource];
const wasSet = oldSource !== ValueSource.Default;
const reset = boxedValue === unsetValue || boxedValue === "";

if (reset) {
this[symbol] = unsetValue;
if (this[computedSource] === propertySource) {
Expand Down Expand Up @@ -760,7 +784,6 @@ export class CssAnimationProperty<T extends Style, U> implements definitions.Css
valueChanged(this, oldValue, value);
}

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

public _initDefaultNativeValue(target: T): void {
const view = target.viewRef.get();
if (!view) {
traceWrite(`_initDefaultNativeValue not executed to view because ".viewRef" is cleared`, traceCategories.Animation, traceMessageType.warn);

return;
}

const defaultValueKey = this.defaultValueKey;

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

const setFunc = (valueSource: ValueSource) => function (this: T, boxedValue: any): void {
const view = this.viewRef.get();
if (!view) {
traceWrite(`${boxedValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);

return;
}

const reset = boxedValue === unsetValue || boxedValue === "";
const currentValueSource: number = this[sourceKey] || ValueSource.Default;
if (reset) {
Expand All @@ -876,7 +912,6 @@ export class InheritedCssProperty<T extends Style, U> extends CssProperty<T, U>
}

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

if (changed) {
const view = this.view;
if (valueChanged) {
valueChanged(this, oldValue, value);
}
Expand Down Expand Up @@ -997,15 +1031,29 @@ export class ShorthandProperty<T extends Style, P> implements definitions.Shorth
const converter = options.converter;

function setLocalValue(this: T, value: string | P): void {
this.view._batchUpdate(() => {
const view = this.viewRef.get();
if (!view) {
traceWrite(`setLocalValue not executed to view because ".viewRef" is cleared`, traceCategories.Animation, traceMessageType.warn);

return;
}

view._batchUpdate(() => {
for (let [p, v] of converter(value)) {
this[p.name] = v;
}
});
}

function setCssValue(this: T, value: string): void {
this.view._batchUpdate(() => {
const view = this.viewRef.get();
if (!view) {
traceWrite(`setCssValue not executed to view because ".viewRef" is cleared`, traceCategories.Animation, traceMessageType.warn);

return;
}

view._batchUpdate(() => {
for (let [p, v] of converter(value)) {
this[p.cssName] = v;
}
Expand Down
4 changes: 2 additions & 2 deletions tns-core-modules/ui/core/view-base/view-base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export abstract class ViewBase extends Observable implements ViewBaseDefinition
public _domId: number;
public _context: any;
public _isAddedToNativeVisualTree: boolean;
public _cssState: ssm.CssState = new ssm.CssState(this);
public _cssState: ssm.CssState = new ssm.CssState(new WeakRef(this));
public _styleScope: ssm.StyleScope;
public _suspendedUpdates: { [propertyName: string]: Property<ViewBase, any> | CssProperty<Style, any> | CssAnimationProperty<Style, any> };
public _suspendNativeUpdatesCount: SuspendType;
Expand Down Expand Up @@ -249,7 +249,7 @@ export abstract class ViewBase extends Observable implements ViewBaseDefinition
constructor() {
super();
this._domId = viewIdCounter++;
this._style = new Style(this);
this._style = new Style(new WeakRef(this));
}

// Used in Angular.
Expand Down
2 changes: 2 additions & 0 deletions tns-core-modules/ui/frame/frame-common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ export class FrameBase extends CustomLayoutView implements FrameDefinition {
} else {
page._tearDownUI(true);
}

removed.resolvedPage = null;
}

// Attempts to implement https://github.com/NativeScript/NativeScript/issues/1311
Expand Down
82 changes: 71 additions & 11 deletions tns-core-modules/ui/styling/style-properties.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ import {
matrixArrayToCssMatrix,
multiplyAffine2d,
} from "../../matrix";
import {
write as traceWrite,
categories as traceCategories,
messageType as traceMessageType,
} from "../../trace";

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

export const minHeightProperty = new CssProperty<Style, Length>({
name: "minHeight", cssName: "min-height", defaultValue: zeroLength, affectsLayout: isIOS, equalityComparer: Length.equals,
valueChanged: (target, oldValue, newValue) => {
target.view.effectiveMinHeight = Length.toDevicePixels(newValue, 0);
const view = target.viewRef.get();
if (view) {
view.effectiveMinHeight = Length.toDevicePixels(newValue, 0);
} else {
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
}
}, valueConverter: Length.parse
});
minHeightProperty.register(Style);
Expand Down Expand Up @@ -237,31 +252,51 @@ paddingProperty.register(Style);
export const paddingLeftProperty = new CssProperty<Style, Length>({
name: "paddingLeft", cssName: "padding-left", defaultValue: zeroLength, affectsLayout: isIOS, equalityComparer: Length.equals,
valueChanged: (target, oldValue, newValue) => {
target.view.effectivePaddingLeft = Length.toDevicePixels(newValue, 0);
const view = target.viewRef.get();
if (view) {
view.effectivePaddingLeft = Length.toDevicePixels(newValue, 0);
} else {
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
}
}, valueConverter: Length.parse
});
paddingLeftProperty.register(Style);

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

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

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

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

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

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

target.view.effectiveBorderLeftWidth = value;
const view = target.viewRef.get();
if (view) {
view.effectiveBorderLeftWidth = value;
} else {
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
}
const background = target.backgroundInternal.withBorderLeftWidth(value);
target.backgroundInternal = background;
}, valueConverter: Length.parse
Expand Down Expand Up @@ -1095,7 +1150,12 @@ export namespace Visibility {

export const visibilityProperty = new CssProperty<Style, Visibility>({
name: "visibility", cssName: "visibility", defaultValue: Visibility.VISIBLE, affectsLayout: isIOS, valueConverter: Visibility.parse, valueChanged: (target, oldValue, newValue) => {
target.view.isCollapsed = (newValue === Visibility.COLLAPSE);
const view = target.viewRef.get();
if (view) {
view.isCollapsed = (newValue === Visibility.COLLAPSE);
} else {
traceWrite(`${newValue} not set to view's property because ".viewRef" is cleared`, traceCategories.Style, traceMessageType.warn);
}
}
});
visibilityProperty.register(Style);
Expand Down

0 comments on commit dfe7621

Please sign in to comment.