Community
Participate
Working Groups
In order to pass RFRS by Instantiations WindowBuilder product it should contain no any of internal API usage. The functionality of org.eclipse.jdt.internal.corext.refactoring.nls.changes.CreateTextFileChange can't be implemented using public API, please move it into public scope.
Can you explain why CreateTextFileChange is valuable? I just did some reference searching and there are no users of getCurrentContent(). So there is not much value left on this class.
Yes, but the rest of functionality from base classes is valuable.
ok, so I chaneg it to 'make CreateFileChange API'
We usually only provide API that is also used (and hence tested) in the Eclipse SDK, so the existing usages of CreateFileChange and CreateTextFileChange should be replaced by references to the new API. Note that there's also a CreateTextFileChangePreviewViewer, which is used to show the contents of the new file in a refactoring UI. That implementation currently hardcodes "java" and "properties" file types -- this should be generalized to use the org.eclipse.compare.contentViewers extension point, like the TextEditChangePreviewViewer already does via CompareUI.findContentViewer(..).
Here is a patch that contains a CreateFileChange that I used for project migration using LTK refactoring: https://github.com/kaloyan-raev/eclipse.jdt.ui/commit/f198a1023d510180c37ee9ec60d31cec7586cc89 I cannot find the jdt.ui project in Eclipse Gerrit and therefore I am using a fork on GitHub. The implementation is very basic - it creates a new file on the specified path and the file content is read from an input stream. There is also an "overwrite" flag which determines if an existing file on the specified path should be overwritten, or the change validation should fail. One open question is what should happen if the file path points to non-existing folder. Should the necessary folder structure be created automatically, or determined by another flag, or should we have instead CreateFolderChange? I also did not implement a ChangeDescriptor because I don't really understand how this works. Let me know how can I improve the patch so it becomes part of the LTK project.
(In reply to comment #5) > Here is a patch that contains a CreateFileChange that I used for project > migration using LTK refactoring: > https://github.com/kaloyan-raev/eclipse.jdt.ui/commit/ > f198a1023d510180c37ee9ec60d31cec7586cc89 > > I cannot find the jdt.ui project in Eclipse Gerrit and therefore I am using > a fork on GitHub. We're in the process of enabling Git for JDT UI. Please attach a patch to this bug as we are not grabbing the stuff from GitHub. Please also read http://wiki.eclipse.org/Development_Resources/Contributing_via_Git#Contributing_via_Bugzilla Thanks.
Created attachment 234447 [details] patch v1 Here is the patch attached. This contribution complies with http://www.eclipse.org/legal/CoO.php
(In reply to comment #5) > One open question is what should happen if the file path points to > non-existing folder. Should the necessary folder structure be created > automatically, or determined by another flag, or should we have instead > CreateFolderChange? Creating parent folders for free is the way to go. > I also did not implement a ChangeDescriptor because I don't really > understand how this works. You don't need a ChangeDescriptor. This is only relevant for the top-level Change returned by a refactoring, where it provides a connection to the refactoring descriptor, which is used to record/replay the refactoring history. > Let me know how can I improve the patch so it becomes part of the LTK > project. See comment 4.
(In reply to comment #8) > Creating parent folders for free is the way to go. OK, I will add this logic. > > Let me know how can I improve the patch so it becomes part of the LTK > > project. > > See comment 4. As far as I get it, we need also a CreateTextFileChange that extends CreateFileChange, but can create file from String and can handle encoding options. We also need a CreateTextFileChangePreviewViewer. I'd like to keep CreateFileChange as it is in patch v1 - having an InputStream for the file content and no preview. This way non-text file can be created and also text files where preview does not make sense, e.g. minified JavaScript library. If anyone wants to have more options for creating text files and have a preview then the CreateTextFileChange should be used. Sounds good?
> having an InputStream for the file content and no preview Oh, I missed that difference so far. I found a similar ad-hoc implementation in JDT UI in org.eclipse.jdt.internal.ui.text.correction.UnresolvedElementsSubProcessor#addCopyAnnotationsJarProposal(..), and I verified it works fine when replacing it with your CreateFileChange. TODOs: - Change the constructor to take an IFile instead of an IPath. I know that other ResourceChange classes also take an IPath, but that was a mistake. - Add tests to org.eclipse.ltk.core.refactoring.tests.resource.ResourceRefactoringTests: - with/without overwrite - with inaccessible destination (e.g. containing project doesn't exist)
Created attachment 234481 [details] patch v2 In this patch: - Constructor takes IFile instead of IPath - Parent folders are created for free if necessary - Tests added I need advice for two things: 1) What should be the value of the force flag when creating and deleting files and folders? 2) What should happen if the IFile's project does not exists? It's easy to decide on folders, but what about the project? If it should be created - what type of project? Or is it better to check this and return fatal error during validation? I will work on the CreateTextFileChange and the preview viewer next week.
(In reply to comment #11) > 1) What should be the value of the force flag when creating and deleting > files and folders? Force should be set when creating the IFile with fOverwrite == true. Everywhere else, it should not be set. > 2) What should happen if the IFile's project does not exists? It's easy to > decide on folders, but what about the project? If it should be created - > what type of project? Or is it better to check this and return fatal error > during validation? isValid(..) should return a fatal error. The constructor's Javadoc should tell that the file's project must exist. > I will work on the CreateTextFileChange and the preview viewer next week. That would be nice for others waiting for this API, but it's no longer required to get your CreateFileChange (with InputStream) in.
Created attachment 234533 [details] patch v3 In this new patch: 1) Force flag: if overwrite is on then a composite change of DeleteResourceChange and CreateFileChange is performed. The force flag is true on the DeleteResourceChange. Everywhere else the flag is false 2) Project existence: I checked that isValid(...) already checks this case: fFile.getLocationURI() is null if the project does not exist and a fatal error is returned. I added a test for this case. Javadoc is added too. I started to work on the CreateTextFileChange that will extend CreateFileChange (not in this patch yet). This may involve changes in the base CreateFileChange class. I am thinking about the "stampToRestore" parameter of CreateTextFileChange. Perhaps, it's generic enough to be pushed down to the base CreateFileChange class. What do you think about this? And how about the "encoding" parameter? It's required for creating an input stream from the source string, but it is also passed to IFile.setCharset(encoding). Does it makes sense to have it also in the base CreateFileChange where the files is created only from an input stream?
Just to let you know that I could not finish the work on CreateTextFileChange this week and in the next two weeks I am on vacation. I will continue after it. Generalizing the CreateTextFileChangePreviewViewer appeared to be more complex than I expected.
