Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Modified indicators incorrectly displayed for some UI5 controls in Adaptation Project #2264

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
0e58c1a
fix: incorrectly placed modified indicators
mmilko01 Aug 21, 2024
21d92fd
refacor: enhance types
mmilko01 Aug 27, 2024
0c74987
fix: type
mmilko01 Aug 27, 2024
3f485ce
refactor: get control instance based on App Component
mmilko01 Aug 28, 2024
8e35073
fix: check if change is available
mmilko01 Aug 28, 2024
5c8efed
Merge branch 'main' into fix/2263/incorrect-modified-indicators
mmilko01 Aug 28, 2024
d67e8eb
refactor: dynamically import ChangeHandlerAPI
mmilko01 Aug 29, 2024
0035131
refactor: get control id via JsControlTreeModifier
mmilko01 Aug 29, 2024
5eefabb
refactor: use different FL API based on UI5 Version
mmilko01 Aug 30, 2024
92f9fb5
test: amend tests
mmilko01 Sep 11, 2024
c4aafd3
chore: merge with main
mmilko01 Sep 11, 2024
e9018da
chore: fix main merge conflicts
mmilko01 Sep 11, 2024
11bd958
chore: create changeset
mmilko01 Sep 11, 2024
2b73513
Merge branch 'main' into fix/2263/incorrect-modified-indicators
mmilko01 Sep 11, 2024
130ba00
chore: fix lint error
mmilko01 Sep 11, 2024
dbc093f
Merge branch 'fix/2263/incorrect-modified-indicators' of https://gith…
mmilko01 Sep 11, 2024
0e6a23a
test: add tests
mmilko01 Sep 12, 2024
55b6b23
fix: sonar issue
mmilko01 Sep 12, 2024
b4f4ee6
test: improve coverage
mmilko01 Sep 12, 2024
4c99027
fix: lint issue
mmilko01 Sep 12, 2024
5dfca73
doc: fix JSDoc
mmilko01 Sep 12, 2024
1931d00
Merge branch 'main' into fix/2263/incorrect-modified-indicators
mmilko01 Sep 13, 2024
9931f05
refactor: do not leak change files to UI
mmilko01 Sep 13, 2024
67f0abf
Merge branch 'fix/2263/incorrect-modified-indicators' of https://gith…
mmilko01 Sep 13, 2024
748bd9d
refactor: change Selector type
mmilko01 Sep 13, 2024
75d7ba3
Merge branch 'main' into fix/2263/incorrect-modified-indicators
mmilko01 Sep 13, 2024
574c393
Merge branch 'main' into fix/2263/incorrect-modified-indicators
mmilko01 Sep 16, 2024
809dc7f
refactor: change changeFiles type
mmilko01 Sep 16, 2024
21ab914
refactor: define changedFiles as object
mmilko01 Sep 16, 2024
64b1527
fix: resolve conflicts
mmilko01 Oct 15, 2024
5a86c0d
fix: correctly display control changes
mmilko01 Oct 17, 2024
729bc12
feat: add control change component
nikmace Oct 17, 2024
fa6bd6e
Merge branch 'main' into fix/2263/incorrect-modified-indicators
mmilko01 Oct 17, 2024
9c4030d
chore: return cap.ts to original state
mmilko01 Oct 17, 2024
65d7d2f
refactor: rename interface
nikmace Oct 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/tiny-ducks-dress.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@sap-ux-private/control-property-editor-common": patch
"@sap-ux-private/preview-middleware": patch
---

Modified indicators incorrectly displayed for some UI5 controls in Adaptation Project
244 changes: 166 additions & 78 deletions packages/preview-middleware-client/src/cpe/changes/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ import {
} from '@sap-ux-private/control-property-editor-common';
import { applyChange } from './flex-change';
import type { SelectionService } from '../selection';

import type { ActionSenderFunction, SubscribeFunction, UI5AdaptationOptions } from '../types';
import type Event from 'sap/ui/base/Event';
import type FlexCommand from 'sap/ui/rta/command/FlexCommand';
import Log from 'sap/base/Log';
import { modeAndStackChangeHandler } from '../rta-service';
import JsControlTreeModifier from 'sap/ui/core/util/reflection/JsControlTreeModifier';
import FlexChange from 'sap/ui/fl/Change';
import { getError } from '../../utils/error';
import { isLowerThanMinimalUi5Version, getUi5Version } from '../../utils/version';

interface ChangeContent {
property: string;
Expand All @@ -43,10 +45,15 @@ interface Change {
content: ChangeContent;
creation: string;
changeType: string;
file: object;
}

type SavedChangesResponse = Record<string, Change>;

type ChangeFiles = {
fileName: string;
};

type Properties<T extends object> = { [K in keyof T]-?: K extends string ? K : never }[keyof T];
/**
* Assert change for its validity. Throws error if no value found in saved changes.
Expand Down Expand Up @@ -92,6 +99,8 @@ function modifyRTAErrorMessage(errorMessage: string, id: string, type: string):
export class ChangeService {
private savedChanges: SavedPropertyChange[] = [];
private sendAction: (action: ExternalAction) => void;
private pendingChanges: PendingChange[] = [];
private changedFiles: ChangeFiles[] = [];
voicis marked this conversation as resolved.
Show resolved Hide resolved
/**
*
* @param options ui5 adaptation options.
Expand Down Expand Up @@ -154,11 +163,11 @@ export class ChangeService {
*
* @param pendingChanges Changes that are waiting to be saved
*/
private updateStack(pendingChanges: PendingChange[] = []) {
private updateStack() {
this.sendAction(
changeStackModified({
saved: this.savedChanges,
pending: pendingChanges
saved: this.savedChanges ?? [],
pending: this.pendingChanges ?? []
})
);
}
Expand All @@ -167,53 +176,68 @@ export class ChangeService {
* Fetches saved changes from the workspace and sorts them.
*/
private async fetchSavedChanges(): Promise<void> {
this.changedFiles = [];
const savedChangesResponse = await fetch(FlexChangesEndPoints.changes + `?_=${Date.now()}`);
const savedChanges = (await savedChangesResponse.json()) as SavedChangesResponse;
const changes = (
Object.keys(savedChanges ?? {})
.map((key): SavedPropertyChange | UnknownSavedChange | undefined => {
const change: Change = savedChanges[key];
try {
assertChange(change);
if (
[change.content.newValue, change.content.newBinding].every(
(item) => item === undefined || item === null
)
) {
throw new Error('Invalid change, missing new value in the change file');
}
if (change.changeType !== 'propertyChange' && change.changeType !== 'propertyBindingChange') {
throw new Error('Unknown Change Type');
}
return {
type: 'saved',
kind: 'valid',
fileName: change.fileName,
controlId: change.selector.id,
propertyName: change.content.property,
value: change.content.newValue ?? change.content.newBinding,
timestamp: new Date(change.creation).getTime(),
controlName: change.selector.type ? (change.selector.type.split('.').pop() as string) : '',
changeType: change.changeType
};
} catch (error) {
// Gracefully handle change files with invalid content
if (change.fileName) {
const unknownChange: UnknownSavedChange = {
type: 'saved',
kind: 'unknown',
fileName: change.fileName,
controlId: change.selector?.id // some changes may not have selector
};
if (change.creation) {
unknownChange.timestamp = new Date(change.creation).getTime();
(
await Promise.all(
Object.keys(savedChanges ?? {}).map(
async (key): Promise<SavedPropertyChange | UnknownSavedChange | undefined> => {
const change: Change = savedChanges[key];
let selectorId;
try {
const flexObject = await this.getFlexObject(change);
selectorId = await this.getControlIdByChange(flexObject);
assertChange(change);
if (
[change.content.newValue, change.content.newBinding].every(
(item) => item === undefined || item === null
)
) {
throw new Error('Invalid change, missing new value in the change file');
}
if (
change.changeType !== 'propertyChange' &&
change.changeType !== 'propertyBindingChange'
) {
throw new Error('Unknown Change Type');
}
this.changedFiles.push(change);
return {
type: 'saved',
kind: 'valid',
fileName: change.fileName,
controlId: selectorId,
propertyName: change.content.property,
value: change.content.newValue ?? change.content.newBinding,
timestamp: new Date(change.creation).getTime(),
controlName: change.selector.type
? (change.selector.type.split('.').pop() as string)
: '',
changeType: change.changeType
};
} catch (error) {
// Gracefully handle change files with invalid content
if (change.fileName) {
this.changedFiles.push(change);
const unknownChange: UnknownSavedChange = {
type: 'saved',
kind: 'unknown',
fileName: change.fileName,
controlId: selectorId // some changes may not have selector
};
if (change.creation) {
unknownChange.timestamp = new Date(change.creation).getTime();
}
return unknownChange;
}
return undefined;
}
return unknownChange;
}
return undefined;
}
})
.filter((change) => !!change) as SavedPropertyChange[]
)
)
).filter((change) => !!change) as SavedPropertyChange[]
).sort((a, b) => b.timestamp - a.timestamp);
this.savedChanges = changes;
}
Expand Down Expand Up @@ -260,47 +284,49 @@ export class ChangeService {
const allCommands = stack.getCommands();
const executedCommands = stack.getAllExecutedCommands();
const inactiveCommandCount = allCommands.length - executedCommands.length;
let activeChanges: PendingChange[] = [];
allCommands.forEach((command: FlexCommand, i): void => {
let i: number, command: FlexCommand;
for ([i, command] of allCommands.entries()) {
try {
if (typeof command.getCommands === 'function') {
const subCommands = command.getCommands();
subCommands.forEach((subCommand) => {
const pendingChange = this.prepareChangeType(subCommand, inactiveCommandCount, i);
if (pendingChange) {
activeChanges.push(pendingChange);
}
});
} else {
const pendingChange = this.prepareChangeType(command, inactiveCommandCount, i);
if (pendingChange) {
activeChanges.push(pendingChange);
for (const subCommand of subCommands) {
await this.handleCommand(subCommand, inactiveCommandCount, i);
}
} else {
await this.handleCommand(command, inactiveCommandCount, i);
}
} catch (error) {
Log.error('CPE: Change creation Failed', getError(error));
}
});

activeChanges = activeChanges.filter((change): boolean => !!change);
}

if (Array.isArray(allCommands) && allCommands.length === 0) {
this.pendingChanges = [];
await this.fetchSavedChanges();
}

this.updateStack(activeChanges);
this.updateStack();
handleStackChange();
};
}

private prepareChangeType(
private async handleCommand(command: FlexCommand, inactiveCommandCount: number, index: number): Promise<void> {
const pendingChange = await this.prepareChangeType(command, inactiveCommandCount, index);
if (pendingChange) {
this.pendingChanges.push(pendingChange);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be an issue with pending changes duplicating on each event. It is probably because pendingChanges field is not reset and we always process all the changes in the stack.

}
}

private async prepareChangeType(
command: FlexCommand,
inactiveCommandCount: number,
index: number
): PendingChange | undefined {
): Promise<PendingChange | undefined> {
let result: PendingChange;
let value = '';
const selectorId = this.getCommandSelectorId(command);

const change = command.getPreparedChange();

const selectorId = await this.getControlIdByChange(change);
const changeType = this.getCommandChangeType(command);

if (!selectorId || !changeType) {
Expand All @@ -315,7 +341,8 @@ export class ChangeService {
value = command.getProperty('newBinding') as string;
break;
}
const { fileName } = command.getPreparedChange().getDefinition();

const { fileName } = change.getDefinition();
if (changeType === 'propertyChange' || changeType === 'propertyBindingChange') {
result = {
type: 'pending',
Expand Down Expand Up @@ -380,18 +407,79 @@ export class ChangeService {
}

/**
* Get command selector id.
* Get element id by change.
*
* @param command to be executed for creating change
* @returns command selector id or undefined
* @param change to be executed for creating change
* @returns element id or empty string
*/
private getCommandSelectorId(command: FlexCommand): string | undefined {
return this.retryOperations([
() => command.getSelector().id,
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
() => command.getElement().getProperty('persistencyKey'),
() => command.getElement().getId(),
() => command.getParent()?.getElement().getId()
]) as string | undefined;
private async getControlIdByChange(change: FlexChange): Promise<string> {
const appComponent = this.options.rta.getRootControlInstance();
const selector = change.getSelector();
const changeType = change.getChangeType();
const layer = change.getLayer();

try {
let control = JsControlTreeModifier.bySelector(selector, appComponent);
if (!control) {
return selector.id;
}

const changeHandlerAPI = (await import('sap/ui/fl/write/api/ChangesWriteAPI')).default;

const changeHandler = await changeHandlerAPI.getChangeHandler({
changeType,
element: control,
modifier: JsControlTreeModifier,
layer
});

if (changeHandler && typeof changeHandler.getChangeVisualizationInfo === 'function') {
const result: { affectedControls?: [string] } = await changeHandler.getChangeVisualizationInfo(
change,
appComponent
);
return JsControlTreeModifier.getControlIdBySelector(
result?.affectedControls?.[0] ?? selector,
appComponent
);
}

return JsControlTreeModifier.getControlIdBySelector(selector, appComponent);
} catch (error) {
Log.error('Getting element ID from change has failed:', getError(error));
return selector.id;
}
}

/**
* Sync outline changes to place modification markers when outline is changed.
*
* @returns void
*/
public async syncOutlineChanges(): Promise<void> {
for (const file of this.changedFiles) {
const flexObject = await this.getFlexObject(file);
const savedChange = this.savedChanges.find((change) => change.fileName === file.fileName);
if (savedChange) {
savedChange.controlId = await this.getControlIdByChange(flexObject);
}
}
this.updateStack();
}

/**
* Get FlexObject from change object based on UI5 version.
*
* @param change change object
* @returns FlexChange
*/
private async getFlexObject(change: object): Promise<FlexChange> {
if (isLowerThanMinimalUi5Version(await getUi5Version(), { major: 1, minor: 109 })) {
const Change = (await import('sap/ui/fl/Change')).default;
return new Change(change);
}

const FlexObjectFactory = (await import('sap/ui/fl/apply/_internal/flexObjects/FlexObjectFactory')).default;
return FlexObjectFactory.createFromFileContent(change) as FlexChange;
}
}
2 changes: 1 addition & 1 deletion packages/preview-middleware-client/src/cpe/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export default function init(
const changesService = new ChangeService({ rta }, selectionService);
const connectorService = new WorkspaceConnectorService();
const rtaService = new RtaService(rta);
const outlineService = new OutlineService(rta);
const outlineService = new OutlineService(rta, changesService);
const quickActionService = new QuickActionService(rta, outlineService, registries);
const services: Service[] = [
selectionService,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Log from 'sap/base/Log';
import type RuntimeAuthoring from 'sap/ui/rta/RuntimeAuthoring';
import type RTAOutlineService from 'sap/ui/rta/command/OutlineService';
import type { ChangeService } from '../changes/service';

import type { ExternalAction } from '@sap-ux-private/control-property-editor-common';
import { outlineChanged, SCENARIO, showMessage } from '@sap-ux-private/control-property-editor-common';
Expand All @@ -19,7 +20,7 @@ export interface OutlineChangedEventDetail {
* A Class of WorkspaceConnectorService
*/
export class OutlineService extends EventTarget {
constructor(private rta: RuntimeAuthoring) {
constructor(private rta: RuntimeAuthoring, private changeService: ChangeService) {
super();
}

Expand All @@ -41,6 +42,7 @@ export class OutlineService extends EventTarget {
const viewNodes = await outline.get();
const controlIndex: ControlTreeIndex = {};
const outlineNodes = await transformNodes(viewNodes, scenario, reuseComponentsIds, controlIndex);
await this.changeService.syncOutlineChanges();
voicis marked this conversation as resolved.
Show resolved Hide resolved

const event = new CustomEvent(OUTLINE_CHANGE_EVENT, {
detail: {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default {
bySelector: jest.fn(),
getControlIdBySelector: jest.fn()
}
Loading
Loading