You need to log in before you can comment on or make changes to this bug.
The work on Ergonomics IDE[1] done so far reveals need more supported way of replacing a project with its proper substitute while the former is being opened. Here is a little enhancement that achieves that with the help of ProjectInitializationHook.
Created an attachment (id=74455) [details] API changes, test, documentation
Created an attachment (id=74456) [details] Simplified usage in ergonomics
MK1: -1 on the whole concept. Open project list is just intended for representing the list of opened projects, not mutate them. The ultimate source of project instances is ProjectManager. The instances of projects need to match or we will get different instance of project via OpenProjects and via FileOwnerQuery, causing trouble all over the IDE.
I don't like it either, we are adding one more possible source of problems during opening project phase. What is real performance benefit of using ProjectInitializationHook?
Re. MK1. Right, the Project instances provided by ProjectManager and OpenProjects shall match.To ensure that in the proposed system, I suggest to add assert p == ProjectManager.getDefault().findProject(p.getProjectDirectory()); after the calls to doInitializeProject. That imho addresses the possible inconsistency issue appropriately.
[JG01] This should not be a part of the public API. A friend API from the projectui module would be better. [JG02] "if (np == null) return np;" is poor style. Return null explicitly if that is what you mean. [JG03] What happens to projects with invalid metadata? Is the behavior of the Open Project dialog (i.e. to display the error) unchanged? [JG04] Why the change to ProjectOpenedHook.java, specifically why is this an @Override? [JG05] I don't follow how the proposed assertion to satisfy MK1 would be helpful. Even if true, there may have been other calls to PM.fP _prior_ to the project being opened. As I understand it (and I have not seen a very clear explanation of what FeatureProjectFactory is doing), this would mean that there would be two distinct active Project's in memory for the same project directory, which would break a very basic invariant in the project system and cause unknown problems. It seems to me that the simpler approach, which would require no API changes that I know of, would be for the FPF to create a proxy Project which would remain active for the remainder of the IDE session or GC, delegating all functions to the Lookup of the real Project once that is loaded. Of course anyone casting a Project instance to a specific type would find their code broken, but that has always (since 4.0) been threatened as a possibility; correct code using Lookup would not be affected. [JOKE01] Your patch assumes that p = np which has not yet been proven.
I would prefer any solution not to mess project instances read by ProjectManager. Unlike jglick, I don't think we can freely start using Project proxies, even though we warned in 4.0 we might. We've passed the point where we can actually enforce it IMHO. Apart from straight hacky code, like casting and reflection calls, there's also objects in lookup being passed the 'inner' project instance around, which could be eventually compared to a given 'outer' project instance. I believe i've seen it more than once in our codebase. The only save viable solution to me seems to have connections points in 1. new project wizard, 2. open project dialog and 3. possibly in ProjectFactory.isProject(). the last one is tricky and if FoD incorporates any UI to enable features, it could end up in randomly popping up dialogs as projects are found. And the project can actually be found at rather random occations. One example to demonstrate. on project popup, the version control actions perform FileOwnerQuery query on all subfolders of the current project's folder and if a project is found there, use the SharabilityQueryImplementation present in the child project's lookup for triggering enabled/disabled state, similarly they do when "Show changes" dialogs are shown. Without a proper (real) project backing up the location, we might end up in adding/committing user files that are not meant to be shared.
Thanks for your comments. If I understand correctly, we all see two options: 1. delegate and proxy 2. create hooks to trigger the right modules. Currently I am fully relying on option #1. I have proxy which delegates as much as possible to the real project, as soon as it is loaded. Things seem to work, just I am not really fond of the reflection used to call projectOpened() protected method and also I do not like that the instance returned from OpenProjects.getOpenProjects() is the fake one not the real one. I used to (and I still can if necessary) call OpenProjects.close and OpenProjects.open to replace the instances, however that seems to me too dynamic, stateful and errorprone. That is why I create the patch http://www.netbeans.org/nonav/issues/showattachment.cgi/74455/api.diff - it adds the only one missing (we have project wizards covered in completely different way) hook - it intercepts all calls to OpenProjects.open and makes sure that the fake project never appears there. It adds new interface, so previous usages remain unmodified. I agree with JG01 that this does not need to be public API, if that is desirable, I can hide the API a bit. I am willing to use both approaches in ergonomics: I will delegate as much as I can from the fake to the real project. However I'd also welcome an API to prevent my fake projects to be ever listed in OpenProjects.getOpenProjects - that can only contribute to the overall stability of the system.
"I am not really fond of the reflection used to call projectOpened() protected method" - I would recommend just making this method public. It is a general antipattern in our code that we try to prevent people from calling things from the wrong direction, only to find later that in fact this is occasionally necessary for transparent proxying. In fact I know it is common to want this to be public from unit tests. "I do not like that the instance returned from OpenProjects.getOpenProjects() is the fake one not the real one" - yes this is undesirable, and I agree with mkleint that it can cause issues with buggy modules. I just think these issues are solvable as straightforward bug fixes, and it is _less dangerous_ than having both the fake and real Project instances appear in the system at different times. What I was trying to say earlier, and perhaps didn't express well: it is a basic invariant of the project system that for a given project directory, there is at most one valid Project instance. The project system is layered so that non-UI code (depending on projectapi) uses ProjectManager.findProject and expects that projects are loaded on request, sometimes implicitly by e.g. FileOwnerQuery; whereas UI code (depending on projectuiapi) additionally uses OpenProjects to see which projects are "open", meaning indicated by the user as somehow active and salient. Code which does not specifically need to know about "openness" (including most queries) can and should ignore OpenProjects. If PM.fP reports a Project instance for a given directory, even if that project is not open, this (usually lower-level) code will begin using that Project (and the services in its Lookup); any substitute placed in OpenProjects later will probably not be seen, and if both start to be used in parallel, it is hard to predict what chaos will result. That is why I feel more comfortable with _consistent_ use of proxying, i.e. the "real" Project is never offered from ProjectManager.
Created an attachment (id=75238) [details] Patch supporting delegation without use of reflection
I guess we are still oscillating between attempt to 1. delegate and proxy 2. create hooks to trigger the right modules. Both approaches are possible and OK from my point of view. The patch for #2 is http://www.netbeans.org/nonav/issues/showattachment.cgi/74455/api.diff attached since beginning. If necessary its API can even be more hidden, so it is not available for non-friend API users. Anyway Jesse seems to prefer solution #1. That is why I've just created patch to support that approach: http://www.netbeans.org/nonav/issues/showattachment.cgi/75238/ErgonomicsIncrementalOpenProject.diff It is surprisingly simple - it just expects that the amount of ProjectOpenHooks can increase while one of them is called. If others are fine with #1, I am fine with this patch as well.
Just for the record I don't wish there to be any commits to projects codebase without my in-depth review. Since I'm on vacation now till end of Dec, the usual 1 week period doesn't apply here
while your ProjectOpenHook based patch is probably technically correct, I don't understand what you need it for. A Lookupmerger<ProjectOpenHook> in FOD mdule shall do the same job as well. Please state your usecase. What are you planning to inject within the ProjectOpenHook? At the same time I'm worried that you actually need something like that when doing #1 (proxying). It appears that you are not really proxying at the point when the project is not opened yet..
I see. LookupMerger works: http://hg.netbeans.org/ergonomics/rev/ebc8ce972a81 It is slightly complicated to use it. I am not sure how to not specify path on SFS to read instances from, but things seem to work and there is no need for reflection anymore. Patch http://www.netbeans.org/nonav/issues/showattachment.cgi/75238/ErgonomicsIncrementalOpenProject.diff is no longer needed. I still leave this issue opened to discuss http://www.netbeans.org/nonav/issues/showattachment.cgi/74455/api.diff and co.
http://www.netbeans.org/nonav/issues/showattachment.cgi/74455/api.diff is conceptually wrong as has been discussed before. What is there to discuss?
Per our yesterday's discussion I've just suppressed warning in ProjectsRootNode in case of ergonomics projects: 6749788f0206 As far as I am aware this was the only outstanding issue where == or instanceof or cast caused visible problems. But I will do audit of our sources later. Anyway this issue can be closed as wontfix.
