Repository navigation
Conversation
jtulach
added
Java
| if (dir != null && dir.isFolder()) { | ||
| try { | ||
| Project p = ProjectManager.getDefault().findProject(dir); | ||
| Project p = ProjectManager.getDefault().findProjectOrFallback(dir); |
There was a problem hiding this comment.
One obvious usage of this new "fallback project" functionality is opening of projects on startup.
The files on disk may change since the NetBeans shutdown. Sometimes it happens that original projects are no longer recognized even the original directory path exists.
In such case it is still better to open the "fallback project" rather than opening nothing.
| if (fo != null && fo.isFolder()) { | ||
| try { | ||
| Project p = ProjectManager.getDefault().findProject(fo); | ||
| Project p = ProjectManager.getDefault().findProjectOrFallback(fo); |
There was a problem hiding this comment.
When using File/Project Groups... menu:
Selecting the menu item opens a dialog
and allows one to click the New group... button to open another dialog. After choosing Folder of Projects and browsing for it:
... one can click "Create Group" button. Which scans for all the projects in the given folder and opens them in Projects view (after closing all previous ones).
No Projects Found
It may happen that there are no projects found during the scan. Leaving the Projects View empty. That's very weird.
With this one line change, the root of the folder is always opened: either as a real project or as a "fallback project".
Works great with #9602 and its nesting capabilities. The root folder is opened on the top and all other projects are nested beneath it.
3392f76 to
a9a36fd
Compare
| if (impl instanceof ProjectManagerImplementation.WithFallback implV2) { | ||
| return implV2.findProjectOrFallback(projectDirectory); | ||
| } else { | ||
| throw new IllegalArgumentException("Cannot create fallback project for " + projectDirectory); // NOI18N |
There was a problem hiding this comment.
This exception message seems off? Doesn't this throw even if the fallback isn't needed?
Personally I'd prefer a default method over the additional type here, but I know your feelings there.
There was a problem hiding this comment.
Done in c01de98 - together with introduction of a new argument type, it is binary compatible change as analyzed here.
There was a problem hiding this comment.
Thanks! Almost what I was going to suggest with the opening flags. I don't agree with all of your post on default methods (JSONList), but I do understand the need for considering compatibility.
In light of which, I am still concerned about the exception in ProjectManager, and the potential for certain places to start throwing exceptions that didn't before. I'd still allow findProjectOrFallback to return null if we're going to use it as a replacement.
There was a problem hiding this comment.
concerned about the exception in
ProjectManager
Which exception do you have in mind? The NPE that could be throw here? Well, that's never going to be thrown as with FindOptions.WITH_FALLBACK argument the implementation never returns null. Just (as a result of introducing a default method for all the FindOptions variants) the type system (nor @NonNull & co. annotations) really can express that easily.
There was a problem hiding this comment.
Which exception do you have in mind? The NPE that could be throw here? Well, that's never going to be thrown as with
FindOptions.WITH_FALLBACKargument the implementation never returnsnull.
Exactly that NPE. Sure, your implementation never returns null, but the public SPI interface default method does. Now, if we're treating that as an SPI that can only have one implementation, that's fine, but then the arguments about maintaining compatibility don't really hold.
So, we could have ProjectManager::findProjectOrFallback throw an exception like this in cases where the implementation doesn't override the default method. But it is not then a simple drop-in replacement for ProjectManager::findProject. Your changes in DirectoryGroup and OpenProjectList retain the null check that is no longer needed, but will hit a different code path if an SPI implementation causes an exception - the latter will catch it, but the former will blow up I think?
So, if we want to treat this as a simple drop-in replacement across our internal code and treat it as a public SPI, I think it's easier to allow a null return as with ProjectManager::findProject when fallback creation fails. If not, then the usages in this PR might need a rethink.
There was a problem hiding this comment.
Exactly that NPE.
OK. I'll think about our alternatives.
if we're treating that as an SPI that can only have one implementation
- I believe we do
- e.g. for all practical purposes there is just a single implementation
NbProjectManager - the assumption there was just a single implementation was the major reason why usage of
defaultmethods seemed justifiable in this case to me
|
(Whatever we do in this area, @eppleton is right we need a more proper check in the source file launcher check; that can be done separately.) I assume the end goal here is to achieve the "Open Folder"/"Add to Workspace" actions, as mentioned in the recent e-mail. I do wholeheartedly agree with adding (some) support for "workspaces" (i.e. ability to work without projects, or without specifying projects one-by-one). What I am not quite sure if whether starting with an API change is the best move. Whether mapping this to project groups matches the requirements also remains to be seen. I think I would first try to implement the actions, and from that it would lead what API changes are needed (I suspect some will be needed). #9631 implemented the Open Folder as Workspace action, but not the Add to Workspace action; and also the resulting UI is I think not ideal: when I do "Open Folder as Workspace", would I really expect to see all the nested projects in the Projects tab? (I realize this is a consequence of reusing the Project Groups directly, but a novice user is likely to be confused by this, I think.) OTOH, I do agree it would be good to open the projects under the workspace folder. I guess if I would be doing this, I would:
|
| return null; | ||
| } else if (o != null && !LoadStatus.SOME_SUCH_PROJECT.is(o)) { | ||
| } | ||
| if (o != null && !LoadStatus.SOME_SUCH_PROJECT.is(o)) { |
There was a problem hiding this comment.
Fallbacks break existing project recognition consumers
Once cached, a fallback is returned by ordinary findProject(). Consequently, FileOwnerQuery assigns previously unowned Java files to it, disabling single-file Run/Debug and classpath support, which require a null owner. It also makes ProjectGenerator.createProject() reject the folder as already containing a project. Preserve these existing consumers’ behavior before exposing fallbacks through the ordinary cache.
Cache clearing returns before replacing fallbacks under write access — NbProjectManager.java (line 517)
postReadRequest() defers execution until the current write lock is released. Creating project metadata, calling clearNonProjectCache(), then calling findProject() within that lock therefore still returns the fallback. Replacement needs to take effect before the subsequent lookup.
Replaced fallbacks remain valid and can retain file ownership — NbProjectManager.java (line 525)
Removal updates only dir2Proj. The fallback remains in proj2Factory, so isValid(fallback) stays true. When replacement is triggered by a factory lookup change, the file-owner cache also remains stale: findProject() returns the real project while FileOwnerQuery returns the fallback. Retire the old instance and invalidate ownership caches together.
There was a problem hiding this comment.
Thanks Martine. Some of these are similar to Toni's findings. Some are new. All fixed in e348182
|
Thank you Jan.
|
Co-authored-by: Martin Balín <Martin.Balin@oracle.com>
Sorry, having given this some more thought, I agree with @lahodaj here. There is more to discuss. I think the requirement for this new API is not currently proven, and handling this probably can, and should, be via an ordinary project factory. As we're effectively in our ramp down phase to the next release, the end user benefits for this are currently minimal, there may still be some fallout from the implementation, and the API may lock us in to something that is the wrong approach, I don't think we should fast track this in. Let's hold fire for the next cycle and consider along with the wider impact usage that goes with it. So, that's a -1 from me to merging this prior to branching NB 32. Unless one of our active maintainers wants to make a strong argument in the other direction. I would really like to see the ability to opt in to opening any folder as a project. Don't get me wrong, I'm not negative on solving the problem. It should be possible for the user to open any folder as a project. There is potential there to address a whole bunch of user requests, including but not just making the folder group function better. eg. I'd like to see the inverse of our current folder action
In terms of marker, I would suggest the presence of the default |
neilcsmith-net
added
the
do not merge
|
@neilcsmith-net @lahodaj I understand not wanting to rush a new API in during ramp-down. But I'd hate for the work done here over the last few days to just sit until the next cycle, so we should at least split out the single-file launcher fix for NB 32. It's useful independently of the fallback API: Run/Debug of a single Java file and its Run Single File properties currently don't work in C/C++, PHP and other non-Java projects, as @jtulach's screenshots show. The change is small and has tests (the shared findJavaOwner check in java.file.launcher plus the JavaNode change). You both said it needs fixing anyway. As its own PR it could still make NB 32. |
| File f = chooser.getSelectedFile(); | ||
| if (f != null) { | ||
| FileObject fo = FileUtil.toFileObject(f); | ||
| Project owner = FileOwnerQuery.getOwner(fo); |
There was a problem hiding this comment.
- one issue that @lahodaj warned about is:
- creating a fallback project inside of another project
- disconnects the
FileOwnerQuery.getOwnerfrom the right project with the right lookup - then the
.javasources may not get the right classpath, etc.
- simplest way is to prevent that altogether
- the adbcfc4 change does that
- even if one selects
srcdirectory in a Maven project - the owning maven
getProjectDirectory()is selected as the "Folder of Projects"
|
|
A pull request is not the place to hold a vote. Nor is it a good way to review what is technically better. And it is secondary to first trying to find consensus. What requirements are there that cannot be met by a standard project factory, and what SPI would be required to meet those? eg. How could a fallback solution be disabled, replaced, reordered, etc. if it is not a factory registered in the layer? Edit :
I do intend to keep my -1 on this until the project factory solution is considered, and the pros and cons to both approaches are weighed correctly considering what we're trying to achieve here. The project factory solution has cons, and will require some SPI to work, but in my opinion has far fewer cons than this API. |
jtulach
changed the title
ProjectManager.findProjectOrFallback API
|
eppleton
left a comment
eppleton
left a comment
There was a problem hiding this comment.
Looked at 83c7fd7. Moving everything into ide/projectui as a plain ProjectFactory addresses the API concerns nicely.
One blocker: the last commit stops normal projects from being reopened on startup (details inline). The rest are leftovers from removing the API.
I ran the projectui tests that restore projects from OpenProjectListSettings. Five of them pass on master and on 6d3969b, but fail on 83c7fd7:
CloseProjectsTest: "Just fifteen left nodes expected:<15> but was:<0>"ProjectsRootNodeFindTest,ProjectsRootNodeInitializedSoonerTest,OpenProjectListSetMain2TestandOpenProjectHookThrowsExceptionTesthang until the 300s timeout.
(ProjectsRootNodePhysicalViewTest times out on master too, so I left it out.)
In the new tests, FallbackFactoryTest.testFallbackProjectLookup and ProjectManagerFallbackTest.testFactoryChangeUpdatesFileOwner fail for me as well. I assume those are the ones you mentioned are still pending.
| Project p = ProjectManager.getDefault().findProject(dir); | ||
| if (p != null && !result.contains(p)) { //#238093, #238811 if multiple entries point to the same project we end up with the same instance multiple times in the linked list. That's wrong. | ||
| result.add(p); | ||
| if (FileOwnerQuery.getOwner(dir) == null) { |
There was a problem hiding this comment.
This skips every regular project too: a project directory is owned by its own project, so FileOwnerQuery.getOwner(dir) is non-null for every real project in the saved list, and URLs2Projects returns nothing for them. With this commit, no project is reopened on startup.
I think the owner check only belongs on the fallback path: use findProject(dir) as before, and fall back only when that returns null and no other project owns dir.
| # under the License. | ||
| is.eager=true | ||
| javac.release=17 | ||
| javac.release=21 |
There was a problem hiding this comment.
Leftover from the API version? projectapi.nb has no source changes any more, but its javac.release (17 → 21) and its projectapi dependency (1.78 → 1.107, in project.xml) are still raised.
| /** | ||
| * Binds to a specific {@link ProjectConfiguration}, which must resolve to an instance of | ||
| * @see <a href="@org-netbeans-modules-maven@/org/netbeans/modules/maven/api/MavenConfiguration.html">MavenConfiguration</a>. | ||
| * @see MavenConfiguration |
There was a problem hiding this comment.
This looks unrelated to the PR. MavenConfiguration isn't visible from projectapi, so @see MavenConfiguration can't resolve; the previous cross-module link worked.
| private FileObject goodproject2; | ||
| private FileObject badproject; | ||
| private FileObject mysteryproject; | ||
| private FileObject justADir; |
There was a problem hiding this comment.
Since the tests moved to projectui, this file only has whitespace changes plus this now-unused justADir field. The org.openide.util.ui.svg test dependency added to ide/projectapi/nbproject/project.xml also seems unneeded now. Maybe revert both?
Firstly, I don't believe there's a demonstrated need for a fallback project, and I don't think it should be inside My view on the project factory solution is that we need a Generic Project type (which I think aligns with @mbien comments elsewhere, and possibly @lahodaj - see the MVP thread on dev@). A generic project type would wrap any folder, allowing it to act as a container for the files or projects within it. This handles a use case for project groups as well as much more. Ideally, this project type exists in a separate module - eg.
I'm not sure trying to do this without an API is desirable or possible, it's just that the API should not embed the project type or require a project to be created.
If we have a standard project type that is created explicitly by user interaction, I'm not sure why this is a requirement in the first place?!
No, I'm really talking about an SPI. What we lack at the moment is an SPI that captures explicit user intent to open a folder as a project. As mentioned in email, it might have some similarity to ProjectConvertor. But we don't want a project factory or convertor that starts recognising every folder as a potential project! So, something like - There may be one, none or many registered. I have a few thoughts where multiple might be useful. In the IDE, there would be at least one in There may also be an API very similar to your original that can be used by project groups, etc. Where it would differ is that it doesn't document a specific project type, but that it queries the registered SPI, and it is still allowed to return
Agreed, assuming we're not discounting file attributes and talking about the default behaviour. We already have something similar in CPPLite storing project info in the userdir and using it to recognise folders. I suggested piggybacking on the default AuxiliaryConfiguration file attribute. This is going to be added anyway if anything uses I'm sure someone on list mentioned the option of having the project/group settings shareable, which would require the option to store in the project. Again, the default
Why? What's the temporary nature solving, and why the added complexity? The above suggestion would allow for projects to be forgotten by removing the attribute. Again the trigger would be explicit user action. |
|
FallbackFactoryto createFallbackProjectfor any folder