Fixes #671 - Move some platform-ui undo utility classes into a core location #672

robstryker · 2023-09-08T20:44:04Z

No description provided.

robstryker · 2023-09-08T20:44:34Z

See #671 for description

mickaelistria · 2023-09-11T07:16:32Z

Couldn't those be in an internal but exported with x-friends package?

robstryker · 2023-09-12T14:11:01Z

Couldn't those be in an internal but exported with x-friends package?

In theory, yes, but, I personally draw the line between what I consider to be major repositories. I can understand if platform core wanted a bundle to be xfriends with something in platform ui, but once you cross the road into JDT-land, it seems that it should be publicly available API.

Basically, once a major consumer is attempting to use it, there's no point to keeping it as xfriends. It should be supported as public.

github-actions · 2023-09-12T14:36:02Z

Test Results

      40 files -     2       40 suites - 2 51m 31s ⏱️ - 11m 13s
  3 775 tests ±    0   3 770 ✔️ -     2   3 💤 ±0 1 ❌ +1 1 🔥 +1
11 181 runs - 147 11 152 ✔️ - 149 27 💤 ±0 1 ❌ +1 1 🔥 +1

For more details on these failures and errors, see this check.

Results for commit 75fb0ca. ± Comparison against base commit 0613f93.

♻️ This comment has been updated with latest results.

mickaelistria · 2023-09-13T08:59:09Z

I'm a bit worried about the names of all that. For example having a "ProjectDescription" API when there is already IProjectDescription as a main object is likely to bring confusion. I'm also not sure about the "descriptors" package which doesn't make it clear what these classes are intended to.
Can you please also show draft PRs for Platform UI and JDT to see how this change would impact consumers?

mickaelistria · 2023-09-13T09:01:22Z

...ipse.core.resources/src/org/eclipse/core/resources/descriptions/IFileContentDescription.java

+import org.eclipse.core.runtime.CoreException;
+
+/**
+ * IFileContentDescription is a description of a file's content.


Why not using directly IFile or IFileInfo for that?

mickaelistria · 2023-09-13T09:06:10Z

OK, I start to understand it better that those classes are meant to get a local snapshot of a resource to store in the undo/redo stack and process later. If there are no other consumers than undo/redo, then we can probably just show undo in the package name, and instead of "Descriptions" name them "Snapshots".
Couldn't we also move in here that bits that take care of undo/restoring (ie applying the "snapshot" to the workspace) ?

robstryker · 2023-09-13T14:27:33Z

I'm a +1 on changing package name to include an undo. I trhink that would add a lot of clarity.

I'm generally against renaming the classes to 'Snapshots' unless there's a wider consensus that's the correct thing to do. Changing package name is one thing but changing the class name, well, I'm not sure. I'll do it if demanded, but...

For JDT, the patch to migrate from use of the ui.ide.undo classes to the new core ones would be as simple as changing imports (assuming we don't change the class name, just the package). This is because our primary entry point to creating these objects is by calling ResourceDescription.fromResource(rootResource);.

For eclipse ui, I'd need to investigate more deeply to see how extensively these classes are used. I can see that org.eclipse.ui.ide.undo, is an exported package that is public API. Migration would probably involve having existing classes extend those in core.

mickaelistria

That looks good overall, just a minor comment inline that we need to address before moving forward.

mickaelistria · 2023-09-13T15:47:32Z

...se.core.resources/src/org/eclipse/core/resources/undo/snapshot/AbstractResourceSnapshot.java

+ * Base implementation of ResourceSnapshot that describes the common attributes
+ * of a resource to be created.
+ *
+ * This class is not intended to be instantiated or used by clients.


We should maybe remove this line?

This line should not be removed. The approved way of accessing these classes is by calling ResourceSnapshot.fromResource(r);

So couldn't the constructors be only package-visible? And the documentation mention that the right approach to get an instance if ResourceSnapshot.fromResource() ?

mickaelistria · 2023-09-15T15:13:15Z

The test failure is also visible on master and doesn't relate to the change (ie actually tests the content of help for last I-Build, not the one that would be generated by this PR).
While I still think we should aim at moving the extension point to make it easier for consumers to use it, I think it's not too bad to have its description in the "legacy" bundle at the moment. So, unless we find a quick and easy solution to the problem (which are yet unclear to me), it would be OK to merge it.

However, I still miss the exact symptom of the problem we're trying to fix. @robstryker do you have a link to some failure somewhere? If it's the current test that we see failing here that we're trying to fix, then the solution can be simply to list the "new" bundle in https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/725cacd808359189e38bda85b416ed80d03bda22/eclipse.platform.common/bundles/org.eclipse.platform.doc.isv/buildDoc.xml#L54

robstryker · 2023-09-15T16:06:02Z

I'll be refactoring this patch to ensure that org.eclipse.ui.ide can properly consume these in an api-compatible fashion. Please hold off.

mickaelistria · 2023-09-15T16:22:18Z

Please hold off.

OK, I'm converting to draft in the meantime.

robstryker · 2023-09-15T19:10:37Z

The test failure is also visible on master and doesn't relate to the change (ie actually tests the content of help for last I-Build, not the one that would be generated by this PR). While I still think we should aim at moving the extension point to make it easier for consumers to use it, I think it's not too bad to have its description in the "legacy" bundle at the moment. So, unless we find a quick and easy solution to the problem (which are yet unclear to me), it would be OK to merge it.

However, I still miss the exact symptom of the problem we're trying to fix. @robstryker do you have a link to some failure somewhere? If it's the current test that we see failing here that we're trying to fix, then the solution can be simply to list the "new" bundle in https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/725cacd808359189e38bda85b416ed80d03bda22/eclipse.platform.common/bundles/org.eclipse.platform.doc.isv/buildDoc.xml#L54

I think this comment was meant to be posted to a different issue. This issue here is not prompted by any test failures.

robstryker · 2023-09-15T19:13:35Z

I have significantly refactored the proposed patch, to add an internal package as the current ui.ide had done. This was an oversight on my part since only 1 of the classes should have been public.

In order to maintain API compatibility with prior consumers of the ui.ide code, I have chosen to make use of interfaces instead of only abstract classes. This allows the existing UI code to simply wrap the core implementation and implement the same interface. This will maintain compatability with all of the existing classes and the expected return values. More of that can be discussed on an issue in the UI repository when appropriate.

…es into a core location Signed-off-by: Rob Stryker <stryker@redhat.com> Update copyright headers and since tags Signed-off-by: Rob Stryker <stryker@redhat.com> Rename API to use snapshot instead of description for clarity Signed-off-by: Rob Stryker <stryker@redhat.com> Missing since tag Signed-off-by: Rob Stryker <stryker@redhat.com> Reorganize newly moved classes and clean up Signed-off-by: Rob Stryker <stryker@redhat.com>

mickaelistria · 2023-09-19T18:11:36Z

....eclipse.core.resources/src/org/eclipse/core/resources/undo/snapshot/IContainerSnapshot.java

+ * IContainerSnapshot is a lightweight description that describes a container to
+ * be created.
+ *
+ * This class is not intended to be instantiated by clients.


This line can be removed as interfaces cannot be instantiated.
However, 1 important thing to settle when introducing this interface API is whether we want to allow other implementations? If we want to restrict the implementations to the ones that are shipped here, we should add a @noimplements tag to make PDE warn in case someone wants to implement it.
It's safer to start with as many restrictions as possible and to remove them later than the other way round, as it gives more freedom for further changes.

mickaelistria · 2023-09-19T18:11:54Z

...org.eclipse.core.resources/src/org/eclipse/core/resources/undo/snapshot/IMarkerSnapshot.java

+ * IMarkerSnapshot is a lightweight snapshot of a marker for the purposes of
+ * undoing.
+ *
+ * This class is not intended to be instantiated by clients.


Same as above.

mickaelistria · 2023-09-19T18:12:27Z

...g.eclipse.core.resources/src/org/eclipse/core/resources/undo/snapshot/IResourceSnapshot.java

+ * IResourceSnapshot is a lightweight snapshot that describes the common
+ * attributes of a resource to be created.
+ *
+ * This class is not intended to be extended by clients.


same as above

Signed-off-by: Rob Stryker <stryker@redhat.com>

ruspl-afed · 2023-09-20T06:11:39Z

...g.eclipse.core.resources/src/org/eclipse/core/resources/undo/snapshot/IResourceSnapshot.java

+	 *            the progress monitor to be used when creating the resource
+	 * @throws CoreException if creation failed
+	 */
+	public void createExistentResourceFromHandle(IResource resource,


I understand that my comment is late, but create method usually returns an instance of something (like the method above). Do you think something like "materialize" or "actualize" could be better here @robstryker @mickaelistria ?

I agree it looks like this particular thing can be improved. I'm also curious about why is this method useful when we already have createResource. @robstryker in which case would one want to call it instead of createResource? What about a new restoreResource that would join both?

I'm also thinking that we could maybe add generics <T extends IResource> here and there to polish things a bit and facilitate consumption.

generics are proposed in #705

mickaelistria reviewed Sep 13, 2023

View reviewed changes

robstryker force-pushed the issue671 branch from 6b7ace5 to f6cf377 Compare September 14, 2023 14:20

mickaelistria marked this pull request as draft September 15, 2023 16:21

robstryker force-pushed the issue671 branch from 329925a to 60cc99a Compare September 15, 2023 19:04

robstryker marked this pull request as ready for review September 18, 2023 17:18

robstryker force-pushed the issue671 branch from 60cc99a to d48570d Compare September 18, 2023 19:13

robstryker force-pushed the issue671 branch from d48570d to 1607f82 Compare September 19, 2023 13:21

mickaelistria requested changes Sep 19, 2023

View reviewed changes

Add noimplement and noextend annotations

75fb0ca

Signed-off-by: Rob Stryker <stryker@redhat.com>

robstryker force-pushed the issue671 branch from 1607f82 to 75fb0ca Compare September 19, 2023 19:09

mickaelistria approved these changes Sep 19, 2023

View reviewed changes

mickaelistria merged commit df661fe into eclipse-platform:master Sep 19, 2023
12 of 14 checks passed

ruspl-afed reviewed Sep 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #671 - Move some platform-ui undo utility classes into a core location #672

Fixes #671 - Move some platform-ui undo utility classes into a core location #672

robstryker commented Sep 8, 2023

robstryker commented Sep 8, 2023

mickaelistria commented Sep 11, 2023

robstryker commented Sep 12, 2023

github-actions bot commented Sep 12, 2023 •

edited

Loading

mickaelistria commented Sep 13, 2023

mickaelistria Sep 13, 2023

mickaelistria commented Sep 13, 2023

robstryker commented Sep 13, 2023

mickaelistria left a comment

mickaelistria Sep 13, 2023

robstryker Sep 14, 2023

mickaelistria Sep 14, 2023

mickaelistria commented Sep 15, 2023

robstryker commented Sep 15, 2023

mickaelistria commented Sep 15, 2023

robstryker commented Sep 15, 2023

robstryker commented Sep 15, 2023

mickaelistria Sep 19, 2023

mickaelistria Sep 19, 2023

mickaelistria Sep 19, 2023

ruspl-afed Sep 20, 2023

mickaelistria Sep 20, 2023

mickaelistria Sep 20, 2023

mickaelistria Sep 20, 2023

Fixes #671 - Move some platform-ui undo utility classes into a core location #672

Fixes #671 - Move some platform-ui undo utility classes into a core location #672

Conversation

robstryker commented Sep 8, 2023

robstryker commented Sep 8, 2023

mickaelistria commented Sep 11, 2023

robstryker commented Sep 12, 2023

github-actions bot commented Sep 12, 2023 • edited Loading

Test Results

mickaelistria commented Sep 13, 2023

Choose a reason for hiding this comment

mickaelistria commented Sep 13, 2023

robstryker commented Sep 13, 2023

mickaelistria 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

mickaelistria commented Sep 15, 2023

robstryker commented Sep 15, 2023

mickaelistria commented Sep 15, 2023

robstryker commented Sep 15, 2023

robstryker commented Sep 15, 2023

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 Sep 12, 2023 •

edited

Loading