Update infrastructure for changes of full qualfied instance names #811

oberlehner · 2024-12-02T12:53:53Z

No description provided.

github-actions · 2024-12-02T12:58:27Z

Test Results

111 files ±0 111 suites ±0 50s ⏱️ ±0s
29 162 tests ±0 29 162 ✅ ±0 0 💤 ±0 0 ❌ ±0
29 163 runs ±0 29 163 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit c59a4b8. ± Comparison against base commit 7e46e08.

♻️ This comment has been updated with latest results.

mx990

I may have a simpler solution for handling qualified name changes in the listener:

collect all execute/undo/redo changes in a single list,
when performing the changes, first merge all changes for the same element in order.

This way, we should be able to avoid keeping a separate history for changes in addition to the command stack. What do you think?

mx990 · 2025-01-09T12:27:32Z

...c.ide.model.commands/src/org/eclipse/fordiac/ide/model/commands/QualNameAffectedCommand.java

+import org.eclipse.fordiac.ide.model.typelibrary.TypeEntry;
+import org.eclipse.gef.commands.Command;
+
+public abstract class QualNameAffectedCommand extends Command implements ScopedCommand {


I would suggest changing this into an interface to make it easier to add to an existing class hierarchy of commands.

mx990 · 2025-01-09T12:30:43Z

...c.ide.model.commands/src/org/eclipse/fordiac/ide/model/commands/QualNameAffectedCommand.java

+
+	protected abstract String getNewQualName();
+
+	protected abstract INamedElement getNotifier();


I would suggest renaming this to getRenamedElement (or something similar) to make it more clear what is being returned.

mx990 · 2025-01-09T12:33:05Z

...c.ide.model.commands/src/org/eclipse/fordiac/ide/model/commands/QualNameAffectedCommand.java

+		Assert.isTrue(rootContainer instanceof LibraryElement);
+		if (rootContainer instanceof final LibraryElement element) {


I believe having another instanceof check after an assertion makes little sense. I would suggest removing the assertion.

mx990 · 2025-01-09T12:35:21Z

...ac.ide.model.commands/src/org/eclipse/fordiac/ide/model/commands/QualNameChangeListener.java

+import org.eclipse.fordiac.ide.model.libraryElement.INamedElement;
+import org.eclipse.fordiac.ide.model.typelibrary.TypeEntry;
+
+public abstract class QualNameChangeListener {


Please extract the implementation into a separeate class and change this into an interface.

mx990 · 2025-01-09T12:36:53Z

...model.commands/src/org/eclipse/fordiac/ide/model/commands/QualNameChangeListenerManager.java

+					listeners.add((QualNameChangeListener) obj);
+				}
+			} catch (final Exception e) {
+				e.printStackTrace();


Please properly log the exception (including the contributing plugin).

mx990 · 2025-01-09T12:41:00Z

...model.commands/src/org/eclipse/fordiac/ide/model/commands/QualNameChangeListenerManager.java

+			if (typeEntry != null) {
+				notifiyCommit(typeEntry);
+			}
+		}


What about POST_FLUSH (e.g., because a reload of the editor)?

mx990 · 2025-01-09T12:43:16Z

...model.commands/src/org/eclipse/fordiac/ide/model/commands/QualNameChangeListenerManager.java

+	}
+
+	private static TypeEntry getTypeEntryKeyFromCommandStack(final CommandStack stack) {
+		final IEditorPart currentActiveEditor = EditorUtils.getCurrentActiveEditor();


I believe this does not handle the case where multiple editors are saved at once (e.g., Save All)? I would suggest adding a dedicated listener instance per editor to keep track of the type entry.

mx990 · 2025-01-09T12:47:35Z

....ide.model.commands/src/org/eclipse/fordiac/ide/model/commands/change/ChangeNameCommand.java

 		this.element = Objects.requireNonNull(element);
 		this.name = name;
 		this.validateName = validateName;
+		this.oldName = element.getQualifiedName();


The oldName is supposed to be the (unqualified) name of the element and is assigned in execute().

mx990 · 2025-01-09T12:51:06Z

....ide.model.commands/src/org/eclipse/fordiac/ide/model/commands/change/ChangeNameCommand.java

+		final String[] split = getOldQualName().split("\\."); //$NON-NLS-1$
+		final StringBuilder sb = new StringBuilder();
+		for (int i = 0; i < split.length; i++) {
+			if (i == split.length - 1) {
+				sb.append(name);
+			} else {
+				sb.append(split[i]);
+				sb.append("."); //$NON-NLS-1$
+			}
+		}
+		return sb.toString();


Suggested change

final String[] split = getOldQualName().split("\\."); //$NON-NLS-1$

final StringBuilder sb = new StringBuilder();

for (int i = 0; i < split.length; i++) {

if (i == split.length - 1) {

sb.append(name);

} else {

sb.append(split[i]);

sb.append("."); //$NON-NLS-1$

}

}

return sb.toString();

return getOldQualName().substring(0, getOldQualName().length() - oldName.length()) + name;

mx990 · 2025-01-09T12:54:57Z

plugins/org.eclipse.fordiac.ide.hierarchymanager.ui/META-INF/MANIFEST.MF

@@ -19,7 +19,10 @@ Require-Bundle: org.eclipse.core.resources,
 org.eclipse.fordiac.ide.ui,
 org.eclipse.fordiac.ide.typemanagement,
 org.eclipse.ui.ide,
- org.eclipse.ui.views.properties.tabbed
+ org.eclipse.emf.mwe.core,


Do we actually need org.eclipse.emf.mwe.core as a dependency?

oberlehner force-pushed the updateInfra branch 6 times, most recently from 0ec6021 to 797d2c9 Compare December 10, 2024 08:14

oberlehner force-pushed the updateInfra branch 5 times, most recently from 666971a to 2018064 Compare December 17, 2024 10:39

oberlehner force-pushed the updateInfra branch 2 times, most recently from ef4d556 to f64ae37 Compare December 17, 2024 16:00

oberlehner force-pushed the updateInfra branch 5 times, most recently from 281f523 to 1b159c0 Compare January 9, 2025 08:07

oberlehner force-pushed the updateInfra branch from 1b159c0 to 5c9a596 Compare January 9, 2025 10:56

oberlehner added 3 commits January 9, 2025 12:54

Extension point for listening on qualified name changes

e847ed1

Instantiated QualNameChangeListenerManager in Editors

f7375a0

Implemented the QualNameChangeListener for updating plant hierarchy

c59a4b8

oberlehner force-pushed the updateInfra branch from 5c9a596 to c59a4b8 Compare January 9, 2025 11:57

oberlehner marked this pull request as ready for review January 9, 2025 11:58

oberlehner requested review from mx990, azoitl and pazi146 January 9, 2025 11:58

oberlehner changed the title ~~Draft for update infra of plant hier~~ Update infrastructure for changes of instance names Jan 9, 2025

oberlehner changed the title ~~Update infrastructure for changes of instance names~~ Update infrastructure for changes of full qualfied instance names Jan 9, 2025

mx990 suggested changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update infrastructure for changes of full qualfied instance names #811

Update infrastructure for changes of full qualfied instance names #811

oberlehner commented Dec 2, 2024

github-actions bot commented Dec 2, 2024 •

edited

Loading

mx990 left a comment

mx990 Jan 9, 2025

mx990 Jan 9, 2025

mx990 Jan 9, 2025

mx990 Jan 9, 2025

mx990 Jan 9, 2025

mx990 Jan 9, 2025

mx990 Jan 9, 2025

mx990 Jan 9, 2025

mx990 Jan 9, 2025

mx990 Jan 9, 2025


		protected abstract String getNewQualName();

		protected abstract INamedElement getNotifier();

		Assert.isTrue(rootContainer instanceof LibraryElement);
		if (rootContainer instanceof final LibraryElement element) {

Update infrastructure for changes of full qualfied instance names #811

Are you sure you want to change the base?

Update infrastructure for changes of full qualfied instance names #811

Conversation

oberlehner commented Dec 2, 2024

github-actions bot commented Dec 2, 2024 • edited Loading

Test Results

mx990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 2, 2024 •

edited

Loading