Community
Participate
Working Groups
When you open the JPA Diagram Editor for the first time while you are using some versioning control system like CVS, GIT, SVN ect. the diagram.xmi is marked as changed. The solution that we made was to move the diagram.xmi to the project's .metadata folder on the file system and to use simple diagram.ini file to store the necessary data for the diagram editor such as: entity name and its coordnates on the diagram. This .ini file will be saved only when the editor is saved...
Created attachment 195219 [details] proposed patch
Neil, does it make sense to propose this one for PMC review? Do you think it's important enough?
Changing the metadata in this way right before the release seems like a very big change to be making at this point, and I would think carries a decent amount of risk. That said, it is good when these types of changes are resolved before they are released so that the metadata doesn't end up changing from release to release. What is the real impact of the bug on users if the bug is not fixed?
Every time when the user opens a saved diagram, the corresponding xmi file is being changed even though the user didn't make any changes in the diagram. It's a behavior dictated by the Graphiti framework. When the user keeps the xmi file in some versioning control system along with the JPA project and when she checks out the project and opens the diagram, the xmi file is being changed and shown as different from the one in the version control system. The solution we've found is to keep the xmi file in the preference store. This file is needed only while the diagram stays open. We don't need it to restore the diagram when it opens, so we don't need to be kept in the VC system. Although, we still need some small part of the info saved in it to restore the diagram. The info we need is just the names of the visualized entities and their coordinates in the diagram. With this fix when the diagram closes we save the necessary info in a simple ini file. This file is part of the JPA project and could be kept in the VC system without the annoying deffect described here. The change became bigger than we expected and maybe it's too risky to submit it before the release.
I asked David Williams for his thoughts on this issue and he was of the general opinion that it would be worth it to try and fix this now to avoid later changes in metadata, and based on the severity of the problem. I've cc'd him so he could add any comments regarding that opinion. I suppose the main question now is how confident you are in the proposed solution. It will be time consuming to make changes to this metadata format after the release so it is important to try and get this right the first time. If you are confident in the changes and the new format then I am prepared to approve and send up for PMC review. In general David and I both agree that it is an important bug to fix, but we want to be careful to get the right solution. Please fill out the questionnaire when you are ready to submit for PMC approval.
I still haven't had the time to test it, but tomorrow or at friday I will be ready to answer your question
Found a glitch: 1. Create JPA project and diagram 2. Close the diagram 3. Delete diagram ini file from the project explorer 4. Open the diagram again Result: The diagram appears as it was saved the last time. It should be empty instead of this. Otherwise, the patch seems ok. P.S.: Please, remove all the validation warnings before submitting the new version
Created attachment 195500 [details] Patch proposal Please try this one. It should cover this case...
Created attachment 195502 [details] patch proposal Previous patch did not work, but this one should work fine...
Created attachment 195564 [details] patch proposal The previous patch became incompatible with the HEAD after the patch from https://bugs.eclipse.org/bugs/show_bug.cgi?id=338960 was being commited...
Created attachment 195584 [details] Read entity coordinates from ini file New version - the patch is ok with just a glitch in it.
This is an important fix, for every JPA developer which uses some kind of version control system, that's why it needs to get into Indigo. The workaround is not to submit the diagram file when the diagram was not change, but anyway it's still confusing for the user. The diagram xmi file is no longer in the project - it's in the preference store. Instead of it there is a an ini file in the 'diagrams' folder of the project. The fix was tested manually. I've checked all the possible scenarios including the one with the old style - if the user has a diagram and opens it in IDE with this fix, the editor will open the diagram and when saved the xmi file will disappear from the project and get to the prefs store. Instead of it a new ini file will appear. All the existing JUnit tests are passing successfully. There is more detailed description of the problem and the solution in comment #4. I've reviewed the fix and it's ok. The risk is low.
I think bug will be headed for RC2 at this point.
ok
I'm all in favor of fixing this for Indigo, since, as you say, it's really bad to change "shared files" if they really should not be shared. But, I couldn't apply the patch, so hard for me to look at code very closely. The reason I was going to look at it closely was because from glancing at the patch, it seems that every method is "public". Even ones like getIniFile(). Normally that's something you wouldn't want to expose to the world, but to keep it private, or at least package protected ... if possible, and I don't know if its possible since I can't really see the code in context. I also found it odd that getIniFile was a static method ... guess I don't know that its for. Another technical comment, I noticed statements such as File newXMIFile = new File(newXMIFilePath.toOSString()); which implies to me you are using "raw" java methods, instead of Eclipse's API for preferences or metadata. Is that common? Seems there's some potential to be restrictive ... such as if/when people start to use a "lined resource" for their project, or some other type of of (future) indirection. So, don't get me wrong ... it's probably all fine, and good reasons for everything ... just making some comments based on casual scan of the patch file. Can't say I know (easily) the "right" way to do anything ... just making observations. Does seem fair for me to ask for a patch I could apply, though ... but, maybe others can apply it? Maybe its just my workspace? (in which case, no big deal). Thanks for catching/fixing this before Indigo.
Sorry for typo, in above comment, where I said "lined resource" I mean "linked resource".
Yes David, you are right that most of the methods shouldn't be "public"... According to getIniFile() - it is invoked by several other classes which are in different packages and is marked as static because of the "iniFile" field, which is static since it is called by another static method from the same class... Actually all methods in the ModelIntegrationUtil.class are static, but I will remove it since I'm trying to refactor the patch in a way to use IFileStore instead of java.io.File. So what do you think, will be fine, if I am using this appraoch?
(In reply to comment #17) > ... since I'm trying to refactor the patch in a way to use > IFileStore instead of java.io.File. > So what do you think, will be fine, if I am using this appraoch? yes, does sound probably ok ... but, be warned :) ... I'm not saying I know what the right design is ... just making observations. (Such, as, I'm do not know if IFileStore can be used in a ".metadata" location ... as far as I know, that's usually done with "preference" APIs ... but, I emphasize, I do not know (I'd need to study up, to refresh my memory). Doubt if there's time for you to do too much studying, but seems this problem would have been solved before, and perhaps some good patterns used, such as VE, WindowBuider, or similar "visual designers". Of course, they might have used bad patterns too :) hard to know. I think fine if you focus on main issue of "getting out of source control" and can always improve in later releases ... and the more "private" you can make the methods, the easier it will be to change in later releases :) But, overall, important work. Thanks for the careful effort.
Created attachment 196100 [details] read entity coordinates from xml file Ok, so in this patch I was trying to reduce the use of java.io.File, the existing .xmi file is moved to the .metadata on the filesystem (i was getting the .metadata location in this way: JPADiagramEditorPlugin.getDefault().getStateLocation()) and the necessary entities' coordinates are stored and read in a xml format in a new .xml file instead of .ini file...
Patch committed in RC2
