Community
Participate
Working Groups
Created attachment 262263 [details] Please read the 2 pages showing the screenshot and the stacktrace in the eclipse platform implementation We have an eclipse based RCP application. We recently upgraded the eclipse version to 451 in our latest release and are seeing a weird behavior in the properties view, after the upgrade. Our RCP product uses the eclipse platform provided properties sheet and contributes a property page for the editable graphical editor supported within the product. (It essentially registers an eclipse Page to be used within the properties sheet which is nothing but a PageBookView). The getContributingPart() implementation of the property page being setup returns the editorPart implementation corresponding to the graphical editor it is meant to represent. We are noticing that, after the upgrade to eclipse 451, whenever the properties view is activated for the first time while the associated graphical editor is in a dirty state, then the properties view also gets a dirty marker. This seems kind of weird, and is a regression since it was not the case with previous versions of eclipse. The previous version implementation of create() within the CompatibilityPart was as follows if (wrapped instanceof ISaveablePart) { part.setDirty(((ISaveablePart) wrapped).isDirty()); } The version of the implementation in eclipse 451 is as follows - (Line 354 in CompatibilityPart.java) ISaveablePart saveable = SaveableHelper.getSaveable(wrapped); if (saveable != null) { part.setDirty(saveable.isDirty()); } We notcied that this change in implementation is the cause for this weird behavior/regression. The "SaveableHelper.getSaveable(wrapped)" returns the graphical editor, since that is being registered as the contributingPart for the property page within the propertiesSheet. And since that is an instance ISaveablePart, the properties view is being set as dirty. Could someone please look at this and confirm if this is a bug? Is there any workaround until this issue/bug is fixed?
Looks like it was fixed by Andrey in commit part of bug 372799.
Reopening: sorry, I misread the above.
*** Bug 470076 has been marked as a duplicate of this bug. ***
Same problem here. Whenever an editor part in my RCP app is dirty. then the Properties View also shows as dirty and prompts the user to save it when closing it.
(In reply to Phil Beauvoir from comment #4) > Same problem here. Whenever an editor part in my RCP app is dirty. then the > Properties View also shows as dirty and prompts the user to save it when > closing it. Sorry I misread this bug. I think your case is bug 470076. We should not ask to save twice, clear. Now regarding *this* bug: I think dirty flag is OK and not a bug (see also bug 125386). Since Properties view can change the content of your editor, it is shown dirty. But might be I'm wrong. @Alok: can you please create a simple reproducible case to play with?
First of all, it is amazing how many inconsistencies we have with that dirty state handling! Note: this concrete bug can only be reproduced if the properties view is opened while a dirty editor is active. Funny is, that once the properties view gets into this "dirty" state, there will be no escape - even if another, not dirty editor will be activated, the properties view will show "dirty" flag. Additionally, if the editor is not dirty, opening properties view and changing something in the editor will not change the dirty state of the properties view. This two bugs are because of two issues: CompatibilityPart does not reset the state of the underlying model part if the wrapped part does not return an adapter to ISaveablePart (see IPropertyListener added in the create() method), and because PropertySheet does NOT call firePropertyChange(IWorkbenchPartConstants.PROP_DIRTY) on selection (for example from updateContentDescription()). To fix this two bugs, CompatibilityPart must learn that there can be parts that *change* they "saveable" state in time, like properties view, and properties view must report that it's "dirty" state is changed! But that is even yet going into the root cause of *this* bug - now let dive deeper. This bug is challenging. We allow workbench parts to implement OR adapt to ISaveablePart (see bug 125386 and bug 372799). So we can have parts which itself aren't implementing ISaveablePart (or any other interface at all), but either use some POJO ISaveablePart to maintain the dirty state or "reuse" the saveable objects from other parts. It means, we can have N:1 relationship between parts and ISaveablePart objects. Bug 125386 introduced ISaveablePart adapter for properties view, and bug 372799 was about proper support for parts not directly implementing ISaveablePart in the "Save" lifecycle. Unfortunately bug 372799 caused a regression in bug 470076 and a direct root cause for this bug, because in some places the old code was not expecting that there can be N:1 relationship between workbench parts and ISaveablePart objects. Here this N:1 relationship causes the "dirty" state appear on more than one workbench part even if there is only on "dirty" object. From the human perspective, this can be surprising, but on the other side, according to the "duck test" principle, if it looks like a duck, swims like a duck, and quacks like a duck, then it probably is a duck. So if a part says "I give you adapter to saveable", it wants to participate in "save" lifecycle (that's the logical consequence of bug 125386), to be consistent this part should also indicate it's "dirty" state. Consequent enough (that's due bug 125386) if "save" is triggered on any of that N "dirty" parts, it will really do the job, because the save handler is working on the same, single saveable object. On the other side, how the user should know if the view can be "saved", if it does not show it's "dirty" state? From the UI consistency language, if a part participates in the "save" lifecycle, the "dirty" state of the part should be visible on the UI so that user knows it is dirty or not. One should be consistent here, but properties view is not consistent and it is not alone. See the breakpoints or variable views with their "detail" pages. Their behavior is inconsistent to each other and to the properties view! Breakpoints view extends variables view and both implement ISaveablePart. In the breakpoints view, if the breakpoint condition changes, it shows "dirty" state, but in the variables view, if one changes the variable value, view does not show "dirty" state, but Ctrl+S will just work in both views! BTW I think no one from my peers knows that Variables view can change *and* save the content by simply using Ctrl+S shortcut - because it does not show "dirty" state. One can for example pin the properties view on the dirty editor, switch to another editor and change something in the properties view. The view itself will represent the old "dirty" editor, but the view state does not reflect it, and Ctrl+S will save the old editor even if it is not visible! I think all this inconsistencies are highly surprising, for everyone, and undesirable. There are at least 3 solutions: 1) All parts exposing ISaveablePart either via implementing it or via providing adapter to it, must properly show dirty state. 2) All parts exposing ISaveablePart either via providing adapter to it, must show dirty state only if they have 1:1 relationship to the adapted object. 3) Allow parts exposing ISaveablePart either via implementing it or via providing adapter to it decide if they want indicate "dirty" state on the UI or not. Discussion of proposals above: 1) That is consistent from programming and UI point of view. To fix it, no extra new interfaces are needed, but it will not fix *this* bug. 2) I think I have a somewhat acceptable (but not 100%) solution at least for the old world (CompatibilityPart) without using new interfaces. For the e4 word, and to have 100% solution for the legacy, we probably will need some extra model flags/tags or interfaces indicating that the part relationship to it's saveable object is 1:1. 3) That requires a new interface for both legacy/e4 world. Both 2) and 3) will keep the UI inconsistency for the end user, because it will be hard to explain why a "not dirty" part allows to use Ctrl+S for saving something, however they will fix this bug. I tend to like 1) solution more, but I also understand that 2) or 3) is probably needed for some special use cases outside of the "default IDE".
(In reply to Andrey Loskutov from comment #5) > (In reply to Phil Beauvoir from comment #4) > > Same problem here. Whenever an editor part in my RCP app is dirty. then the > > Properties View also shows as dirty and prompts the user to save it when > > closing it. > > Sorry I misread this bug. I think your case is bug 470076. We should not ask > to save twice, clear. > > Now regarding *this* bug: I think dirty flag is OK and not a bug (see also > bug 125386). Since Properties view can change the content of your editor, it > is shown dirty. > > But might be I'm wrong. > > @Alok: can you please create a simple reproducible case to play with? Hi Andrey, Based on your detailed explanation of the problem with the solution proposals, I assume you already know how to reproduce this problem. Anyways, here is the repro scenario 1. Have an editor which contributes its own page to the PropertySheet (properties view of eclipse) 2. Launch the RCP application - don't have the Properties view activated. 3. Perform an operation to make the editor dirty. 4. Activate the properties view - -- you'll see that it shows up dirty. -- Closing the application will ask to save the Properties view in addition to the dirty editor. Both these are a regression in behavior, 451 onwards
Also see bug 112225 comment 11 and bug 112225 comment 13 with some initial design ideas and discussion about the save handling.
New Gerrit change created: https://git.eclipse.org/r/75126
(In reply to Eclipse Genie from comment #9) > New Gerrit change created: https://git.eclipse.org/r/75126 This is solution #1 from comment 6. Properties view will properly track dirty state of the contributing part. If the part will be saved, dirty flag will disappear etc. One can see how this works now (and how it was broken before) by playing with *.ecore model editors. The details of the nodes of that editor are only editable from the properties view, which now properly reflects the editor dirty state. I think for this use model a dirty state on Properties view is absolutely OK and a good hint for the user that his input is not yet saved. I also imagine that there can be probably other editors where something should be shown in the properties view but not directly editable - those editors will probably don't want that their dirty state is shown it the properties view, because it cannot be used for editing - but properties view will still show the dirty state! The question now is: should we allow opt-out for such cases, and if yes, how? The problem is that if a part implements ISaveablePart, Properties view automatically exposes this to the framework, which only cares about isDirty() flag on that interface. Not propagating ISaveablePart will break "Save" functionality (bug 125386), propagating it currently means dirty flag (bug 372799). All "legacy" or better say non "pure" e4 editors implementing IEditorPart's are automatically ISaveablePart's. The only way to "hide" the dirty flag on Properties view for an editor would be to implement the editor without extending any existing super classes which implement IEditorPart (basically from scratch), or as a pure e4 object; provide an adapter to ISaveablePart, but do not implement this interface directly. This is doable without changing API. Another (ugly) way would be wrap the ISaveablePart from contributed parts to a proxy ISaveablePart which would answer "not dirty" (but I fear this can cause lot of unexpected side effects). Next we also can introduce a special interface in org.eclipse.ui.workbench plugin (or better in the new world in org.eclipse.e4.ui.workbench) which can be used by CompatibilityPart (and eventually e4 renderers) to recognize parts which do not want to propagate their "dirty" state to the UI. Properties view could implement this interface, and delegate the decision to the contributed PropertySheetPage's, which would require another new interface to extend IPropertySheetPage or IPropertySourceProvider. I have no clear plan yet how all this can be addressed in the most elegant way, proposals are welcome.
(In reply to Andrey Loskutov from comment #10) > I think for this use model a dirty state on Properties view is absolutely OK > and a good hint for the user that his input is not yet saved. Actually I think this is a very bad idea. Already the Editor shows a dirty state. Surely the Properties view is a general view that is not really tied to any one editor?
I forgot to add - for my RCP application (http://www.archimatetool.com) it makes absolutely no sense for the Properties View to show a dirty state. It's just a Properties view. Dirty state is shown elsewhere.
Created attachment 262386 [details] Properties Dirty Dialog I was editing an Ecore model in Eclipse 4.6 and then attempted to close the Properties View. This dialog appeared. Saving the Properties View is not something I want to do!
(In reply to Phil Beauvoir from comment #13) > Created attachment 262386 [details] > Properties Dirty Dialog > > I was editing an Ecore model in Eclipse 4.6 and then attempted to close the > Properties View. This dialog appeared. Saving the Properties View is not > something I want to do! Sure, this is bug 470076 and the patch is waiting for hudson.
(In reply to Andrey Loskutov from comment #14) > (In reply to Phil Beauvoir from comment #13) > > Created attachment 262386 [details] > > Properties Dirty Dialog > > > > I was editing an Ecore model in Eclipse 4.6 and then attempted to close the > > Properties View. This dialog appeared. Saving the Properties View is not > > something I want to do! > > Sure, this is bug 470076 and the patch is waiting for hudson. OK, I'm now confused between this bug and bug 470076. Aren't they connected?
(In reply to Phil Beauvoir from comment #15) > (In reply to Andrey Loskutov from comment #14) > > (In reply to Phil Beauvoir from comment #13) > > > Created attachment 262386 [details] > > > Properties Dirty Dialog > > > > > > I was editing an Ecore model in Eclipse 4.6 and then attempted to close the > > > Properties View. This dialog appeared. Saving the Properties View is not > > > something I want to do! > > > > Sure, this is bug 470076 and the patch is waiting for hudson. > > OK, I'm now confused between this bug and bug 470076. Aren't they connected? Yes, both are regressions from bug 372799, but can be fixed separately from each other.
New Gerrit change created: https://git.eclipse.org/r/75318
(In reply to Andrey Loskutov from comment #6) > First of all, it is amazing how many inconsistencies we have with that dirty > state handling! > [...] > I think all this inconsistencies are highly surprising, for everyone, and > undesirable. > There are at least 3 solutions: I think I have *another* one, acceptable solution, see below. (In reply to Eclipse Genie from comment #17) > New Gerrit change created: https://git.eclipse.org/r/75126 This patch makes the Properties view *properly* track the dirty state of the connected ISaveablePart. For example with a dirty *.ecore model editor and few Properties view opened, pinned to this editor, we will see all of them changing "dirty" state on each change/save operation. This can be desired for some tools, but meanwhile I think this is more disturbing for user. It is really highly controversial behavior and I see both pros and cons of that equally good/bad. However, this patch alone would mean we will "break" the expected behavior of existing tools (the concern expressed on this bug). As a negative side effect, "dirty" state from ISaveablePart is propagated to the Properties view even if the Properties view does not provide any means to "edit" something on that ISaveablePart. Previously this was not "visible", but now due the "dirty" indication this is more prominent. This can be seen for example with Breakpoints view, where changing breakpoint conditions makes the Properties view "dirty" even if no content is shown in the Properties view itself. That's hard to fix with the current code base, since PropertySheetPage itself has no means of "I'm not empty and editable now?" API, this need to be somehow computed from the data (IPropertySource objects) and can't be implemented in backward compatible way for custom implementations of IPropertySheetPage. So I think we can't merge this patch without providing the backwards compatible way to disable the "dirty" state indication => therefore the next patch below. > New Gerrit change created: https://git.eclipse.org/r/75318 This patch is based on the previous one and makes the "dirty" state propagation behavior for Properties view configurable. Per default, Properties view will not be "dirty". After applying this patch this bug can be considered as fixed. To be backwards compatible, "dirty" state propagation behavior is implemented as "opt-in": per default, framework do not shows the "dirty" state for Properties view (or any of ISecondarySaveableSource instances), even if the connected ISaveablePart is dirty. To do so, a new interface "org.eclipse.ui.ISecondarySaveableSource" is added, which implements isDirtyStateSupported() method and by default returns "false". PropertySheet implements this ISecondarySaveableSource interface now and re-implements this in the way that it delegates the isDirtyStateSupported() decision to the PropertySheetPage / connected ISaveablePart. Therefor, if desired by a some tool/workflow, the "dirty" state tracking of ISaveablePart can be enabled for Properties view. The default "false" can be changed to "true" by: 1) Contributing custom IPropertySheetPage implementation to the tracked part and overriding getAdapter() to return custom ISecondarySaveableSource. 2) Contributing custom ISecondarySaveableSource adapter to the tracked part. 3) Contributing global IAdapterFactory which provides custom adapter to ISecondarySaveableSource (not recommended, because if not properly implemented this can affect unrelated code). With this, I think we have a good compromise between backwards compatibility, logically consistent code, "natural" UI behavior expectations and customization of the Properties view. => going to request review for both patches.
Sounds great, Andrey. If and when a patch is accepted, might it be possible to get it into a Neon 4.6 maintenance update?
(In reply to Phil Beauvoir from comment #19) > Sounds great, Andrey. If and when a patch is accepted, might it be possible > to get it into a Neon 4.6 maintenance update? Let us wait for the review results first. I've just asked Platform UI for comments on this proposal.
I think I got the Idea and I looked at the code, but this is not trivial. Can you provide an example project (*) and a sequence of steps to reproduce? (*)[or, tell which plug-ins to add to SDK to edit .*ecore model] From what I understand the root cause is we started allowing Parts to implement OR adapt ISaveablePart. I agree that those parts must be responsible for the dirty state. IIUC, the first change, https://git.eclipse.org/r/#/c/75126/ , enforce a part adapting ISaveablePart, to handle and propagate the dirty state. But, change 75126, breaks the expected behavior of existing tools. (I assume) because it'll show the dirty marker on views, <i.e.> the PropertySheetPage at comment #0 or the Breakpoint view Hence, we need the second patch, based on the first one, but with the "dirty" state propagation configurable. IIUC, by default the framework does not show "dirty", but developers can opt in by changing the default "false" to "true" by: 1) Contributing custom IPropertySheetPage implementation to the tracked part and overriding getAdapter() to return custom ISecondarySaveableSource. 2) Contributing custom ISecondarySaveableSource adapter to the tracked part. 3) Contributing global IAdapterFactory which provides custom adapter to ISecondarySaveableSource (not recommended, because if not properly implemented this can affect unrelated code). (the above list is from comment in Change 75318) IMO, the opt in in solution #2, is kind of complex, and without docs easy to understand, the developers will have hard times to understand.
(In reply to Patrik Suzzi from comment #21) > I think I got the Idea and I looked at the code, Thanks for your time! > but this is not trivial. If it would be trivial, everyone would be able to fix it and contribute a patch, but it is not yet fixed => ergo it is not trivial. > Can you provide an example project (*) and a sequence of steps to reproduce? > (*)[or, tell which plug-ins to add to SDK to edit .*ecore model] I simply use *any* project and inside the project create ecore model file with a wizard: File->New->Eclipse Modelling Framework->Ecore Model. If you have EMF installed (and I think you should have this installed if you followed the Platfrom UI wiki), you can do this easily. > From what I understand the root cause is we started allowing Parts to > implement OR adapt ISaveablePart. Yes. The original javadoc contract in ISaveablePart was saying "OR" but not in all places it was working, so bug 372799 was created and then fixed in 4.5.1, causing the regression here. > I agree that those parts must be responsible for the dirty state. > > IIUC, the first change, https://git.eclipse.org/r/#/c/75126/ , enforce a > part adapting ISaveablePart, to handle and propagate the dirty state. Not quite right: not *any* part, but Properties view which can be connected to a ISaveablePart. Also part of this change is to fix CompatibilityPart to allow views reset their dirty state if they have changed the ISaveablePart adapter to null. This affects all views, but I think there are no known bugs except this one related to this weird Properties view behavior. > But, change 75126, breaks the expected behavior of existing tools. > (I assume) because it'll show the dirty marker on views, > <i.e.> the PropertySheetPage at comment #0 or the Breakpoint view Again, this affects the dirty state marker on Properties view only, *connected* to some other view, like Breakpoints view. So with first patch both views will have a "dirty" state indication. > Hence, we need the second patch, based on the first one, but with the > "dirty" state propagation configurable. Yep. > IIUC, by default the framework does not show "dirty", but developers can opt > in by changing the default "false" to "true" by: > 1) Contributing custom IPropertySheetPage implementation to the tracked > part and overriding getAdapter() to return custom > ISecondarySaveableSource. > 2) Contributing custom ISecondarySaveableSource adapter to the tracked > part. > 3) Contributing global IAdapterFactory which provides custom adapter to > ISecondarySaveableSource (not recommended, because if not properly > implemented this can affect unrelated code). > (the above list is from comment in Change 75318) > > IMO, the opt in in solution #2, is kind of complex Yes it is, also all this Properties framework is complex, so the solution fits to the framework :-) I would be happy to provide an easier solution, I've discussed with myself some ideas on this bug before, but I think none of them would be *both* easy and make everyone happy. > and without docs easy to > understand, the developers will have hard times to understand. What do you mean "without docs"? In the second patch you will see javadocs on both new interface and Properties view implementation. If you mean this is hard to read and/or understand, or even incomplete, please comment on the Gerrit, that's exact the reason I've asked for the review :-)
I've been looking over Andrey's changes and digging into the background, and gosh this is complex! Thanks Andrey for tackling this. It seems to me that the root cause here is that the workbench has no way to differentiate between "should *this* part be marked as dirty" from "what is this part's source of saveable content". The workbench currently asks the part for its ISaveablePart and then uses that ISaveablePart's dirty status. Andrey's change essentially introduces another layer that supports separation these two questions, although it isn't quite phrased in these terms. The workbench instead asks the part whether it should use the part's ISaveablePart's dirty status. I'm not sure where I sit on this change. In part, I think it's adding flexibility where there's no usecase calling for it. We have clear feedback that the Properties view should not be reflecting the dirty state of its associated part. And when I think of a JDT perspective (with Javadoc, Declarations, and Outline views) or back when I used Dreamweaver (which provided about 8 views complementing source), I would not want all of the views flickering on and off reflecting the dirty state of the editor. So I'm not sure that views should just reflect the ISaveablePart's dirty flag. But I do think it can make sense for the Properties view to reflect dirty state of its *own* content, even if it's just reflecting content from an editor. Consider the following situation: 1. I edit an .ecore file. 2. I select an element within the file; that element's properties are shown in the Properties view. 3. I switch to the Properties view and begin to edit one of the property values. At this point the Properties view is dirty, since I've made a change, but the editor should not be dirty as that change has not yet been propagated to the editor. (If I hit <Escape>, the change is abandoned.) Once I hit <enter>, the change should be applied, the editor should be dirty, and the Properties is no longer be dirty (since it now reflects the value of the editor).
I don't think that the Properties view should show a dirty state under any circumstances. It's a re-assignable view showing different contexts, unlike an editor which is definitely tied to one context. I'm not against making it optional, however.
(In reply to Brian de Alwis from comment #23) > I've been looking over Andrey's changes and digging into the background, and > gosh this is complex! Thanks Andrey for tackling this. > > It seems to me that the root cause here is that the workbench has no way to > differentiate between "should *this* part be marked as dirty" from "what is > this part's source of saveable content". The workbench currently asks the > part for its ISaveablePart and then uses that ISaveablePart's dirty status. Yes, this is the main issue. UI simply shows status of a ISaveablePart. The first patch shows what this means if we *consequently* do this. > Andrey's change essentially introduces another layer that supports > separation these two questions, although it isn't quite phrased in these > terms. The workbench instead asks the part whether it should use the part's > ISaveablePart's dirty status. > > I'm not sure where I sit on this change. In part, I think it's adding > flexibility where there's no usecase calling for it. As you have seen on comments, I needed many takes on this to convince myself what is the expected behavior and how to make all possible use cases happy. This is clearly very complex, but we have at least 2 concrete use cases - this bug and the enhancement of the Properties view you've requested below. The main use case is that we can't differentiate between views which provide adapter to ISaveablePart because it is they *own* state (and so they want see dirty flag shown) and views like Properties view which provide *not* they own adapter to ISaveablePart, and so don't want to be shown "dirty". The second patch, based on the first one, tries to bring this controversial requirements fixed by introducing another API "layer". > We have clear feedback > that the Properties view should not be reflecting the dirty state of its > associated part. And when I think of a JDT perspective (with Javadoc, > Declarations, and Outline views) or back when I used Dreamweaver (which > provided about 8 views complementing source), I would not want all of the > views flickering on and off reflecting the dirty state of the editor. Absolutely agree. This is what I've struggled most at the beginning. > So I'm not sure that views should just reflect the ISaveablePart's dirty flag. This is why by default the new API disables the dirty state for that views, and introduces flexibility for those who may really want this. The problem is that the views like Properties view *want* to participate in the "Save" process (and so adapt to ISaveablePart) but *do not want* to be shown dirty. > But I do think it can make sense for the Properties view to reflect dirty > state of its *own* content, even if it's just reflecting content from an > editor. Consider the following situation: > > 1. I edit an .ecore file. > 2. I select an element within the file; that element's properties are > shown in the Properties view. > 3. I switch to the Properties view and begin to edit one of the property > values. > > At this point the Properties view is dirty, since I've made a change, but > the editor should not be dirty as that change has not yet been propagated to > the editor. (If I hit <Escape>, the change is abandoned.) Once I hit > <enter>, the change should be applied, the editor should be dirty, and the > Properties is no longer be dirty (since it now reflects the value of the > editor). This makes absolutely sense, but deserves a new enhancement request and therefore we should separate it from this bug to get the current weird behavior fixed first. If we go the proposed way it would be possible to modify the Properties view so that it provides a more clever ISecondarySaveableSource adapter and some new API, which answers true in "isDirtyStateSupported" if and only if the Properties view has focus in its embedded editor and the embedded editor is "dirty". The new API is required for that because PropertySheetPage itself has no easy way to answer "I'm being edited & embedded editor content differs now from external one?", this need to be somehow computed from the data (IPropertySource objects) and probably can't be implemented in backward compatible way for custom implementations of IPropertySheetPage. But as said, this is for the new enhancement request.
> The second patch, based on the first one, tries to bring this controversial requirements fixed by introducing another API "layer". My only reservation here is the ISecondarySaveableSource as it's very specific — and I worry too specific — to the situation here. I could see a need for a part to want to control its dirty state independent of being a secondary part, though I don't have a real usecase. So I'm ok with this patch, using ISecondarySaveableSource, but it should be made internal and not API, and marked as experimental. Thanks against for tackling this thorny issue Andrey.
Gerrit change https://git.eclipse.org/r/75126 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e80a4bfc6ec76a2db6f22cfdbe79ee08c5697cb2
Gerrit change https://git.eclipse.org/r/75318 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c113a64d421edfdfc5bc7565b67dd2609bd25fd6
(In reply to Eclipse Genie from comment #27) > Gerrit change https://git.eclipse.org/r/75126 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=e80a4bfc6ec76a2db6f22cfdbe79ee08c5697cb2 (In reply to Eclipse Genie from comment #28) > Gerrit change https://git.eclipse.org/r/75318 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=c113a64d421edfdfc5bc7565b67dd2609bd25fd6 Thanks Patrik, Brian, Dirk for the review! I've pushed the changes now. Question: do we want to backport the patches to 4.6.1?
> Question: do we want to backport the patches to 4.6.1? +1 ;-)
(In reply to Phil Beauvoir from comment #30) > > Question: do we want to backport the patches to 4.6.1? > > +1 ;-) OK, let be more precise: Platform UI team, do we want to backport the patches to 4.6.1? Since now no new API were in game, we can even do it without PMC approval :-)
+1 to backporting. I managed to recreate the puzzling dirty marker on the properties view that cannot be cleared and it is baffling.
New Gerrit change created: https://git.eclipse.org/r/77036
New Gerrit change created: https://git.eclipse.org/r/77035
(In reply to Eclipse Genie from comment #33) > New Gerrit change created: https://git.eclipse.org/r/77036 (In reply to Eclipse Genie from comment #34) > New Gerrit change created: https://git.eclipse.org/r/77035 4.6.1 backport patches. (In reply to Brian de Alwis from comment #32) > +1 to backporting. Brian, thanks for feedback, I need your "oficial" +1 on each review for backporting. I've added you as reviewer on gerrit.
Backported to 4.6.1 via https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_6_maintenance&id=a6caf724e1deda57bdaa9cb36c979393ec2a12ad, https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_6_maintenance&id=d9b3efa8810ad8864886f3223c61d22a8f14b3f8
Gerrit change https://git.eclipse.org/r/77035 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=a6caf724e1deda57bdaa9cb36c979393ec2a12ad
Gerrit change https://git.eclipse.org/r/77036 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d9b3efa8810ad8864886f3223c61d22a8f14b3f8
New Gerrit change created: https://git.eclipse.org/r/77290
New Gerrit change created: https://git.eclipse.org/r/77291
Gerrit change https://git.eclipse.org/r/77291 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4890aabdea8f36d586ad0532de39b21ee63842db
New Gerrit change created: https://git.eclipse.org/r/77406
Gerrit change https://git.eclipse.org/r/77406 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4e6cc07f6b22c0e6caa1d6074bc6fbbd2c9fec28
(In reply to Andrey Loskutov from comment #31) > (In reply to Phil Beauvoir from comment #30) > > > Question: do we want to backport the patches to 4.6.1? > > > > +1 ;-) > > OK, let be more precise: Platform UI team, do we want to backport the > patches to 4.6.1? Since now no new API were in game, we can even do it > without PMC approval :-) Well, you did add new API: org.eclipse.ui.views.properties.PropertySheet.isDirtyStateSupported() You would have seen this if you correctly set the API Baseline to 4.6. It reports two errors: 1) Invalid @since 3.9 tag on isDirtyStateSupported(); expecting @since 3.8 2) The minor version should be incremented in version 3.8.101, since new APIs have been added since version 3.8.10 Fixing 2) will make 1) go away. One can also see the problem (bundle version warning for org.eclipse.ui.views) by looking at the API Tools Verification Report in our maintenance build: http://download.eclipse.org/eclipse/downloads/drops4/M20160727-1700/apitools/analysis/html/index.html
(In reply to Dani Megert from comment #44) > 1) Invalid @since 3.9 tag on isDirtyStateSupported(); expecting @since 3.8 > 2) The minor version should be incremented in version 3.8.101, since new > APIs have been added since version 3.8.10 > > Fixing 2) will make 1) go away. NOTE: That is not an option since master has 3.9.0.
(In reply to Dani Megert from comment #44) > Well, you did add new API: > org.eclipse.ui.views.properties.PropertySheet.isDirtyStateSupported() I haven't realized that, so I request your approval on API change in 4.6.1. > You would have seen this if you correctly set the API Baseline to 4.6. I have set it correctly. I guess you are running a 4.7 build with some PDE/JDT fixes? I'm trying to download latest build right now, but it will take some time. > It reports two errors: Probably it is a bug with default methods? I'm running 4.6.0 and target is 4.6.0, and I don't see any API errors. (In reply to Dani Megert from comment #45) > (In reply to Dani Megert from comment #44) > > 1) Invalid @since 3.9 tag on isDirtyStateSupported(); expecting @since 3.8 > > 2) The minor version should be incremented in version 3.8.101, since new > > APIs have been added since version 3.8.10 > > > > Fixing 2) will make 1) go away. > > NOTE: That is not an option since master has 3.9.0. Sorry, you mean we must change version in 4.6.1 to 3.9.0 but we can't do it because 4.7 has 3.9.0 too? But we can change 4.7.0 to 3.9.1 and it should be OK again?
(In reply to Andrey Loskutov from comment #46) > (In reply to Dani Megert from comment #44) > > You would have seen this if you correctly set the API Baseline to 4.6. > > I have set it correctly. but the wrong workspace :-(
New Gerrit change created: https://git.eclipse.org/r/78440
New Gerrit change created: https://git.eclipse.org/r/78443
(In reply to Eclipse Genie from comment #48) > New Gerrit change created: https://git.eclipse.org/r/78440 (In reply to Eclipse Genie from comment #49) > New Gerrit change created: https://git.eclipse.org/r/78443 @Dani: here are the proposed changes. Can you please review?
(In reply to Andrey Loskutov from comment #50) > (In reply to Eclipse Genie from comment #48) > > New Gerrit change created: https://git.eclipse.org/r/78440 > > (In reply to Eclipse Genie from comment #49) > > New Gerrit change created: https://git.eclipse.org/r/78443 > > @Dani: here are the proposed changes. Can you please review? The PMC first needs to decide how we handle the version numbering in the light of the new Update releases. For the SRs, where API changes and new features were the rare exception case, we never increased the minor version and simply suppressed the API Tools errors. With more API and feature changes being made, we might want to change that. I will keep you posted.
(In reply to Dani Megert from comment #51) > (In reply to Andrey Loskutov from comment #50) > > (In reply to Eclipse Genie from comment #48) > > > New Gerrit change created: https://git.eclipse.org/r/78440 > > > > (In reply to Eclipse Genie from comment #49) > > > New Gerrit change created: https://git.eclipse.org/r/78443 > > > > @Dani: here are the proposed changes. Can you please review? > > The PMC first needs to decide how we handle the version numbering in the > light of the new Update releases. This is tracked by bug 499164.
Andrey, please send a request to the PMC mailing list for the API change.
(In reply to Dani Megert from comment #53) > Andrey, please send a request to the PMC mailing list for the API change. Done.
(In reply to Andrey Loskutov from comment #54) > (In reply to Dani Megert from comment #53) > > Andrey, please send a request to the PMC mailing list for the API change. > > Done. I did not see a new message on the list yet.
(In reply to Dani Megert from comment #55) > (In reply to Andrey Loskutov from comment #54) > > (In reply to Dani Megert from comment #53) > > > Andrey, please send a request to the PMC mailing list for the API change. > > > > Done. > > I did not see a new message on the list yet I hope this was right list: http://dev.eclipse.org/mhonarc/lists/eclipse-pmc/msg02668.html
New Gerrit change created: https://git.eclipse.org/r/78959
New Gerrit change created: https://git.eclipse.org/r/78961
New Gerrit change created: https://git.eclipse.org/r/78960
(In reply to Eclipse Genie from comment #57) > New Gerrit change created: https://git.eclipse.org/r/78959 > New Gerrit change created: https://git.eclipse.org/r/78961 > New Gerrit change created: https://git.eclipse.org/r/78960 Reverted changes from 4.6.1 as requested in https://dev.eclipse.org/mhonarc/lists/eclipse-pmc/msg02685.html @Dani: do I need +1 from Platform UI committers for revert commits? > The Javadoc for 'PropertySheet#isDirtyStateSupported()' is wrong: > @returnreturns {@code false} by default. Will fix. > The API class 'PropertySheet' leaks internal stuff from > 'ISecondarySaveableSource'. An API must not inherit from > or implement internal types. @Dani: since the new API is needed here to fix the bug, I assume you mean that ISecondarySaveableSource must be made API (moved from internal package)?
(In reply to Andrey Loskutov from comment #60) > (In reply to Eclipse Genie from comment #57) > > New Gerrit change created: https://git.eclipse.org/r/78959 > > New Gerrit change created: https://git.eclipse.org/r/78961 > > New Gerrit change created: https://git.eclipse.org/r/78960 > > Reverted changes from 4.6.1 as requested in > https://dev.eclipse.org/mhonarc/lists/eclipse-pmc/msg02685.html > > @Dani: do I need +1 from Platform UI committers for revert commits? No, fine with me. > > The API class 'PropertySheet' leaks internal stuff from > > 'ISecondarySaveableSource'. An API must not inherit from > > or implement internal types. > > @Dani: since the new API is needed here to fix the bug, I assume you mean > that ISecondarySaveableSource must be made API (moved from internal package)? Since you had it internal, do you really need an interface, or is adding it to the class enough?
(In reply to Dani Megert from comment #61) > > @Dani: since the new API is needed here to fix the bug, I assume you mean > > that ISecondarySaveableSource must be made API (moved from internal package)? > > Since you had it internal, do you really need an interface, or is adding it > to the class enough? Yes, interface is needed. Current code in org.eclipse.ui.internal.SaveableHelper (from org.eclipse.ui.workbench) uses this internal API defined in same plugin, but PropertySheet is coming from org.eclipse.ui.views plugin, so we can't access PropertySheet from org.eclipse.ui.workbench. So my first idea was to make it public API, but later during thr review we decided to make it internal because of the limited use.
Gerrit change https://git.eclipse.org/r/78959 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3aedd2a119c8a4bb6eedb1802c5e371d148565fd
Gerrit change https://git.eclipse.org/r/78960 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=95dc0f989b45182c0a095182328830277bd29a8f
Gerrit change https://git.eclipse.org/r/78961 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3b94b08bd36e82fffd41893c775cbf0b820960b9
New Gerrit change created: https://git.eclipse.org/r/78979
(In reply to Eclipse Genie from comment #66) > New Gerrit change created: https://git.eclipse.org/r/78979 Promoted ISecondarySaveableSource to public API.
(In reply to Andrey Loskutov from comment #67) > (In reply to Eclipse Genie from comment #66) > > New Gerrit change created: https://git.eclipse.org/r/78979 > > Promoted ISecondarySaveableSource to public API. Looks good to me, but minor version must not be increased, see bug 499164.
(In reply to Dani Megert from comment #68) > (In reply to Andrey Loskutov from comment #67) > > (In reply to Eclipse Genie from comment #66) > > > New Gerrit change created: https://git.eclipse.org/r/78979 > > > > Promoted ISecondarySaveableSource to public API. > > Looks good to me, but minor version must not be increased, see bug 499164. Thanks. I assume you mean that "must not be increased" for the backport only?
Gerrit change https://git.eclipse.org/r/78979 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=a72dda9b94f07f1c95d7f51f9380a17eec581c88
New Gerrit change created: https://git.eclipse.org/r/79163
New Gerrit change created: https://git.eclipse.org/r/79162
New Gerrit change created: https://git.eclipse.org/r/79161
New Gerrit change created: https://git.eclipse.org/r/79164
(In reply to Dani Megert from comment #68) > (In reply to Andrey Loskutov from comment #67) > > (In reply to Eclipse Genie from comment #66) > > > New Gerrit change created: https://git.eclipse.org/r/78979 > > > > Promoted ISecondarySaveableSource to public API. > > Looks good to me, but minor version must not be increased, see bug 499164. Commits below are reverting reverted backport commits on 4_6 branch: https://git.eclipse.org/r/79161 https://git.eclipse.org/r/79162 https://git.eclipse.org/r/79163 This commit below is the backported version of https://git.eclipse.org/r/78979 *without* incremented minor version but with updated API problem filters: https://git.eclipse.org/r/79164 I plan to merge it to 4.6.1 as soon as hudson builder finishes, so that the changes can go into 4.6.1 RC1 tomorrow.
Gerrit change https://git.eclipse.org/r/79161 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=bac31b4f952d74d52e6b699cb9f3ede4e6c7118c
Gerrit change https://git.eclipse.org/r/79162 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=9105a945cb29195342d18f928268734ea3028cd6
Gerrit change https://git.eclipse.org/r/79163 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=25a79ea97f00f8bdfcea3e5526f88b1480bc6e1f
Gerrit change https://git.eclipse.org/r/79164 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f0b0d2e04614b83869d6d745ba0fb0612d6cc9b4
Now everything is merged back into 4.6.1.
Verified in 4.6.1 with M20160817-0420
