From fc42102ba6bb55da380152d2cc061f713e6dc450 Mon Sep 17 00:00:00 2001 From: James Hollyer Date: Mon, 10 Jan 2022 17:25:05 -0800 Subject: [PATCH 1/9] Add WebGL escape hatch --- tensorboard/webapp/app_routing/app_root.ts | 9 +++++++++ .../widgets/line_chart_v2/line_chart_internal_utils.ts | 4 ++++ 2 files changed, 13 insertions(+) diff --git a/tensorboard/webapp/app_routing/app_root.ts b/tensorboard/webapp/app_routing/app_root.ts index b33a63e5aef..354ae50747c 100644 --- a/tensorboard/webapp/app_routing/app_root.ts +++ b/tensorboard/webapp/app_routing/app_root.ts @@ -21,6 +21,15 @@ export class AppRootProvider { constructor(location: Location) { this.appRoot = this.getAppRootFromMetaElement(location); + + // These secret query parameters are here to allow users having trouble with + // our WebGL renderer to force the use of our SVG renderer. + if (window.location.href.includes('_ForceSVG')) { + window.localStorage.setItem('forcedRenderer', 'SVG'); + } + if (window.location.href.includes('_DefaultRenderer')) { + window.localStorage.removeItem('forcedRenderer'); + } } /** diff --git a/tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils.ts b/tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils.ts index b7ac4fa84f9..2294203487f 100644 --- a/tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils.ts +++ b/tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils.ts @@ -75,6 +75,10 @@ export function computeDataSeriesExtent( export function getRendererType( preferredRendererType: RendererType ): RendererType { + if (window.localStorage.getItem('forcedRenderer') == 'SVG') { + return RendererType.SVG; + } + switch (preferredRendererType) { case RendererType.SVG: return RendererType.SVG; From 31a537080b6963cc324b15a24a9fb636ed39dc21 Mon Sep 17 00:00:00 2001 From: James Hollyer Date: Tue, 18 Jan 2022 12:20:01 -0800 Subject: [PATCH 2/9] mock getForceSvgFeatureFlag in all scalar card tests. --- tensorboard/webapp/app_routing/app_root.ts | 9 --------- tensorboard/webapp/feature_flag/effects/BUILD | 1 + .../webapp/feature_flag/effects/feature_flag_effects.ts | 5 ++++- .../webapp/feature_flag/store/feature_flag_selectors.ts | 7 +++++++ .../store/feature_flag_store_config_provider.ts | 1 + tensorboard/webapp/feature_flag/types.ts | 2 ++ tensorboard/webapp/metrics/views/card_renderer/BUILD | 1 + .../views/card_renderer/scalar_card_component.ng.html | 2 +- .../metrics/views/card_renderer/scalar_card_component.ts | 1 + .../metrics/views/card_renderer/scalar_card_container.ts | 3 +++ tensorboard/webapp/webapp_data_source/BUILD | 1 + .../tb_feature_flag_data_source_types.ts | 2 ++ .../webapp/webapp_data_source/tb_feature_flag_module.ts | 2 ++ .../widgets/line_chart_v2/line_chart_internal_utils.ts | 4 ---- 14 files changed, 26 insertions(+), 15 deletions(-) diff --git a/tensorboard/webapp/app_routing/app_root.ts b/tensorboard/webapp/app_routing/app_root.ts index 354ae50747c..b33a63e5aef 100644 --- a/tensorboard/webapp/app_routing/app_root.ts +++ b/tensorboard/webapp/app_routing/app_root.ts @@ -21,15 +21,6 @@ export class AppRootProvider { constructor(location: Location) { this.appRoot = this.getAppRootFromMetaElement(location); - - // These secret query parameters are here to allow users having trouble with - // our WebGL renderer to force the use of our SVG renderer. - if (window.location.href.includes('_ForceSVG')) { - window.localStorage.setItem('forcedRenderer', 'SVG'); - } - if (window.location.href.includes('_DefaultRenderer')) { - window.localStorage.removeItem('forcedRenderer'); - } } /** diff --git a/tensorboard/webapp/feature_flag/effects/BUILD b/tensorboard/webapp/feature_flag/effects/BUILD index 8fe18308001..9e8006188a5 100644 --- a/tensorboard/webapp/feature_flag/effects/BUILD +++ b/tensorboard/webapp/feature_flag/effects/BUILD @@ -11,6 +11,7 @@ tf_ng_module( "//tensorboard/webapp/feature_flag/actions", "//tensorboard/webapp/feature_flag/store", "//tensorboard/webapp/feature_flag/store:types", + "//tensorboard/webapp/webapp_data_source:feature_flag", "//tensorboard/webapp/webapp_data_source:feature_flag_types", "@npm//@angular/core", "@npm//@ngrx/effects", diff --git a/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts b/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts index 226c6291583..b3ddbe15fb3 100644 --- a/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts +++ b/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts @@ -17,6 +17,7 @@ import {Injectable} from '@angular/core'; import {Actions, createEffect, ofType} from '@ngrx/effects'; import {Action, createAction, Store} from '@ngrx/store'; import {combineLatestWith, map} from 'rxjs/operators'; +import {ForceSVGDataSource} from '../../webapp_data_source/force_SVG_data_source'; import {TBFeatureFlagDataSource} from '../../webapp_data_source/tb_feature_flag_data_source_types'; import {partialFeatureFlagsLoaded} from '../actions/feature_flag_actions'; import {getIsAutoDarkModeAllowed} from '../store/feature_flag_selectors'; @@ -33,6 +34,7 @@ export class FeatureFlagEffects { combineLatestWith(this.store.select(getIsAutoDarkModeAllowed)), map(([, isDarkModeAllowed]) => { const features = this.dataSource.getFeatures(isDarkModeAllowed); + features.forceSVG = this.ForceSVGDataSource.getAndUpdateForceSVGFlag(); return partialFeatureFlagsLoaded({features}); }) ) @@ -41,7 +43,8 @@ export class FeatureFlagEffects { constructor( private readonly actions$: Actions, private readonly store: Store, - private readonly dataSource: TBFeatureFlagDataSource + private readonly dataSource: TBFeatureFlagDataSource, + private readonly ForceSVGDataSource: ForceSVGDataSource ) {} /** @export */ diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts b/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts index 9b2ec299a0a..e1080a18387 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts @@ -129,3 +129,10 @@ export const getEnabledTimeNamespacedState = createSelector( return flags.enabledTimeNamespacedState; } ); + +export const getForceSVGFeatureFlag = createSelector( + getFeatureFlags, + (flags: FeatureFlags): boolean => { + return flags.forceSVG; + } +); diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts b/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts index 9818fa28f2b..78c766b1261 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts @@ -32,6 +32,7 @@ export const initialState: FeatureFlagState = { enableTimeSeriesPromotion: false, enabledCardWidthSetting: true, enabledTimeNamespacedState: false, + forceSVG: false, }, flagOverrides: {}, }; diff --git a/tensorboard/webapp/feature_flag/types.ts b/tensorboard/webapp/feature_flag/types.ts index 208643055c0..10ddc7098b6 100644 --- a/tensorboard/webapp/feature_flag/types.ts +++ b/tensorboard/webapp/feature_flag/types.ts @@ -50,4 +50,6 @@ export interface FeatureFlags { // Whether to enable time-namespaced state and how it impacts how user // settings are kept during navigation. enabledTimeNamespacedState: boolean; + + forceSVG: boolean; } diff --git a/tensorboard/webapp/metrics/views/card_renderer/BUILD b/tensorboard/webapp/metrics/views/card_renderer/BUILD index cedcd19c4f4..81c8199979e 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/BUILD +++ b/tensorboard/webapp/metrics/views/card_renderer/BUILD @@ -268,6 +268,7 @@ tf_ng_module( "//tensorboard/webapp/angular:expect_angular_material_menu", "//tensorboard/webapp/angular:expect_angular_material_progress_spinner", "//tensorboard/webapp/experiments:types", + "//tensorboard/webapp/feature_flag/store", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/metrics/data_source", "//tensorboard/webapp/metrics/store", diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html index 8b1c6d89035..b2b78693d48 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html @@ -102,7 +102,7 @@ > { @Input() xAxisType!: XAxisType; @Input() xScaleType!: ScaleType; @Input() useDarkMode!: boolean; + @Input() forceSVG!: boolean; @Input() selectedTime!: ViewSelectedTime | null; @Output() onFullSizeToggle = new EventEmitter(); diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index a88676a9f0b..07f646b2a27 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -37,6 +37,7 @@ import { } from 'rxjs/operators'; import {State} from '../../../app_state'; import {ExperimentAlias} from '../../../experiments/types'; +import {getForceSVGFeatureFlag} from '../../../feature_flag/store/feature_flag_selectors'; import { getCardPinnedState, getCurrentRouteRunSelection, @@ -127,6 +128,7 @@ function areSeriesEqual( [xScaleType]="xScaleType$ | async" [useDarkMode]="useDarkMode$ | async" [selectedTime]="selectedTime$ | async" + [forceSVG]="forceSVG$ | async" (onFullSizeToggle)="onFullSizeToggle()" (onPinClicked)="pinStateChanged.emit($event)" observeIntersection @@ -174,6 +176,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { readonly ignoreOutliers$ = this.store.select(getMetricsIgnoreOutliers); readonly tooltipSort$ = this.store.select(getMetricsTooltipSort); readonly xAxisType$ = this.store.select(getMetricsXAxisType); + readonly forceSVG$ = this.store.select(getForceSVGFeatureFlag); readonly xScaleType$ = this.store.select(getMetricsXAxisType).pipe( map((xAxisType) => { switch (xAxisType) { diff --git a/tensorboard/webapp/webapp_data_source/BUILD b/tensorboard/webapp/webapp_data_source/BUILD index 05a29b639f9..5b82c0179a8 100644 --- a/tensorboard/webapp/webapp_data_source/BUILD +++ b/tensorboard/webapp/webapp_data_source/BUILD @@ -91,6 +91,7 @@ tf_ng_module( tf_ng_module( name = "feature_flag", srcs = [ + "force_SVG_data_source.ts", "query_params.ts", "tb_feature_flag_data_source.ts", "tb_feature_flag_module.ts", diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_types.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_types.ts index e6db8b43058..b76ed94e5e3 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_types.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_types.ts @@ -46,3 +46,5 @@ export const ENABLE_DARK_MODE_QUERY_PARAM_KEY = 'darkMode'; export const ENABLE_LINK_TIME_PARAM_KEY = 'enableLinkTime'; export const ENABLE_TIME_NAMESPACED_STATE = 'enableTimeNamespacedState'; + +export const FORCE_SVG_RENDERER = 'forceSVG'; diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_module.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_module.ts index 91d3e8d0657..301cd69ffc2 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_module.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_module.ts @@ -14,6 +14,7 @@ limitations under the License. ==============================================================================*/ import {NgModule} from '@angular/core'; +import {ForceSVGDataSource} from './force_SVG_data_source'; import {QueryParams} from './query_params'; import {QueryParamsFeatureFlagDataSource} from './tb_feature_flag_data_source'; import {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; @@ -22,6 +23,7 @@ import {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; providers: [ // Provide as injectable for other app-level implementations of // TBFeatureFlagDataSource. + ForceSVGDataSource, QueryParamsFeatureFlagDataSource, QueryParams, // Provide as the TBFeatureFlagDataSource implementation for the OSS app. diff --git a/tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils.ts b/tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils.ts index 2294203487f..b7ac4fa84f9 100644 --- a/tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils.ts +++ b/tensorboard/webapp/widgets/line_chart_v2/line_chart_internal_utils.ts @@ -75,10 +75,6 @@ export function computeDataSeriesExtent( export function getRendererType( preferredRendererType: RendererType ): RendererType { - if (window.localStorage.getItem('forcedRenderer') == 'SVG') { - return RendererType.SVG; - } - switch (preferredRendererType) { case RendererType.SVG: return RendererType.SVG; From 4f9dec7f20b5371d1a2a18d0e66e57f7ea01eba6 Mon Sep 17 00:00:00 2001 From: James Hollyer Date: Wed, 19 Jan 2022 08:47:15 -0800 Subject: [PATCH 3/9] add forceSVGDataSource file --- .../force_SVG_data_source.ts | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts diff --git a/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts b/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts new file mode 100644 index 00000000000..f700d991c41 --- /dev/null +++ b/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts @@ -0,0 +1,43 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ +import {Injectable} from '@angular/core'; +import {QueryParams} from './query_params'; +import {FORCE_SVG_RENDERER} from './tb_feature_flag_data_source_types'; + +@Injectable() +export class ForceSVGDataSource { + constructor(readonly queryParams: QueryParams) {} + + getAndUpdateForceSVGFlag(): boolean { + this.updateLocalStorage(); + if (localStorage.getItem(FORCE_SVG_RENDERER)) { + return true; + } + return false; + } + + private updateLocalStorage() { + const params = this.queryParams.getParams(); + console.log('checking force param'); + if (params.has(FORCE_SVG_RENDERER)) { + console.log('has query param'); + if (params.get(FORCE_SVG_RENDERER) !== 'false') { + localStorage.setItem(FORCE_SVG_RENDERER, 'true'); + } else { + localStorage.removeItem(FORCE_SVG_RENDERER); + } + } + } +} From 7c57f7835921c48410699770f3ff402d0bb36f6e Mon Sep 17 00:00:00 2001 From: James Hollyer Date: Wed, 19 Jan 2022 08:57:07 -0800 Subject: [PATCH 4/9] clean up forceSVGDataSource --- .../webapp/webapp_data_source/force_SVG_data_source.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts b/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts index f700d991c41..0ad06ec3b5d 100644 --- a/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts +++ b/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts @@ -1,4 +1,4 @@ -/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. +/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -30,9 +30,7 @@ export class ForceSVGDataSource { private updateLocalStorage() { const params = this.queryParams.getParams(); - console.log('checking force param'); if (params.has(FORCE_SVG_RENDERER)) { - console.log('has query param'); if (params.get(FORCE_SVG_RENDERER) !== 'false') { localStorage.setItem(FORCE_SVG_RENDERER, 'true'); } else { From 7e647debe0e6b26579857f2fae47d9da5065b1f0 Mon Sep 17 00:00:00 2001 From: James Hollyer Date: Wed, 19 Jan 2022 15:26:06 -0800 Subject: [PATCH 5/9] Code Review 1 --- tensorboard/webapp/feature_flag/BUILD | 11 +++++ tensorboard/webapp/feature_flag/effects/BUILD | 1 + .../effects/feature_flag_effects.ts | 13 ++++-- .../webapp/feature_flag/force_svg_util.ts | 30 ++++++++++++++ .../store/feature_flag_selectors.ts | 4 +- .../feature_flag_store_config_provider.ts | 2 +- tensorboard/webapp/feature_flag/types.ts | 2 +- .../scalar_card_component.ng.html | 2 +- .../card_renderer/scalar_card_component.ts | 2 +- .../card_renderer/scalar_card_container.ts | 6 +-- tensorboard/webapp/webapp_data_source/BUILD | 1 - .../force_SVG_data_source.ts | 41 ------------------- .../tb_feature_flag_data_source.ts | 5 +++ .../tb_feature_flag_module.ts | 2 - 14 files changed, 65 insertions(+), 57 deletions(-) create mode 100644 tensorboard/webapp/feature_flag/force_svg_util.ts delete mode 100644 tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts diff --git a/tensorboard/webapp/feature_flag/BUILD b/tensorboard/webapp/feature_flag/BUILD index 77ce87c6395..2830c3d6723 100644 --- a/tensorboard/webapp/feature_flag/BUILD +++ b/tensorboard/webapp/feature_flag/BUILD @@ -19,6 +19,17 @@ tf_ng_module( ], ) +tf_ts_library( + name = "force_svg_util", + srcs = [ + "force_svg_util.ts", + ], + deps = [ + "//tensorboard/webapp/webapp_data_source:feature_flag_types", + "@npm//@angular/core", + ], +) + tf_ts_library( name = "types", srcs = [ diff --git a/tensorboard/webapp/feature_flag/effects/BUILD b/tensorboard/webapp/feature_flag/effects/BUILD index 9e8006188a5..34c1b421d5d 100644 --- a/tensorboard/webapp/feature_flag/effects/BUILD +++ b/tensorboard/webapp/feature_flag/effects/BUILD @@ -8,6 +8,7 @@ tf_ng_module( "feature_flag_effects.ts", ], deps = [ + "//tensorboard/webapp/feature_flag:force_svg_util", "//tensorboard/webapp/feature_flag/actions", "//tensorboard/webapp/feature_flag/store", "//tensorboard/webapp/feature_flag/store:types", diff --git a/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts b/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts index b3ddbe15fb3..422e3e2f273 100644 --- a/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts +++ b/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts @@ -17,9 +17,9 @@ import {Injectable} from '@angular/core'; import {Actions, createEffect, ofType} from '@ngrx/effects'; import {Action, createAction, Store} from '@ngrx/store'; import {combineLatestWith, map} from 'rxjs/operators'; -import {ForceSVGDataSource} from '../../webapp_data_source/force_SVG_data_source'; import {TBFeatureFlagDataSource} from '../../webapp_data_source/tb_feature_flag_data_source_types'; import {partialFeatureFlagsLoaded} from '../actions/feature_flag_actions'; +import {getForceSvgFlag, updateForceSvg} from '../force_svg_util'; import {getIsAutoDarkModeAllowed} from '../store/feature_flag_selectors'; import {State} from '../store/feature_flag_types'; @@ -34,7 +34,13 @@ export class FeatureFlagEffects { combineLatestWith(this.store.select(getIsAutoDarkModeAllowed)), map(([, isDarkModeAllowed]) => { const features = this.dataSource.getFeatures(isDarkModeAllowed); - features.forceSVG = this.ForceSVGDataSource.getAndUpdateForceSVGFlag(); + + if (features.forceSvg != null) { + updateForceSvg(features.forceSvg); + } else { + features.forceSvg = getForceSvgFlag(); + } + return partialFeatureFlagsLoaded({features}); }) ) @@ -43,8 +49,7 @@ export class FeatureFlagEffects { constructor( private readonly actions$: Actions, private readonly store: Store, - private readonly dataSource: TBFeatureFlagDataSource, - private readonly ForceSVGDataSource: ForceSVGDataSource + private readonly dataSource: TBFeatureFlagDataSource ) {} /** @export */ diff --git a/tensorboard/webapp/feature_flag/force_svg_util.ts b/tensorboard/webapp/feature_flag/force_svg_util.ts new file mode 100644 index 00000000000..66ea526e964 --- /dev/null +++ b/tensorboard/webapp/feature_flag/force_svg_util.ts @@ -0,0 +1,30 @@ +/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ +import {FORCE_SVG_RENDERER} from '../webapp_data_source/tb_feature_flag_data_source_types'; + +export function getForceSvgFlag(): boolean { + if (localStorage.getItem(FORCE_SVG_RENDERER)) { + return true; + } + return false; +} + +export function updateForceSvg(forceSvgFlag: boolean) { + if (forceSvgFlag) { + localStorage.setItem(FORCE_SVG_RENDERER, 'true'); + } else { + localStorage.removeItem(FORCE_SVG_RENDERER); + } +} diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts b/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts index e1080a18387..61e99e29d51 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_selectors.ts @@ -130,9 +130,9 @@ export const getEnabledTimeNamespacedState = createSelector( } ); -export const getForceSVGFeatureFlag = createSelector( +export const getForceSvgFeatureFlag = createSelector( getFeatureFlags, (flags: FeatureFlags): boolean => { - return flags.forceSVG; + return flags.forceSvg; } ); diff --git a/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts b/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts index 78c766b1261..551d4d2c25b 100644 --- a/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts +++ b/tensorboard/webapp/feature_flag/store/feature_flag_store_config_provider.ts @@ -32,7 +32,7 @@ export const initialState: FeatureFlagState = { enableTimeSeriesPromotion: false, enabledCardWidthSetting: true, enabledTimeNamespacedState: false, - forceSVG: false, + forceSvg: false, }, flagOverrides: {}, }; diff --git a/tensorboard/webapp/feature_flag/types.ts b/tensorboard/webapp/feature_flag/types.ts index 10ddc7098b6..bd854bd3a83 100644 --- a/tensorboard/webapp/feature_flag/types.ts +++ b/tensorboard/webapp/feature_flag/types.ts @@ -51,5 +51,5 @@ export interface FeatureFlags { // settings are kept during navigation. enabledTimeNamespacedState: boolean; - forceSVG: boolean; + forceSvg: boolean; } diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html index b2b78693d48..8ce3ef97f2a 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html @@ -102,7 +102,7 @@ > { @Input() xAxisType!: XAxisType; @Input() xScaleType!: ScaleType; @Input() useDarkMode!: boolean; - @Input() forceSVG!: boolean; + @Input() forceSvg!: boolean; @Input() selectedTime!: ViewSelectedTime | null; @Output() onFullSizeToggle = new EventEmitter(); diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index 07f646b2a27..90556879280 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -37,7 +37,7 @@ import { } from 'rxjs/operators'; import {State} from '../../../app_state'; import {ExperimentAlias} from '../../../experiments/types'; -import {getForceSVGFeatureFlag} from '../../../feature_flag/store/feature_flag_selectors'; +import {getForceSvgFeatureFlag} from '../../../feature_flag/store/feature_flag_selectors'; import { getCardPinnedState, getCurrentRouteRunSelection, @@ -128,7 +128,7 @@ function areSeriesEqual( [xScaleType]="xScaleType$ | async" [useDarkMode]="useDarkMode$ | async" [selectedTime]="selectedTime$ | async" - [forceSVG]="forceSVG$ | async" + [forceSvg]="forceSvg$ | async" (onFullSizeToggle)="onFullSizeToggle()" (onPinClicked)="pinStateChanged.emit($event)" observeIntersection @@ -176,7 +176,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { readonly ignoreOutliers$ = this.store.select(getMetricsIgnoreOutliers); readonly tooltipSort$ = this.store.select(getMetricsTooltipSort); readonly xAxisType$ = this.store.select(getMetricsXAxisType); - readonly forceSVG$ = this.store.select(getForceSVGFeatureFlag); + readonly forceSvg$ = this.store.select(getForceSvgFeatureFlag); readonly xScaleType$ = this.store.select(getMetricsXAxisType).pipe( map((xAxisType) => { switch (xAxisType) { diff --git a/tensorboard/webapp/webapp_data_source/BUILD b/tensorboard/webapp/webapp_data_source/BUILD index 5b82c0179a8..05a29b639f9 100644 --- a/tensorboard/webapp/webapp_data_source/BUILD +++ b/tensorboard/webapp/webapp_data_source/BUILD @@ -91,7 +91,6 @@ tf_ng_module( tf_ng_module( name = "feature_flag", srcs = [ - "force_SVG_data_source.ts", "query_params.ts", "tb_feature_flag_data_source.ts", "tb_feature_flag_module.ts", diff --git a/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts b/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts deleted file mode 100644 index 0ad06ec3b5d..00000000000 --- a/tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts +++ /dev/null @@ -1,41 +0,0 @@ -/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -==============================================================================*/ -import {Injectable} from '@angular/core'; -import {QueryParams} from './query_params'; -import {FORCE_SVG_RENDERER} from './tb_feature_flag_data_source_types'; - -@Injectable() -export class ForceSVGDataSource { - constructor(readonly queryParams: QueryParams) {} - - getAndUpdateForceSVGFlag(): boolean { - this.updateLocalStorage(); - if (localStorage.getItem(FORCE_SVG_RENDERER)) { - return true; - } - return false; - } - - private updateLocalStorage() { - const params = this.queryParams.getParams(); - if (params.has(FORCE_SVG_RENDERER)) { - if (params.get(FORCE_SVG_RENDERER) !== 'false') { - localStorage.setItem(FORCE_SVG_RENDERER, 'true'); - } else { - localStorage.removeItem(FORCE_SVG_RENDERER); - } - } - } -} diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts index 2f6b313333c..106656d030b 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts @@ -23,6 +23,7 @@ import { ENABLE_LINK_TIME_PARAM_KEY, ENABLE_TIME_NAMESPACED_STATE, EXPERIMENTAL_PLUGIN_QUERY_PARAM_KEY, + FORCE_SVG_RENDERER, SCALARS_BATCH_SIZE_PARAM_KEY, TBFeatureFlagDataSource, } from './tb_feature_flag_data_source_types'; @@ -90,6 +91,10 @@ export class QueryParamsFeatureFlagDataSource params.get(ENABLE_TIME_NAMESPACED_STATE) !== 'false'; } + if (params.has(FORCE_SVG_RENDERER)) { + featureFlags.forceSvg = params.get(FORCE_SVG_RENDERER) !== 'false'; + } + return featureFlags; } diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_module.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_module.ts index 301cd69ffc2..91d3e8d0657 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_module.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_module.ts @@ -14,7 +14,6 @@ limitations under the License. ==============================================================================*/ import {NgModule} from '@angular/core'; -import {ForceSVGDataSource} from './force_SVG_data_source'; import {QueryParams} from './query_params'; import {QueryParamsFeatureFlagDataSource} from './tb_feature_flag_data_source'; import {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; @@ -23,7 +22,6 @@ import {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; providers: [ // Provide as injectable for other app-level implementations of // TBFeatureFlagDataSource. - ForceSVGDataSource, QueryParamsFeatureFlagDataSource, QueryParams, // Provide as the TBFeatureFlagDataSource implementation for the OSS app. From 6a2f1281ede1d36042cd8ebd8e9b13032b002619 Mon Sep 17 00:00:00 2001 From: James Hollyer Date: Thu, 20 Jan 2022 21:21:50 -0800 Subject: [PATCH 6/9] Make ForceSVGDataSource injectable and add tests --- tensorboard/webapp/feature_flag/BUILD | 22 ++++-- tensorboard/webapp/feature_flag/effects/BUILD | 4 +- .../effects/feature_flag_effects.ts | 9 ++- .../effects/feature_flag_effects_test.ts | 54 +++++++++++++- .../feature_flag/feature_flag_module.ts | 2 + ...e_svg_util.ts => force_svg_data_source.ts} | 26 ++++--- .../force_svg_data_source_module.ts | 22 ++++++ .../force_svg_data_source_test.ts | 74 +++++++++++++++++++ tensorboard/webapp/feature_flag/testing.ts | 1 + tensorboard/webapp/webapp_data_source/BUILD | 1 + .../tb_feature_flag_testing.ts | 3 +- 11 files changed, 195 insertions(+), 23 deletions(-) rename tensorboard/webapp/feature_flag/{force_svg_util.ts => force_svg_data_source.ts} (64%) create mode 100644 tensorboard/webapp/feature_flag/force_svg_data_source_module.ts create mode 100644 tensorboard/webapp/feature_flag/force_svg_data_source_test.ts diff --git a/tensorboard/webapp/feature_flag/BUILD b/tensorboard/webapp/feature_flag/BUILD index 2830c3d6723..0c22963d737 100644 --- a/tensorboard/webapp/feature_flag/BUILD +++ b/tensorboard/webapp/feature_flag/BUILD @@ -8,6 +8,7 @@ tf_ng_module( "feature_flag_module.ts", ], deps = [ + ":force_svg_data_source", "//tensorboard/webapp/feature_flag/effects", "//tensorboard/webapp/feature_flag/store", "//tensorboard/webapp/feature_flag/store:types", @@ -19,10 +20,11 @@ tf_ng_module( ], ) -tf_ts_library( - name = "force_svg_util", +tf_ng_module( + name = "force_svg_data_source", srcs = [ - "force_svg_util.ts", + "force_svg_data_source.ts", + "force_svg_data_source_module.ts", ], deps = [ "//tensorboard/webapp/webapp_data_source:feature_flag_types", @@ -40,6 +42,16 @@ tf_ts_library( tf_ts_library( name = "testing", testonly = True, - srcs = ["testing.ts"], - deps = [":types"], + srcs = [ + "force_svg_data_source_test.ts", + "testing.ts", + ], + deps = [ + ":force_svg_data_source", + ":types", + "//tensorboard/webapp/angular:expect_angular_core_testing", + "//tensorboard/webapp/util:local_storage_testing", + "//tensorboard/webapp/webapp_data_source:feature_flag_types", + "@npm//@types/jasmine", + ], ) diff --git a/tensorboard/webapp/feature_flag/effects/BUILD b/tensorboard/webapp/feature_flag/effects/BUILD index 34c1b421d5d..91ff8480a37 100644 --- a/tensorboard/webapp/feature_flag/effects/BUILD +++ b/tensorboard/webapp/feature_flag/effects/BUILD @@ -8,7 +8,7 @@ tf_ng_module( "feature_flag_effects.ts", ], deps = [ - "//tensorboard/webapp/feature_flag:force_svg_util", + "//tensorboard/webapp/feature_flag:force_svg_data_source", "//tensorboard/webapp/feature_flag/actions", "//tensorboard/webapp/feature_flag/store", "//tensorboard/webapp/feature_flag/store:types", @@ -31,7 +31,9 @@ tf_ng_module( ":effects", "//tensorboard/webapp/angular:expect_angular_core_testing", "//tensorboard/webapp/angular:expect_ngrx_store_testing", + "//tensorboard/webapp/feature_flag:force_svg_data_source", "//tensorboard/webapp/feature_flag:testing", + "//tensorboard/webapp/feature_flag:types", "//tensorboard/webapp/feature_flag/actions", "//tensorboard/webapp/feature_flag/store", "//tensorboard/webapp/feature_flag/store:types", diff --git a/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts b/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts index 422e3e2f273..b47665cf9a7 100644 --- a/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts +++ b/tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts @@ -19,7 +19,7 @@ import {Action, createAction, Store} from '@ngrx/store'; import {combineLatestWith, map} from 'rxjs/operators'; import {TBFeatureFlagDataSource} from '../../webapp_data_source/tb_feature_flag_data_source_types'; import {partialFeatureFlagsLoaded} from '../actions/feature_flag_actions'; -import {getForceSvgFlag, updateForceSvg} from '../force_svg_util'; +import {ForceSvgDataSource} from '../force_svg_data_source'; import {getIsAutoDarkModeAllowed} from '../store/feature_flag_selectors'; import {State} from '../store/feature_flag_types'; @@ -36,9 +36,9 @@ export class FeatureFlagEffects { const features = this.dataSource.getFeatures(isDarkModeAllowed); if (features.forceSvg != null) { - updateForceSvg(features.forceSvg); + this.forceSvgDataSource.updateForceSvgFlag(features.forceSvg); } else { - features.forceSvg = getForceSvgFlag(); + features.forceSvg = this.forceSvgDataSource.getForceSvgFlag(); } return partialFeatureFlagsLoaded({features}); @@ -49,7 +49,8 @@ export class FeatureFlagEffects { constructor( private readonly actions$: Actions, private readonly store: Store, - private readonly dataSource: TBFeatureFlagDataSource + private readonly dataSource: TBFeatureFlagDataSource, + private readonly forceSvgDataSource: ForceSvgDataSource ) {} /** @export */ diff --git a/tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts b/tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts index 54fa78b5660..5274298dfac 100644 --- a/tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts +++ b/tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts @@ -23,21 +23,25 @@ import { TestingTBFeatureFlagDataSource, } from '../../webapp_data_source/tb_feature_flag_testing'; import {partialFeatureFlagsLoaded} from '../actions/feature_flag_actions'; +import {ForceSvgDataSource} from '../force_svg_data_source'; +import {ForceSvgDataSourceModule} from '../force_svg_data_source_module'; import {getIsAutoDarkModeAllowed} from '../store/feature_flag_selectors'; import {State} from '../store/feature_flag_types'; import {buildFeatureFlag} from '../testing'; +import {FeatureFlags} from '../types'; import {FeatureFlagEffects} from './feature_flag_effects'; -describe('feature_flag_effects', () => { +fdescribe('feature_flag_effects', () => { let actions: ReplaySubject; let store: MockStore; let dataSource: TestingTBFeatureFlagDataSource; + let forceSvgDataSource: ForceSvgDataSource; let effects: FeatureFlagEffects; beforeEach(async () => { actions = new ReplaySubject(1); await TestBed.configureTestingModule({ - imports: [TBFeatureFlagTestingModule], + imports: [TBFeatureFlagTestingModule, ForceSvgDataSourceModule], providers: [ provideMockActions(actions), FeatureFlagEffects, @@ -47,6 +51,7 @@ describe('feature_flag_effects', () => { effects = TestBed.inject(FeatureFlagEffects); store = TestBed.inject>(Store) as MockStore; dataSource = TestBed.inject(TestingTBFeatureFlagDataSource); + forceSvgDataSource = TestBed.inject(ForceSvgDataSource); store.overrideSelector(getIsAutoDarkModeAllowed, false); }); @@ -79,5 +84,50 @@ describe('feature_flag_effects', () => { }), ]); }); + + it('calls updateForceSvgFlag when getFeatures returns a value for forceSvg', () => { + spyOn(dataSource, 'getFeatures').and.returnValue( + buildFeatureFlag({ + forceSvg: true, + }) + ); + let updateSpy = spyOn( + forceSvgDataSource, + 'updateForceSvgFlag' + ).and.stub(); + + actions.next(effects.ngrxOnInitEffects()); + + expect(recordedActions).toEqual([ + partialFeatureFlagsLoaded({ + features: buildFeatureFlag({ + forceSvg: true, + }), + }), + ]); + + expect(updateSpy).toHaveBeenCalledOnceWith(true); + }); + + fit('gets forceSVG flag from ForceSvgDataSource when getFeatures returns no value for forceSvg', () => { + let featureFlags: Partial = buildFeatureFlag(); + delete featureFlags.forceSvg; + spyOn(dataSource, 'getFeatures').and.returnValue(featureFlags); + let getSpy = spyOn(forceSvgDataSource, 'getForceSvgFlag').and.returnValue( + true + ); + + actions.next(effects.ngrxOnInitEffects()); + + expect(recordedActions).toEqual([ + partialFeatureFlagsLoaded({ + features: buildFeatureFlag({ + forceSvg: true, + }), + }), + ]); + + expect(getSpy).toHaveBeenCalledOnceWith(); + }); }); }); diff --git a/tensorboard/webapp/feature_flag/feature_flag_module.ts b/tensorboard/webapp/feature_flag/feature_flag_module.ts index 1bee177bb1d..7a3f26f741e 100644 --- a/tensorboard/webapp/feature_flag/feature_flag_module.ts +++ b/tensorboard/webapp/feature_flag/feature_flag_module.ts @@ -23,6 +23,7 @@ import { } from '../persistent_settings'; import {TBFeatureFlagModule} from '../webapp_data_source/tb_feature_flag_module'; import {FeatureFlagEffects} from './effects/feature_flag_effects'; +import {ForceSvgDataSourceModule} from './force_svg_data_source_module'; import {reducers} from './store/feature_flag_reducers'; import {getEnableDarkModeOverride} from './store/feature_flag_selectors'; import { @@ -44,6 +45,7 @@ export function getThemeSettingSelector() { @NgModule({ imports: [ + ForceSvgDataSourceModule, TBFeatureFlagModule, StoreModule.forFeature( FEATURE_FLAG_FEATURE_KEY, diff --git a/tensorboard/webapp/feature_flag/force_svg_util.ts b/tensorboard/webapp/feature_flag/force_svg_data_source.ts similarity index 64% rename from tensorboard/webapp/feature_flag/force_svg_util.ts rename to tensorboard/webapp/feature_flag/force_svg_data_source.ts index 66ea526e964..e926f570ecc 100644 --- a/tensorboard/webapp/feature_flag/force_svg_util.ts +++ b/tensorboard/webapp/feature_flag/force_svg_data_source.ts @@ -12,19 +12,25 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ +import {Injectable} from '@angular/core'; import {FORCE_SVG_RENDERER} from '../webapp_data_source/tb_feature_flag_data_source_types'; -export function getForceSvgFlag(): boolean { - if (localStorage.getItem(FORCE_SVG_RENDERER)) { - return true; +@Injectable() +export class ForceSvgDataSource { + constructor() {} + + getForceSvgFlag(): boolean { + if (localStorage.getItem(FORCE_SVG_RENDERER)) { + return true; + } + return false; } - return false; -} -export function updateForceSvg(forceSvgFlag: boolean) { - if (forceSvgFlag) { - localStorage.setItem(FORCE_SVG_RENDERER, 'true'); - } else { - localStorage.removeItem(FORCE_SVG_RENDERER); + updateForceSvgFlag(forceSvgFlag: boolean) { + if (forceSvgFlag) { + localStorage.setItem(FORCE_SVG_RENDERER, 'true'); + } else { + localStorage.removeItem(FORCE_SVG_RENDERER); + } } } diff --git a/tensorboard/webapp/feature_flag/force_svg_data_source_module.ts b/tensorboard/webapp/feature_flag/force_svg_data_source_module.ts new file mode 100644 index 00000000000..e3629444364 --- /dev/null +++ b/tensorboard/webapp/feature_flag/force_svg_data_source_module.ts @@ -0,0 +1,22 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ + +import {NgModule} from '@angular/core'; +import {ForceSvgDataSource} from './force_svg_data_source'; + +@NgModule({ + providers: [ForceSvgDataSource], +}) +export class ForceSvgDataSourceModule {} diff --git a/tensorboard/webapp/feature_flag/force_svg_data_source_test.ts b/tensorboard/webapp/feature_flag/force_svg_data_source_test.ts new file mode 100644 index 00000000000..14f685776d2 --- /dev/null +++ b/tensorboard/webapp/feature_flag/force_svg_data_source_test.ts @@ -0,0 +1,74 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ +import {TestBed} from '@angular/core/testing'; +import {LocalStorageTestingModule} from '../util/local_storage_testing'; +import {FORCE_SVG_RENDERER} from '../webapp_data_source/tb_feature_flag_data_source_types'; +import {ForceSvgDataSource} from './force_svg_data_source'; + +describe('feature_flag/force_svg_util test', () => { + let dataSource: ForceSvgDataSource; + let getItemReturnValue: string | null; + let getItemSpy: jasmine.Spy; + let setItemSpy: jasmine.Spy; + let removeItemSpy: jasmine.Spy; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [LocalStorageTestingModule], + providers: [ForceSvgDataSource], + }).compileComponents(); + + // localStorage = TestBed.inject(TestingLocalStorage); + getItemSpy = spyOn(window.localStorage, 'getItem').and.callFake( + (key: string) => { + return getItemReturnValue; + } + ); + setItemSpy = spyOn(window.localStorage, 'setItem').and.stub(); + removeItemSpy = spyOn(window.localStorage, 'removeItem').and.stub(); + dataSource = TestBed.inject(ForceSvgDataSource); + }); + describe('#getForceSVG', () => { + it('returns false if localStorage.getItem returns null', () => { + getItemReturnValue = null; + const actual = dataSource.getForceSvgFlag(); + expect(getItemSpy).toHaveBeenCalledOnceWith(FORCE_SVG_RENDERER); + expect(actual).toBeFalse(); + }); + + it('returns true if there is a value returned by localstorage.getItem with the key "forceSVG"', () => { + getItemReturnValue = 'this should not matter'; + const actual = dataSource.getForceSvgFlag(); + expect(getItemSpy).toHaveBeenCalledOnceWith(FORCE_SVG_RENDERER); + expect(actual).toBeTruthy(); + }); + }); + + describe('updateForceSVG', () => { + it('Creates localStorage entry with key forceSVG when passed truthy', () => { + let dataSource = new ForceSvgDataSource(); + dataSource.updateForceSvgFlag(true); + expect(setItemSpy).toHaveBeenCalledOnceWith( + FORCE_SVG_RENDERER, + jasmine.any(String) + ); + }); + it('calls localStorage.removeItem with key forceSVG', () => { + let dataSource = new ForceSvgDataSource(); + dataSource.updateForceSvgFlag(false); + expect(removeItemSpy).toHaveBeenCalledOnceWith(FORCE_SVG_RENDERER); + }); + }); +}); diff --git a/tensorboard/webapp/feature_flag/testing.ts b/tensorboard/webapp/feature_flag/testing.ts index 1720fb68326..6ea5bd1d00c 100644 --- a/tensorboard/webapp/feature_flag/testing.ts +++ b/tensorboard/webapp/feature_flag/testing.ts @@ -32,6 +32,7 @@ export function buildFeatureFlag( enableTimeSeriesPromotion: false, enabledCardWidthSetting: false, enabledTimeNamespacedState: false, + forceSvg: false, ...override, }; } diff --git a/tensorboard/webapp/webapp_data_source/BUILD b/tensorboard/webapp/webapp_data_source/BUILD index 05a29b639f9..575a40ea7e4 100644 --- a/tensorboard/webapp/webapp_data_source/BUILD +++ b/tensorboard/webapp/webapp_data_source/BUILD @@ -114,6 +114,7 @@ tf_ng_module( deps = [ ":feature_flag_types", "//tensorboard/webapp/feature_flag:testing", + "//tensorboard/webapp/feature_flag:types", "@npm//@angular/core", ], ) diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_testing.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_testing.ts index 5e75c6e5a5d..0f441dc3c3d 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_testing.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_testing.ts @@ -15,11 +15,12 @@ limitations under the License. import {Injectable, NgModule} from '@angular/core'; import {buildFeatureFlag} from '../feature_flag/testing'; +import {FeatureFlags} from '../feature_flag/types'; import {TBFeatureFlagDataSource} from './tb_feature_flag_data_source_types'; @Injectable() export class TestingTBFeatureFlagDataSource extends TBFeatureFlagDataSource { - getFeatures() { + getFeatures(): Partial { return buildFeatureFlag(); } } From 07e41d497e1ac1913cc7014c7206f8be2dbbd821 Mon Sep 17 00:00:00 2001 From: James Hollyer Date: Fri, 21 Jan 2022 13:28:14 -0800 Subject: [PATCH 7/9] Code Review 2 --- .../effects/feature_flag_effects_test.ts | 9 +++++-- .../feature_flag/force_svg_data_source.ts | 13 +++++++--- .../force_svg_data_source_module.ts | 2 +- .../force_svg_data_source_test.ts | 25 +++++++++++-------- tensorboard/webapp/feature_flag/types.ts | 3 ++- .../views/card_renderer/scalar_card_test.ts | 19 ++++++++++++++ .../tb_feature_flag_data_source_test.ts | 22 ++++++++++++++++ 7 files changed, 74 insertions(+), 19 deletions(-) diff --git a/tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts b/tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts index 5274298dfac..a2e4b1b16f7 100644 --- a/tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts +++ b/tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts @@ -31,7 +31,7 @@ import {buildFeatureFlag} from '../testing'; import {FeatureFlags} from '../types'; import {FeatureFlagEffects} from './feature_flag_effects'; -fdescribe('feature_flag_effects', () => { +describe('feature_flag_effects', () => { let actions: ReplaySubject; let store: MockStore; let dataSource: TestingTBFeatureFlagDataSource; @@ -109,10 +109,14 @@ fdescribe('feature_flag_effects', () => { expect(updateSpy).toHaveBeenCalledOnceWith(true); }); - fit('gets forceSVG flag from ForceSvgDataSource when getFeatures returns no value for forceSvg', () => { + it('gets forceSVG flag from ForceSvgDataSource when getFeatures returns no value for forceSvg', () => { let featureFlags: Partial = buildFeatureFlag(); delete featureFlags.forceSvg; spyOn(dataSource, 'getFeatures').and.returnValue(featureFlags); + let updateSpy = spyOn( + forceSvgDataSource, + 'updateForceSvgFlag' + ).and.stub(); let getSpy = spyOn(forceSvgDataSource, 'getForceSvgFlag').and.returnValue( true ); @@ -128,6 +132,7 @@ fdescribe('feature_flag_effects', () => { ]); expect(getSpy).toHaveBeenCalledOnceWith(); + expect(updateSpy).toHaveBeenCalledTimes(0); }); }); }); diff --git a/tensorboard/webapp/feature_flag/force_svg_data_source.ts b/tensorboard/webapp/feature_flag/force_svg_data_source.ts index e926f570ecc..ba15022bc53 100644 --- a/tensorboard/webapp/feature_flag/force_svg_data_source.ts +++ b/tensorboard/webapp/feature_flag/force_svg_data_source.ts @@ -13,14 +13,15 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {Injectable} from '@angular/core'; -import {FORCE_SVG_RENDERER} from '../webapp_data_source/tb_feature_flag_data_source_types'; + +const FORCE_SVG_RENDERER_KEY = '_tb_force_svg'; @Injectable() export class ForceSvgDataSource { constructor() {} getForceSvgFlag(): boolean { - if (localStorage.getItem(FORCE_SVG_RENDERER)) { + if (localStorage.getItem(FORCE_SVG_RENDERER_KEY)) { return true; } return false; @@ -28,9 +29,13 @@ export class ForceSvgDataSource { updateForceSvgFlag(forceSvgFlag: boolean) { if (forceSvgFlag) { - localStorage.setItem(FORCE_SVG_RENDERER, 'true'); + localStorage.setItem(FORCE_SVG_RENDERER_KEY, 'present'); } else { - localStorage.removeItem(FORCE_SVG_RENDERER); + localStorage.removeItem(FORCE_SVG_RENDERER_KEY); } } } + +export const TEST_ONLY = { + FORCE_SVG_RENDERER_KEY, +}; diff --git a/tensorboard/webapp/feature_flag/force_svg_data_source_module.ts b/tensorboard/webapp/feature_flag/force_svg_data_source_module.ts index e3629444364..dcc995d8352 100644 --- a/tensorboard/webapp/feature_flag/force_svg_data_source_module.ts +++ b/tensorboard/webapp/feature_flag/force_svg_data_source_module.ts @@ -1,4 +1,4 @@ -/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. +/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/tensorboard/webapp/feature_flag/force_svg_data_source_test.ts b/tensorboard/webapp/feature_flag/force_svg_data_source_test.ts index 14f685776d2..d506069ad89 100644 --- a/tensorboard/webapp/feature_flag/force_svg_data_source_test.ts +++ b/tensorboard/webapp/feature_flag/force_svg_data_source_test.ts @@ -1,4 +1,4 @@ -/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. +/* Copyright 2022 The TensorFlow Authors. All Rights Reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,8 +14,7 @@ limitations under the License. ==============================================================================*/ import {TestBed} from '@angular/core/testing'; import {LocalStorageTestingModule} from '../util/local_storage_testing'; -import {FORCE_SVG_RENDERER} from '../webapp_data_source/tb_feature_flag_data_source_types'; -import {ForceSvgDataSource} from './force_svg_data_source'; +import {ForceSvgDataSource, TEST_ONLY} from './force_svg_data_source'; describe('feature_flag/force_svg_util test', () => { let dataSource: ForceSvgDataSource; @@ -30,7 +29,6 @@ describe('feature_flag/force_svg_util test', () => { providers: [ForceSvgDataSource], }).compileComponents(); - // localStorage = TestBed.inject(TestingLocalStorage); getItemSpy = spyOn(window.localStorage, 'getItem').and.callFake( (key: string) => { return getItemReturnValue; @@ -40,35 +38,40 @@ describe('feature_flag/force_svg_util test', () => { removeItemSpy = spyOn(window.localStorage, 'removeItem').and.stub(); dataSource = TestBed.inject(ForceSvgDataSource); }); + describe('#getForceSVG', () => { it('returns false if localStorage.getItem returns null', () => { getItemReturnValue = null; const actual = dataSource.getForceSvgFlag(); - expect(getItemSpy).toHaveBeenCalledOnceWith(FORCE_SVG_RENDERER); + expect(getItemSpy).toHaveBeenCalledOnceWith( + TEST_ONLY.FORCE_SVG_RENDERER_KEY + ); expect(actual).toBeFalse(); }); it('returns true if there is a value returned by localstorage.getItem with the key "forceSVG"', () => { getItemReturnValue = 'this should not matter'; const actual = dataSource.getForceSvgFlag(); - expect(getItemSpy).toHaveBeenCalledOnceWith(FORCE_SVG_RENDERER); + expect(getItemSpy).toHaveBeenCalledOnceWith( + TEST_ONLY.FORCE_SVG_RENDERER_KEY + ); expect(actual).toBeTruthy(); }); }); describe('updateForceSVG', () => { - it('Creates localStorage entry with key forceSVG when passed truthy', () => { - let dataSource = new ForceSvgDataSource(); + it('creates localStorage entry with key forceSVG when passed truthy', () => { dataSource.updateForceSvgFlag(true); expect(setItemSpy).toHaveBeenCalledOnceWith( - FORCE_SVG_RENDERER, + TEST_ONLY.FORCE_SVG_RENDERER_KEY, jasmine.any(String) ); }); it('calls localStorage.removeItem with key forceSVG', () => { - let dataSource = new ForceSvgDataSource(); dataSource.updateForceSvgFlag(false); - expect(removeItemSpy).toHaveBeenCalledOnceWith(FORCE_SVG_RENDERER); + expect(removeItemSpy).toHaveBeenCalledOnceWith( + TEST_ONLY.FORCE_SVG_RENDERER_KEY + ); }); }); }); diff --git a/tensorboard/webapp/feature_flag/types.ts b/tensorboard/webapp/feature_flag/types.ts index bd854bd3a83..a4b82a42093 100644 --- a/tensorboard/webapp/feature_flag/types.ts +++ b/tensorboard/webapp/feature_flag/types.ts @@ -50,6 +50,7 @@ export interface FeatureFlags { // Whether to enable time-namespaced state and how it impacts how user // settings are kept during navigation. enabledTimeNamespacedState: boolean; - + // Flag for the escape hatch from webGL. This only effects the TimeSeries + // Scalar cards. forceSvg: boolean; } diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts index 288bfa060d9..28d07e57025 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -500,6 +500,25 @@ describe('scalar card', () => { expect(lineChartEl.componentInstance.useDarkMode).toBe(true); })); + + it('sets preferredRendererType to SVG when getForceSvgFeatureFlag returns true', fakeAsync(() => { + store.overrideSelector(selectors.getForceSvgFeatureFlag, false); + const fixture = createComponent('card1'); + fixture.detectChanges(); + + const lineChartEl = fixture.debugElement.query(Selector.LINE_CHART); + expect(lineChartEl.componentInstance.preferredRendererType).toBe( + RendererType.WEBGL + ); + + store.overrideSelector(selectors.getForceSvgFeatureFlag, true); + store.refreshState(); + fixture.detectChanges(); + + expect(lineChartEl.componentInstance.preferredRendererType).toBe( + RendererType.SVG + ); + })); }); describe('displayName', () => { diff --git a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts index feed77c6671..3914c0685c6 100644 --- a/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts +++ b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source_test.ts @@ -183,6 +183,28 @@ describe('tb_feature_flag_data_source', () => { }); }); + it('returns forceSVG from the query params', () => { + getParamsSpy.and.returnValues( + new URLSearchParams('forceSVG=false'), + new URLSearchParams('forceSVG='), + new URLSearchParams('forceSVG=true'), + new URLSearchParams('forceSVG=foo') + ); + + expect(dataSource.getFeatures()).toEqual({ + forceSvg: false, + }); + expect(dataSource.getFeatures()).toEqual({ + forceSvg: true, + }); + expect(dataSource.getFeatures()).toEqual({ + forceSvg: true, + }); + expect(dataSource.getFeatures()).toEqual({ + forceSvg: true, + }); + }); + it('returns all flag values when they are all set', () => { getParamsSpy.and.returnValue( new URLSearchParams( From 0f54bb04a9bcabe99da09b3e5fbec0eb31a84156 Mon Sep 17 00:00:00 2001 From: James Hollyer Date: Mon, 24 Jan 2022 11:03:20 -0800 Subject: [PATCH 8/9] Capitalize WebGL nit --- tensorboard/webapp/feature_flag/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tensorboard/webapp/feature_flag/types.ts b/tensorboard/webapp/feature_flag/types.ts index a4b82a42093..a98490715a4 100644 --- a/tensorboard/webapp/feature_flag/types.ts +++ b/tensorboard/webapp/feature_flag/types.ts @@ -50,7 +50,7 @@ export interface FeatureFlags { // Whether to enable time-namespaced state and how it impacts how user // settings are kept during navigation. enabledTimeNamespacedState: boolean; - // Flag for the escape hatch from webGL. This only effects the TimeSeries + // Flag for the escape hatch from WebGL. This only effects the TimeSeries // Scalar cards. forceSvg: boolean; } From 8f8472abe4e7c065c781049d96a0d596f2c9679a Mon Sep 17 00:00:00 2001 From: James Hollyer Date: Mon, 24 Jan 2022 15:45:00 -0800 Subject: [PATCH 9/9] mock out getForceSvgFeatureFlagselector before all scalar card tests --- .../webapp/metrics/views/card_renderer/scalar_card_test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts index 28d07e57025..69941c35f6a 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -294,6 +294,7 @@ describe('scalar card', () => { ); store.overrideSelector(selectors.getRunColorMap, {}); store.overrideSelector(selectors.getDarkModeEnabled, false); + store.overrideSelector(selectors.getForceSvgFeatureFlag, false); }); describe('basic renders', () => {