Skip to content

Commit

Permalink
Fix UI not being able to switch OS on tree view
Browse files Browse the repository at this point in the history
This commit addresses a bug where updating the tree collection before
node updates caused rendering errors due to the presence of nodes that
no longer exist in the new collection.

Changes:

- Fix rendering order issue when updating tree collections by manually
  controlling the rendering process, ensuring that the rendering queue
  and currently rendered nodes are cleared before updating. This aligns
  the rendering process with the updated collection, avoiding errors
  related to outdated node states.
- Add Cypress E2E to switch between all operating systems and script
  views to ensure there are no uncaught errors to avoid regression.
- In download URL list view, use unified `getSupportedOsList()` from
  application instead of hardcoding supported operating systems to
  prevent duplication and easier future changes.
- Rename `initial-nodes` to `nodes` as these nodes are mutable and not
  necessarily only the "initial" ones.
- Refactor to centralize function to render/get operating system names
  for better re-usability in E2E tests.
  • Loading branch information
undergroundwires committed Dec 13, 2023
1 parent fe3de49 commit 1dd3a46
Show file tree
Hide file tree
Showing 14 changed files with 288 additions and 75 deletions.
18 changes: 5 additions & 13 deletions src/presentation/components/Scripts/Menu/TheOsChanger.vue
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@
import { defineComponent, computed } from 'vue';
import { injectKey } from '@/presentation/injectionSymbols';
import { OperatingSystem } from '@/domain/OperatingSystem';
import { getOperatingSystemDisplayName } from '@/presentation/components/Shared/OperatingSystemNames';
import MenuOptionList from './MenuOptionList.vue';
import MenuOptionListItem from './MenuOptionListItem.vue';
interface IOsViewModel {
interface OperatingSystemOption {
readonly name: string;
readonly os: OperatingSystem;
}
Expand All @@ -31,12 +32,12 @@ export default defineComponent({
const { modifyCurrentContext, currentState } = injectKey((keys) => keys.useCollectionState);
const { application } = injectKey((keys) => keys.useApplication);
const allOses = computed<ReadonlyArray<IOsViewModel>>(
const allOses = computed<ReadonlyArray<OperatingSystemOption>>(
() => application
.getSupportedOsList()
.map((os) : IOsViewModel => ({
.map((os) : OperatingSystemOption => ({
os,
name: renderOsName(os),
name: getOperatingSystemDisplayName(os),
})),
);
Expand All @@ -57,13 +58,4 @@ export default defineComponent({
};
},
});
function renderOsName(os: OperatingSystem): string {
switch (os) {
case OperatingSystem.Windows: return 'Windows';
case OperatingSystem.macOS: return 'macOS';
case OperatingSystem.Linux: return 'Linux (preview)';
default: throw new RangeError(`Cannot render os name: ${OperatingSystem[os]}`);
}
}
</script>
6 changes: 3 additions & 3 deletions src/presentation/components/Scripts/View/Tree/ScriptsTree.vue
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
'top-padding': hasTopPadding,
}"
>
<template v-if="initialNodes.length">
<template v-if="nodes.length">
<TreeView
:initial-nodes="initialNodes"
:nodes="nodes"
:selected-leaf-node-ids="selectedScriptNodeIds"
:latest-filter-event="latestFilterEvent"
@node-state-changed="handleNodeChangedEvent($event)"
Expand Down Expand Up @@ -61,7 +61,7 @@ export default defineComponent({
}
return {
initialNodes: treeViewInputNodes,
nodes: treeViewInputNodes,
selectedScriptNodeIds,
latestFilterEvent,
handleNodeChangedEvent,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,19 @@ import { ReadOnlyTreeNode } from '../Node/TreeNode';
import { useNodeStateChangeAggregator } from '../UseNodeStateChangeAggregator';
import { TreeRoot } from '../TreeRoot/TreeRoot';
import { useCurrentTreeNodes } from '../UseCurrentTreeNodes';
import { QueryableNodes } from '../TreeRoot/NodeCollection/Query/QueryableNodes';
import { NodeRenderingStrategy } from './Scheduling/NodeRenderingStrategy';
import { DelayScheduler } from './DelayScheduler';
import { TimeoutDelayScheduler } from './Scheduling/TimeoutDelayScheduler';
import { RenderQueueOrderer } from './Ordering/RenderQueueOrderer';
import { CollapsedParentOrderer } from './Ordering/CollapsedParentOrderer';

export interface NodeRenderingControl {
readonly renderingStrategy: NodeRenderingStrategy;
clearRenderingStates(): void;
notifyRenderingUpdates(): void;
}

/**
* Renders tree nodes gradually to prevent UI freeze when loading large amounts of nodes.
*/
Expand All @@ -22,7 +29,7 @@ export function useGradualNodeRendering(
initialBatchSize = 30,
subsequentBatchSize = 5,
orderer: RenderQueueOrderer = new CollapsedParentOrderer(),
): NodeRenderingStrategy {
): NodeRenderingControl {
const nodesToRender = new Set<ReadOnlyTreeNode>();
const nodesBeingRendered = shallowRef(new Set<ReadOnlyTreeNode>());
let isRenderingInProgress = false;
Expand All @@ -31,6 +38,10 @@ export function useGradualNodeRendering(
const { onNodeStateChange } = useChangeAggregator(treeRootRef);
const { nodes } = useTreeNodes(treeRootRef);

function notifyRenderingUpdates() {
triggerRef(nodesBeingRendered);
}

function updateNodeRenderQueue(node: ReadOnlyTreeNode, isVisible: boolean) {
if (isVisible
&& !nodesToRender.has(node)
Expand All @@ -43,23 +54,31 @@ export function useGradualNodeRendering(
}
if (nodesBeingRendered.value.has(node)) {
nodesBeingRendered.value.delete(node);
triggerRef(nodesBeingRendered);
notifyRenderingUpdates();
}
}
}

watch(nodes, (newNodes) => {
function clearRenderingStates() {
nodesToRender.clear();
nodesBeingRendered.value.clear();
}

function initializeAndRenderNodes(newNodes: QueryableNodes) {
clearRenderingStates();
if (!newNodes || newNodes.flattenedNodes.length === 0) {
triggerRef(nodesBeingRendered);
notifyRenderingUpdates();
return;
}
newNodes
.flattenedNodes
.filter((node) => node.state.current.isVisible)
.forEach((node) => nodesToRender.add(node));
beginRendering();
}

watch(nodes, (newNodes) => {
initializeAndRenderNodes(newNodes);
}, { immediate: true });

onNodeStateChange((change) => {
Expand Down Expand Up @@ -91,7 +110,7 @@ export function useGradualNodeRendering(
nodesToRender.delete(node);
nodesBeingRendered.value.add(node);
});
triggerRef(nodesBeingRendered);
notifyRenderingUpdates();
scheduler.scheduleNext(
() => renderNextBatch(subsequentBatchSize),
renderingDelayInMs,
Expand All @@ -103,6 +122,10 @@ export function useGradualNodeRendering(
}

return {
shouldRender: shouldNodeBeRendered,
renderingStrategy: {
shouldRender: shouldNodeBeRendered,
},
clearRenderingStates,
notifyRenderingUpdates,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
ref="treeContainerElement"
class="tree"
>
<TreeRoot :tree-root="tree" :rendering-strategy="nodeRenderingScheduler">
<TreeRoot :tree-root="tree" :rendering-strategy="renderingStrategy">
<template #default="slotProps">
<slot name="node-content" v-bind="slotProps" />
</template>
Expand All @@ -15,6 +15,7 @@
import {
defineComponent, onMounted, watch,
shallowRef, toRef, shallowReadonly,
nextTick,
} from 'vue';
import { TreeRootManager } from './TreeRoot/TreeRootManager';
import TreeRoot from './TreeRoot/TreeRoot.vue';
Expand All @@ -27,15 +28,15 @@ import { useLeafNodeCheckedStateUpdater } from './UseLeafNodeCheckedStateUpdater
import { TreeNodeStateChangedEmittedEvent } from './Bindings/TreeNodeStateChangedEmittedEvent';
import { useAutoUpdateParentCheckState } from './UseAutoUpdateParentCheckState';
import { useAutoUpdateChildrenCheckState } from './UseAutoUpdateChildrenCheckState';
import { useGradualNodeRendering } from './Rendering/UseGradualNodeRendering';
import { useGradualNodeRendering, NodeRenderingControl } from './Rendering/UseGradualNodeRendering';
import type { PropType } from 'vue';
export default defineComponent({
components: {
TreeRoot,
},
props: {
initialNodes: {
nodes: {
type: Array as PropType<readonly TreeInputNodeData[]>,
default: () => [],
},
Expand Down Expand Up @@ -65,7 +66,7 @@ export default defineComponent({
useLeafNodeCheckedStateUpdater(treeRef, toRef(props, 'selectedLeafNodeIds'));
useAutoUpdateParentCheckState(treeRef);
useAutoUpdateChildrenCheckState(treeRef);
const nodeRenderingScheduler = useGradualNodeRendering(treeRef);
const nodeRenderer = useGradualNodeRendering(treeRef);
const { onNodeStateChange } = useNodeStateChangeAggregator(treeRef);
Expand All @@ -78,18 +79,44 @@ export default defineComponent({
});
onMounted(() => {
watch(() => props.initialNodes, (nodes) => {
tree.collection.updateRootNodes(nodes);
watch(() => props.nodes, async (nodes) => {
await forceRerenderNodes(
nodeRenderer,
() => tree.collection.updateRootNodes(nodes),
);
}, { immediate: true });
});
return {
treeContainerElement,
nodeRenderingScheduler,
renderingStrategy: nodeRenderer.renderingStrategy,
tree,
};
},
});
/**
* This function is used to manually trigger a re-render of the tree nodes.
* In Vue, manually controlling the rendering process is typically an anti-pattern,
* as Vue's reactivity system is designed to handle updates efficiently. However,
* in this specific case, it's necessary to ensure the correct order of rendering operations.
* This function first clears the rendering queue and the currently rendered nodes,
* ensuring that UI elements relying on outdated node states are removed. This is needed
* in scenarios where the collection is updated before the nodes, which can lead to errors
* if nodes that no longer exist in the collection are still being rendered.
* Using this function, we ensure a clean state before updating the nodes, aligning with
* the updated collection.
*/
async function forceRerenderNodes(
renderer: NodeRenderingControl,
nodeUpdater: () => void,
) {
renderer.clearRenderingStates();
renderer.notifyRenderingUpdates();
await nextTick();
nodeUpdater();
}
</script>

<style scoped lang="scss">
Expand Down
14 changes: 14 additions & 0 deletions src/presentation/components/Shared/OperatingSystemNames.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { OperatingSystem } from '@/domain/OperatingSystem';

export function getOperatingSystemDisplayName(os: OperatingSystem): string {
switch (os) {
case OperatingSystem.Windows:
return 'Windows';
case OperatingSystem.macOS:
return 'macOS';
case OperatingSystem.Linux:
return 'Linux (preview)';
default:
throw new RangeError(`Unsupported operating system ID: ${os}`);
}
}
13 changes: 5 additions & 8 deletions src/presentation/components/TheFooter/DownloadUrlList.vue
Original file line number Diff line number Diff line change
Expand Up @@ -24,26 +24,23 @@

<script lang="ts">
import { defineComponent } from 'vue';
import { OperatingSystem } from '@/domain/OperatingSystem';
import { injectKey } from '@/presentation/injectionSymbols';
import AppIcon from '@/presentation/components/Shared/Icon/AppIcon.vue';
import DownloadUrlListItem from './DownloadUrlListItem.vue';
const supportedOperativeSystems: readonly OperatingSystem[] = [
OperatingSystem.Windows,
OperatingSystem.Linux,
OperatingSystem.macOS,
];
export default defineComponent({
components: {
DownloadUrlListItem,
AppIcon,
},
setup() {
const { os: currentOs } = injectKey((keys) => keys.useRuntimeEnvironment);
const { application } = injectKey((keys) => keys.useApplication);
const supportedOperativeSystems = application.getSupportedOsList();
const supportedDesktops = [
...supportedOperativeSystems,
...application.getSupportedOsList(),
].sort((os) => (os === currentOs ? 0 : 1));
const hasCurrentOsDesktopVersion = currentOs === undefined
Expand Down
17 changes: 2 additions & 15 deletions src/presentation/components/TheFooter/DownloadUrlListItem.vue
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
} from 'vue';
import { injectKey } from '@/presentation/injectionSymbols';
import { OperatingSystem } from '@/domain/OperatingSystem';
import { getOperatingSystemDisplayName } from '@/presentation/components/Shared/OperatingSystemNames';
export default defineComponent({
props: {
Expand All @@ -33,7 +34,7 @@ export default defineComponent({
});
const operatingSystemName = computed<string>(() => {
return getOperatingSystemName(props.operatingSystem);
return getOperatingSystemDisplayName(props.operatingSystem);
});
const hasCurrentOsDesktopVersion = computed<boolean>(() => {
Expand All @@ -58,20 +59,6 @@ function hasDesktopVersion(os: OperatingSystem): boolean {
|| os === OperatingSystem.Linux
|| os === OperatingSystem.macOS;
}
function getOperatingSystemName(os: OperatingSystem): string {
switch (os) {
case OperatingSystem.Linux:
return 'Linux (preview)';
case OperatingSystem.macOS:
return 'macOS';
case OperatingSystem.Windows:
return 'Windows';
default:
throw new Error(`Unsupported os: ${OperatingSystem[os]}`);
}
}
</script>

<style scoped lang="scss">
Expand Down
6 changes: 6 additions & 0 deletions src/presentation/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions tests/e2e/.eslintrc.cjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module.exports = {
plugins: [
'../../.eslintrc.cjs', // Adjust the path as per your project structure
'cypress',
],
env: {
Expand Down
Loading

0 comments on commit 1dd3a46

Please sign in to comment.