Revert IFile.create(...REPLACE) - fixes #1433 #1434
Conversation
Allows to use modifyRule for IFile.write() if the file already exits, so that every IFile.setContents() can be replaced with IFile.create(). eclipse-platform#1433
| setDerived(derived, null); | ||
| } | ||
| setContents(content, force, keepHistory, monitor); |
There was a problem hiding this comment.
Usually, methods in resource are supposed to be atomic (ie grouped in a single operation), here those 2 methods would lead to 2 operations side by side but not grouped.
What you can try to remediate that would be something like
if (derived == isDerived()) {
setContents(...);
} else {
SubMonitor progress = SubMonitor.convert(monitor, 100).checkCanceled();
// need to group operation
final ISchedulingRule groupRule = new MultiRule[] { workspace.getRuleFactory().derivedRule(this), workspace.getRuleFactory().modifyRule(this) };
try {
workspace.prepareOperation(rule, progress.split(1)); // locks
workspace.beginOperation(true);
setDerived(derived, progress.split(1));
setContents(content, force, keepHistory, progress.split(98))
} catch (OperationCanceledException e) {
workspace.getWorkManager().operationCanceled();
throw e;
} finally {
progress.done();
workspace.endOperation(rule, true); // this is extremely important of workspace is locked
}There was a problem hiding this comment.
API-wise it's totally fine. See nested comment about the workspace operation should be atomic.
There was a problem hiding this comment.
Thanks for the suggestion! I have tried your implementation and it would work but can not be done in default method of IFile due to references to internal methods. but based on the idea i think i have found a easier solution with less overhead. please review again. Junit also tests for atomic operations now.
There was a problem hiding this comment.
in default method of IFile due to references to internal methods.
There is nothing mandating to create a default method here. IFile is annotated as "noextends/noimplements", so we can add new interface methods in here and have the implementation in File.
There was a problem hiding this comment.
So far the therory. In practice there are other implementations even within the IDE (PDE's ContributionDataFile) :-(
There was a problem hiding this comment.
While it's an annoyance, I think keeping the resource code as maintainable as posible is more important than keeping e4 tools alive. IMO, it's best to go for a regular interface method and impl in File, and to adapt PDE then.
There was a problem hiding this comment.
sure, that would be good. But i have no idea why it is like that in PDE and how it could be solved. So as long as we can implement a default its much easier to proceed.
...lipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java
Outdated
Show resolved
Hide resolved
| @@ -414,7 +414,7 @@ public default void create(byte[] content, int createFlags, IProgressMonitor mon | |||
| * in sync with the local file system | |||
| * @param derived Specifying this flag is equivalent to atomically calling | |||
| * {@link IResource#setDerived(boolean)} immediately after | |||
| * creating the resource or non-atomically setting the | |||
| * creating the resource or atomically setting the | |||
There was a problem hiding this comment.
Given the pain introduced by "atomically setting the derived flag before setting the content of an already" which as far as I understand is not a very interesting case (derived usually matters at creation), maybe we should just restrict it and make it "createAsDerived" and ignore it when resource already exists.
There was a problem hiding this comment.
The single most important usage of this API still is org.eclipse.jdt.internal.core.builder.AbstractImageBuilder.writeClassFile(ClassFile, SourceFile, boolean) where the derived flag is updated also for existing files. Thats ancient code and i do not know why. But its legit.
As a compromise we could only change (i.e. set) derived flag if derived==true is given as argument. In that case we would also not need the REPLACE. I pushed a implemementation for that: "A value of false will not update the derived flag of an existing file."
| setDerived(derived, null); | ||
| } | ||
| setContents(content, force, keepHistory, monitor); |
There was a problem hiding this comment.
in default method of IFile due to references to internal methods.
There is nothing mandating to create a default method here. IFile is annotated as "noextends/noimplements", so we can add new interface methods in here and have the implementation in File.
|
@mickaelistria can you please review again - i would like to merge today |
There was a problem hiding this comment.
My previous comments still stand, and I even support it stronger: it's better to just adopt the usual pattern of decl in interface + impl in File to not be bothered with REPLACE in lower-level API, even if it requires changing PDE also. This will ultimately lead to better result.
The over-usage of the REPLACE field in the lower level FileSystemResourceManager is worrying me, it's not the right grain where to hook this complexity. This is IMO a kind of design leak from a higer-level utility method into its low-level backend, and we shouldn't allow that unneeded change in the backend just for introduction of a convenience method downstream.
@mickaelistria i already solved that as you recommended. Please read the code again. |
To be clear, I think that the introduction of
The thing is that I barely see your proposal here as a fix but more as some higher level concepts leaking into lower-level API, thus placing complexity at the wrong level which will utlimately hurt. |
ok, i moved the code from FileSystemResourceManager to File - it can be done with the default method to not produce error in PDE. |
Allows to use modifyRule for IFile.write() if the file already exits, so that every IFILE.setContents() can be replaced with IFile.create().
#1433