Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for issue #634 closing project automatically loads the CDT project #725

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jantje
Copy link
Contributor

@jantje jantje commented Mar 4, 2024

isNewStyleProject called by gui loads closed CDT projects

@ruspl-afed
Copy link
Member

The proposed change may have much more impact than desired, because for other use cases client code already expects the current behavior of ICProjectDescriptionManager.

Please consider the fix near org.eclipse.cdt.internal.ui.workingsets.HasManagedCdtProjectSelection.hasManagedCdtProjectSelection(HasManagedCdtProjectSelection.java:43) where context may be enough to understand that we don't need to load closed projects.

Also, at least ECA check should became happy, PR cannot be processed further in the current state.

@jantje
Copy link
Contributor Author

jantje commented Apr 22, 2024

The proposed change may have much more impact than desired, because for other use cases client code already expects the current behavior of ICProjectDescriptionManager.

I disagree on that.
public boolean isNewStyleProject(IProject project)
Returns a boolean which the calling code explicitly requested so it should be able to handle both true and false as return value.
The change in the behavior is that the isNewStyleProject may have returned true when the project is closed where in the new behavior it will return false.
In that specific case the project is not opened (only the CDT content is loaded) and any call to iProject may fail.

I took a quick look at the call hierarchy of isNewStyleProject and found that in most cases the project is open. But the ones marked with red below all have the same issue.
image

So fixing this at the level of hasManagedCdtProjectSelection will not fix all cases.

@jonahgraham
Copy link
Member

This comment in #634:

Because getProjectDescription(project, false) translates to getProjectDescription(project, true, false) which means "load the project when it is not yet loaded" all closed CDT projects are loaded when they are selected in the project explorer.

This is a (at least sometimes) incorrect conclusion. If a project is closed, then getProjectDescription(project, true, false) eventually calls getProjectDescriptionStorage which checks if the project is accessible, and because the project is closed it is not accessible, so the project is not loaded and a core exception is thrown:

private AbstractCProjectDescriptionStorage getProjectDescriptionStorage(IProject project) throws CoreException {
if (project == null || !project.isAccessible())
throw ExceptionFactory.createCoreException(
MessageFormat.format(CCorePlugin.getResourceString("ProjectDescription.ProjectNotAccessible"), //$NON-NLS-1$
new Object[] { project != null ? project.getName() : "<null>" })); //$NON-NLS-1$

that exception is caught and a null is returned for project description:

} catch (CoreException e) {
// FIXME Currently the resource change handler ResourceChangeHandler.getProjectDescription(...)
// Does this when the project is closed. Don't log an error or the tests will fail
// CCorePlugin.log(e);
}
return null;


I'll add a comment to the issue too in case I have any useful insight.

@jonahgraham
Copy link
Member

I have converted this to draft until it is ready for a new review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants