Community
Participate
Working Groups
This looks like another difference between how Project Explorer works and other viewers (such as Package Explorer or Navigator) work. If your project is modified and you change focus between the Project Explorer and some one of the other views, you'll see that the save icon (and File > Save menu item) goes from disabled (when the Project Explorer has focus) to enabled (when some other view has focus). Quick way to re-create this behavior: Open the Project Explorer view Create a new project Create a new file in the project Open the new file in a text editor and enter some text Save will be enabled because the text editor has focus Click on the Project Explorer title to set focus in that view Note that save is now disabled Open Navigator or Package Explorer Note that save is now enabled
The Save As... menu item is broken in the same way
I'd like to raise the priority of this issue as this is a really annoying small one.
In SaveHandler.java, the issue seems to be that getSaveablePart(context) return ProjectExplorer when ProjectExplorer is selected, whereas it's more expected to return the TextEditor (at least, that's what JDT Package Explorer seems to do).
To be more precide, the root issue is that CommonNavigator does implement ISaveablePart (and ISaveableSource). Just removing the "implements ISaveablePart" in CommonNavigator does the trick and delegates Save command (and enablement) to the active editor. This change constitutes an API change, but it's really a major bug in Project Explorer. Is it possible to get it as part of Luna RC1? FYI, JBoss Developer Studio relies on CNF and on the Project Explorer view for an easier navigation, and we wouldn't mind to have this behaviour changing positively in a near future ;)
We can't change the API here. Also, I'm not convinced change the behaviour here is a good thing in the service release. IIRC the common navigator allows the content providers to return their own set of saveables (it has its own save infrastructure) which would stop working if we removed that interface. We can consider a way to fix it for Mars (like have the default content provider plug into the CNF saveable API) but that sounds like a lot of work. PW
Ok, thanks for the explanation. Do you have an example of a CNF extension which does provide some specific Saveable parts in its context provider? Then what would you think about an implementation of the default content provide that would simply return the active editor (ie output of Util.getAdapter(activePart, ISaveablePart.class) when activePart isn't an ISaveablePart) by default?
The default content provider would have to implement saveables to the CNF API. I don't know what that API requires, though (it would require some research :-) PW
CommonNavigator currently contains: public boolean isSaveAsAllowed() { return false; } I tried to replace the "false" by "isDirty()", and then everything seems to work fine. Would this "isDirty()" be a better implementation? That would be great since it doesn't require any API change. Is isDirty() a good enough implementation? At least, it seems better than "false"...
(In reply to Mickael Istria from comment #8) > CommonNavigator currently contains: > > public boolean isSaveAsAllowed() { > return false; > } > > I tried to replace the "false" by "isDirty()", and then everything seems to > work fine. Would this "isDirty()" be a better implementation? > That would be great since it doesn't require any API change. Is isDirty() a > good enough implementation? At least, it seems better than "false"... Please ignore this comment, which was totally wrong.
If I understand correctly, the CommonNavigator was designed this way (using a NavigatorSaveableService and implementing the SaveableSource and SaveablePart interfaces) because it was designed in order to be used as an editor too. Am I right? If so, then the Project Navigator is not meant to be an editor, but a Navigator, so we could think of overriding the right methods to delegate save state to editors. Would that be an acceptable fix?
I had looked into it, and really the appropriate fix here would be for the default content provider in org.eclipse.ui.navigator.resources to provide the saveables through the CNF API. PW
I'm digging in the code of CNF and Saveable and it's not very obvious to me... In order to fix this bug, it seems like that things to provide are: * a SaveableProvider instance that would return as Saveable the current active editor. Let's call it "ActiveEditorSaveableProvider" * Make the ResourceExtensionContentProvider implement IAdaptable and make the adapt() method return my ActiveEditorSaveableProvider. Would this work? Is that all? If it is enough, wouldn't it be better to create a DummyContentProvider which would adapt the the ActiveEditorSaveableProvider (since after all, the ActiveEditorSaveableProvider is not related at all with the ResourceExtensionsContentProvider) ?
The more I look at the NavigatorSaveableService, the more I have the impression that it's just that the Package Explorer should not at all rely on the save mechanism provided by CNF. With save implementation relying on CNF for Package Explorer, then the save is computed/enabled depending on what's selected inside the Project Navigator. It seems totally counter intuitive as the Package Explorer and all other edition tools in general only take into account the opened editors to decide what to save or not. Even if implementing the right ContentProvider and SaveableSources, it would just be a terrible hack that would introduce the fact that whatever is the context of the CNF, the NavigationSaveableService returns the current editor as saveable. It's IMO much cleaner to say that Package Explorer doesn't want to rely on the NavigationSaveableService or doesn't want to contribute to the save action enablement.
The Project Explorer needs to respond to pluggable saveables. Products use the CNF pluggable capabilities but expose it by simply adding the Project Explorer in their perspectives. The Project Explorer can't just suddenly only track the active editor. That's why having org.eclipse.core.navigator.resources implement the active editor using the standard CNF mechanism is probably the only way to solve this bug. PW
Products that rely on CNF to compute the save state of Project Navigator may be unhappy with that change since it would break there assumptions on Project Navigator behaviour. Is it acceptable to change so much the default behaviour, in one way or the other?
New Gerrit change created: https://git.eclipse.org/r/143097
(In reply to Eclipse Genie from comment #16) > New Gerrit change created: https://git.eclipse.org/r/143097 Patch set 3 is promising, we'll most likely manage to polish it and get it fixed early in 4.13.
Gerrit change https://git.eclipse.org/r/143097 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=bc59fb9238e5b0ff5d1ba87f6c6f1d724ad15c2a
Thank you!
Reopening: With this patch, the "dirty" marker is sometimes shown of Project Explorer. This is not desired.
New Gerrit change created: https://git.eclipse.org/r/144681
(In reply to Mickael Istria from comment #20) > Reopening: With this patch, the "dirty" marker is sometimes shown of Project > Explorer. This is not desired. Fix https://git.eclipse.org/r/144681
@Laksminarayana: Can you please also add a note about this noticeable improvement in the N&N => https://git.eclipse.org/c/www.eclipse.org/eclipse/news.git ?
Gerrit change https://git.eclipse.org/r/144681 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=07f05f87e6f140d55afc9bb1a8f4cc9c897b682f
(In reply to Eclipse Genie from comment #24) > Gerrit change https://git.eclipse.org/r/144681 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=07f05f87e6f140d55afc9bb1a8f4cc9c897b682f This causes API error: The superinterfaces set has been expanded for class org.eclipse.ui.navigator.resources.ProjectExplorer Please fix.
(In reply to Andrey Loskutov from comment #25) > > This causes API error: > > The superinterfaces set has been expanded for class > org.eclipse.ui.navigator.resources.ProjectExplorer > > Please fix. I couldn't reproduce it in my workspace. Can you provide more details? Where can I check?
(In reply to Lakshminarayana from comment #26) > (In reply to Andrey Loskutov from comment #25) > > > > This causes API error: > > > > The superinterfaces set has been expanded for class > > org.eclipse.ui.navigator.resources.ProjectExplorer > > > > Please fix. > > I couldn't reproduce it in my workspace. Can you provide more details? Where > can I check? You should have API tooling installed, API baseline set to 4.12 and run clean build on org.eclipse.ui.navigator.resources project.
(In reply to Andrey Loskutov from comment #27) > You should have API tooling installed, API baseline set to 4.12 and run > clean build on org.eclipse.ui.navigator.resources project. I also don't see the problem you're mentioning, while API Baseline is set. But I do see "The minor version should be incremented in version 3.6.600, since new APIs have been added since version 3.6.500". I also tried it with the API Tools application used in build and didn't see it. Investigating...
(In reply to Mickael Istria from comment #28) > (In reply to Andrey Loskutov from comment #27) > > You should have API tooling installed, API baseline set to 4.12 and run > > clean build on org.eclipse.ui.navigator.resources project. > > I also don't see the problem you're mentioning, while API Baseline is set. > But I do see "The minor version should be incremented in version 3.6.600, > since new APIs have been added since version 3.6.500". > I also tried it with the API Tools application used in build and didn't see > it. Investigating... Yes, I also have the same situation.
(In reply to Mickael Istria from comment #28) > (In reply to Andrey Loskutov from comment #27) > > You should have API tooling installed, API baseline set to 4.12 and run > > clean build on org.eclipse.ui.navigator.resources project. > > I also don't see the problem you're mentioning, while API Baseline is set. > But I do see "The minor version should be incremented in version 3.6.600, > since new APIs have been added since version 3.6.500". > I also tried it with the API Tools application used in build and didn't see > it. Investigating... This IS what I see, I just made it easier for you to understand where the error is coming from. If you look into the error details (Ctrl+1 on error in manifest), you will see the error. However PDE API tooling is most likely wrong. The class is final, and addition of a method to a final class is not an API break (no one can extend final class), see notes on https://wiki.eclipse.org/Evolving_Java-based_APIs#Example_4_-_Adding_an_API_method. I've created PDE bug for that, see bug 548607. I hate compilation errors in my workspace, but I'm not sure if the fix is trivial or not, let wait for Vikas answer on bug 548607 before updating version.
(In reply to Andrey Loskutov from comment #30) > This IS what I see Ok, good then. > However PDE API tooling is most likely wrong. The class is final, and > addition of a method to a final class is not an API break (no one can extend > final class), see notes on > https://wiki.eclipse.org/Evolving_Java-based_APIs#Example_4_- > _Adding_an_API_method. I've given more thoughts about it this morning, and I think we're in a regular error case that requires a regular *minor* version bump. What's reported here is actually not an API Breakage, but more an API addition (ie ProjectExplorer can now be dereferenced for usage of ISecondarySaveableSource#isDirtyStateSupported). I think we should just bump version to 3.7.0.
(In reply to Mickael Istria from comment #31) > I think we should just bump version to 3.7.0. Yes, I was on a wrong trip with the API bug. Please bump the version.
New Gerrit change created: https://git.eclipse.org/r/144811
Gerrit change https://git.eclipse.org/r/144811 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=dbe850894ee4f8e6c8b9cfd9b48be9b9229c19a2
Thanks Laksminarayana and Andrey! @Laksminarayana Can you please add a note about this in the news and noteworthy document for the 4.13 release?
(In reply to Mickael Istria from comment #35) > Thanks Laksminarayana and Andrey! > @Laksminarayana Can you please add a note about this in the news and > noteworthy document for the 4.13 release? okay, will add in a bit
New Gerrit change created: https://git.eclipse.org/r/144870
Gerrit change https://git.eclipse.org/r/144870 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=8ec5103214102646870dcdfbbeec78d9f97cb4c6
Thanks Lakshminarayana!
