Community
Participate
Working Groups
Currently, the workbench calls the saveState(IMemento) method on each visible workbench part when the workbench closes and also periodically from the workbench's Auto-Save Job. Specifically, it is *not* called on a workbench part when the part closes. Not calling saveState automatically when a view closes is an asymmetry with init(IViewSite, IMemento) being called by the workbench each time a view opens. This asymmetry is further emphasised by the fact that in E4, a part's method annotated with @PersistState *is* called when the part closes. As originally discussed in bug 543109, this needs to be fixed; i.e., whenever a part is being closed, the workbench should automatically call the part's saveState(IMemento) method. I'm going to provide a patch.
New Gerrit change created: https://git.eclipse.org/r/135377
I spent some time debugging the whole thing and now I realize that, contrary to the method name, both ViewReference.persist() and EditorReference.persist() do not actually *persist* anything. They just put a memento string to the persistedState map of the underlying MPart object. The actual persistence occurs when the updated EMF model gets saved to the workbench.xmi file, i.e., when the workbench closes or Workbench.persistWorkbenchModel() is called by the auto-save job. Also, debugging suggests that for views, even when a view is closed, the workbench keeps the underlying MPart instance in the model. When we call, for example, view = page.showView(ID); page.hideView(view); view2 = page.showView(ID); both view and view2 instances share the same underlying MPart object. So, when ViewReference.persist() is called on view close, a memento string is put in the persistedState map of the underlying MPart instance, which gets reused when the view is opened again in the same session. This all happens entirely in memory. No persistence to a file occurs until the workbench closes or the auto-save job is run. For editors, the underlying MPart is discarded when the editor is closed. A new editor instance always has a new underlying MPart object. So, calling EditorReference.persist() on editor close just does not make sense; it would just be an extra work that is discarded right away. Interestingly, the workbench provides the IWorkbenchPage.getEditorState(..)/openEditors(.., IMemento[], ..) API since 3.8.2, which clients might use to reuse the editor state when opening an editor. I think that editors should be excluded from the scope of this bug. Regarding views, I think that it still makes sense to call ViewReference.persist() on view close, as originally proposed, despite the fact that the actual persistence to a file would be deferred until the workbench closes or the auto-save job is run. Otherwise, when a view is reopened in the same session, it would be restored to the state last saved by the auto-save job or the state of the previous session, not the state at the moment it was closed, which might be confusing to the user. I do not think that we should actually persist the workbench model (i.e., call Workbench.persistWorkbenchModel()) when a view closes; it might be a bit heavyweight. If a crash occurs, the view would just be restored to the last saved state. If there are no objections, I will provide an updated patch.
Another option would be to ensure that no memento is passed to the init(IViewSite, IMemento) method except for the case of restoring a view that was open when the previous session ended; in all other cases, a view would be created in a default state. That would provide symmetry and that's how things seemingly worked in the past (before E4?). Of course, there would be no need to call ViewReference.persist() on view close in that case.
(In reply to Vladimir Piskarev from comment #3) > Another option would be This alternative approach might be supported by the Javadoc of the init(IViewSite, IMemento) method, which says: "A memento is passed to the view which contains a snapshot of the views state from a *previous* session." I may be wrong, but reading between the lines suggests that this method can be called with a non-null memento only for restoration of an open view from the previous session. In that case, one could argue that the current behavior violates the contract, since a view that was closed and is being reopened in the same session may currently be passed a non-null memento (which may even originate from that same session because of the auto-save job).
I debugged the relevant code in Eclipse 3.8.2 and can confirm that a non-null memento is passed to the init(IViewSite, IMemento) method only in the case of restoring an open view from the previous session; in all other cases, a view is always created in a default state (i.e., null memento is passed to init).
I think this is indeed a regression in Eclipse 4.x. To reproduce, launch Eclipse with a fresh workspace, close it and launch again to make sure there is a saved workbench state. In Java perspective, close the Package Explorer view and then open it again in the same session. In Eclipse 3.8.2, init(IViewSite, IMemento) is called with null memento and the view is created in a default state. In Eclipse 4.x, init(IViewSite, IMemento) is called with a non-null memento and the view is created in a previously saved state (either the state from the previous session or the state saved by the auto-save job in the current session). In 3.8.2, the method ViewFactory.createView(String id, String secondaryId) contains the following code: String key = getKey(id, secondaryId); IMemento memento = (IMemento) mementoTable.get(key); ref = new ViewReference(this, id, secondaryId, memento); mementoTable.remove(key); Once a view reference is created for a given view, the memento (if any) is removed from the memento table. If another view reference is created for the same view in the same session, it will have no memento. In 4.x, the constructor ViewReference(IEclipseContext windowContext, IWorkbenchPage page, MPart part, ViewDescriptor descriptor) contains the following code: String mementoString = part.getPersistedState().get(MEMENTO_KEY); if (mementoString != null) { try { memento = XMLMemento.createReadRoot(new StringReader(mementoString)); } catch (WorkbenchException e) { WorkbenchPlugin.log(e); } } But, as noted in comment 2, when a view is closed, the workbench keeps the underlying MPart instance in the model, together with the memento string (if any) stored in the persistedState map of the MPart. If another view reference is created for the same view in the same session, it will have the same MPart and, hence, the same memento string. It does not seem that this behavior change in view state management was intentional. Moreover, the new behavior seems to violate the contract of the IViewPart.init(IViewState, IMemento) method, as noted in comment 4. So, does not it look like the actual bug that should be fixed?
Thanks Vlad. Your analysis seems solid. However, is it a bug or is it a feature? It seems that no one objects that views are now called with a non null memento. Most views have a button to go into the default state ([-] button for package explorer) in case you suggest that closing and opening a view was the old way of resetting the view. Also, what is the definition of a session? Is it a view-open/close cycle or a workbench-start/stop cycle. If you think it is a bug that should be fixed does that then not contradict the purpose of this bug?
(In reply to Wim Jongman from comment #7) Thanks for the reply Wim. > is it a bug or is it a feature? That is the question. My analysis suggests that it was not intentionally designed as a feature, and looks like a regression. This needs a judge :-) I think a central question here is whether the current behavior violates the contract of the IViewPart.init(IViewSite, IMemento) method. Unfortunately, the Javadoc is a bit vague, but we can appeal to the archeology and study the Eclipse 3.x behavior to attempt to gain more understanding of the original design. In particular, > what is the definition of a session? Is it a view-open/close cycle or > a workbench-start/stop cycle. I think it is safe to say that the API contract meant a workbench-start/stop cycle. At least, that's how Eclipse 3.x behaves. Also, we have a further evidence in bug 30042 and bug 307319 that were both closed as WONTFIX in Eclipse 3.x (see bug 30042 comment 7 and bug 307319 comment 3 in particular). In any case, the current behavior is buggy in that an incorrect memento may be passed to the view's init method (that was the observation that initially led to the whole discussion and to this bug in particular). > If you think it is a bug that should be fixed does that then not contradict > the purpose of this bug? Yes, those two are mutually exclusive ways to fix the current behavior in view state management. I have just been trying to understand alternatives and find the root cause. Now that the groundwork is more or less done, it needs to be decided what (if any) to do with this mess :-) > Most views have a button to go into the default state ([-] button for > package explorer) in case you suggest that closing and opening a view was > the old way of resetting the view. I'm afraid I did not entirely get it. If you meant the "Collapse All" button, the view state might be much more than that. For example, the memento string for the Package Explorer includes the following: <view group_libraries="1" layout="2" linkWithEditor="0" rootMode="1" workingSetName="Aggregate for window 1548353431379"> <customFilters userDefinedPatternsEnabled="false"> <xmlDefinedFilters> <child filterId="org.eclipse.jdt.ui.PackageExplorer.StaticsFilter" isEnabled="false"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.NonJavaProjectsFilter" isEnabled="false"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer_patternFilterId_.*" isEnabled="true"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.NonSharedProjectsFilter" isEnabled="false"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.SyntheticMembersFilter" isEnabled="true"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.ContainedLibraryFilter" isEnabled="false"/> <child filterId="org.eclipse.jdt.internal.ui.PackageExplorer.HideInnerClassFilesFilter" isEnabled="true"/> <child filterId="org.eclipse.jdt.internal.ui.PackageExplorer.EmptyInnerPackageFilter" isEnabled="true"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.ClosedProjectsFilter" isEnabled="false"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.EmptyLibraryContainerFilter" isEnabled="true"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.PackageDeclarationFilter" isEnabled="true"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.ImportDeclarationFilter" isEnabled="true"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.NonJavaElementFilter" isEnabled="false"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.LibraryFilter" isEnabled="false"/> <child filterId="org.eclipse.pde.ui.BinaryProjectFilter1" isEnabled="false"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.CuAndClassFileFilter" isEnabled="false"/> <child filterId="org.eclipse.jdt.internal.ui.PackageExplorer.EmptyPackageFilter" isEnabled="false"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.NonPublicFilter" isEnabled="false"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.LocalTypesFilter" isEnabled="false"/> <child filterId="org.eclipse.pde.ui.ExternalPluginLibrariesFilter1" isEnabled="true"/> <child filterId="org.eclipse.jdt.ui.PackageExplorer.FieldsFilter" isEnabled="false"/> </xmlDefinedFilters> </customFilters> </view>
(In reply to Vladimir Piskarev from comment #8) > memento string for the Package Explorer includes the following: > > <view group_libraries="1" layout="2" linkWithEditor="0" rootMode="1" .. > </view> So IIUC: in 3.8.2 this filter would be persisted between workbench sessions but closing and opening a view in the same session would throw away that information and rebuild the view from scratch? That does not sound desirable. Having said this, there is also the perspective reset. In this case, I would expect that all views would be opened in a default state. What do you see when a perspective is reset? Is it different from closing and opening a perspective?
(In reply to Wim Jongman from comment #9) > So IIUC: in 3.8.2 this filter would be persisted between workbench sessions > but closing and opening a view in the same session would throw away that > information and rebuild the view from scratch? Yes. > That does not sound desirable. Perhaps, but at least that behavior is consistent and exactly matches the API contract. > Having said this, there is also the perspective reset. In this case, I would > expect that all views would be opened in a default state. What do you see > when a perspective is reset? Is it different from closing and opening a > perspective? The behavior of the perspective reset is also different between 3.8.2 and 4.x. Let's start with the default Java perspective. Close the Package Explorer view and reset the perspective. 3.8.2: a ViewReference is created for the Package Explorer with null memento. IViewPart.init(IViewReference, IMemento) is called on the Package Explorer view with null memento. 4.x: a ViewReference is created for *each and every* view in the default Java perspective with a non-null memento obtained from the corresponding MPart in the model. IViewPart.init(IViewReference, IMemento) is called on the Package Explorer view with a non-null memento. In other words, the Package Explorer is opened in a "last saved" state and not in a default state. Now, without changing anything, let's reset the perspective again. 3.8.2: nothing happens; no ViewReferences are created. 4.x: a ViewReference is created for *each and every* view in the default Java perspective with a non-null memento obtained from the corresponding MPart in the model.
The rabbit hole deepens. Do you have suggestions?
I would suggest trying to restore (i.e., emulate) the original 3.x behavior in regard to the view state management. Risking to repeat myself, I think that the current behavior violates the contract of the IViewPart.init(IViewSite, IMemento) method, whereas the 3.x behavior is consistent. If a behavior violates a contract, it is a bug. Unfortunately, this was introduced a long time ago, so fixing it might (in theory, at least) affect existing clients. That's what makes me a bit cautious about it. On the other hand, I do not think that just keeping the current buggy behavior would be a much better choice. This is a mess, and there seems to be no perfect way out. I also think that my original proposal of calling saveState on close was (mis)guided by an incomplete understanding and could potentially make things even worse than they currently are by masking the underlying issue with superficial consistency. An entirely new API would probably be needed for saving/restoring the view state in a view-close/open cycle. That's all just my two cents, of course.
(In reply to Vladimir Piskarev from comment #12) > superficial consistency. An entirely new API would probably be needed for > saving/restoring the view state in a view-close/open cycle. > > That's all just my two cents, of course. You could probably hook into the dispose method to save state when closing and restore while creating the parts' controls.
(In reply to Wim Jongman from comment #13) > (In reply to Vladimir Piskarev from comment #12) > > > superficial consistency. An entirely new API would probably be needed for > > saving/restoring the view state in a view-close/open cycle. > > > > That's all just my two cents, of course. > > You could probably hook into the dispose method to save state when closing > and restore while creating the parts' controls. To manage the view state "manually" for a specific view? That is certainly an option.
Hi Wim, It looks like I have changed my mind on this. Having debugged it a bit more, I see that the workbench keeps a separate MPart instance per view per *workbench window*. Thus, the argument in bug 30042 comment 7 regarding multiple instances of the same view in different windows seems to be not that relevant now, since in 4.x, it is always known for which window the view state is saved/restored. Also, it seems that the argument in bug 307319 comment 3 against calling saveState/init whenever the view is closed/opened within the running session is no longer relevant, because, as we know, the init is already called in 4.x in this case; the damage (if any) has occurred. I now think that the Eclipse 4.x behavior can actually be made consistent by applying the patch in Gerrit. It would just be a different behavior compared to 3.x, but it would nonetheless be consistent. Regarding resetting a perspective: it *seems* that the goal of the perspective reset is just to restore a default arrangement of views, not their settings. Note that those views in the default layout that are already open don't get re-initialized on perpective reset both in 3.x and 4.x. It is just that a view which was closed and is being opened in the same session is initialiazed in the default state in 3.x and in the remembered state in 4.x, and that includes the case when a perspective is reset. But the behavior is consistent in both cases. So, the only missing piece to consistency in view management in 4.x seems to be saving the view state when a view closes, exactly what the patch does.
I am about to release this patch from Validimir https://git.eclipse.org/r/135377 for which he provided the following commit message: Fixed the asymmetry in the workbench calling IViewPart.init(IViewSite, IMemento) when a view part opens, but not calling saveState(IMemento) when the view part closes within the running workbench session. Modified WorkingSetTest to make testDeletedAndRecreated and testMissingProjectsInWorkingSet pass again. They contained seemingly needless calls to DisplayHelper.runEventLoop, which allowed the asyncExec lambda scheduled in WorkingSetActionProvider.restoreState to execute. This messed up the viewer input. The primary cause for the test failures was the implicit assumption that saveState will not be called on the tested navigator view. This assumption was wrong anyway because of the workbench auto-save job, but calling saveState when the view is closed in NavigatorTestBase.tearDown() made the tests fail predictably.
> Validimir, I don't get how @PersistState and workbench save work > together. Can you explain? > > When is @PersistState executed and why does the workbench save not > use this mechanism? Wim, as far as I can see, Workbench.persist() saves the state in a specific order: first, open editors; then, workbench state; last, open views. Probably, that's why Workbench.persist() explicitly calls EditorReference.persist() and ViewReference.persist() to update the saved state of the editors and views, rather than uses the @PersistState mechanism. A method annotated with @PersistState is called whenever a part is disposed (it is called from PartRenderingEngine.safeRemoveGui), either when a part itself is closed or when the containing workbench window or the workbench is closed. When the workbench closes, Workbench.persist() has already been called before a @PersistState method is called. Hence, to avoid calling ViewReference.persist twice in the row, we check in CompatibilityView.persistState() that the workbench is not closing.
Since there are not many people on this bug, I have made an announcement on the platform-dev list. I am confident about this change and without objections, I will release it at the end of the day.
(In reply to Wim Jongman from comment #18) > Since there are not many people on this bug, I have made an announcement on > the platform-dev list. I am confident about this change and without > objections, I will release it at the end of the day. I plan to take a look this week. Please wait with merging. Also, we contribute to M2 this week and this change might cause issues (just a feeling, as said, I did not yet look at it in detail).
(In reply to Dani Megert from comment #19) > (In reply to Wim Jongman from comment #18) > > Since there are not many people on this bug, I have made an announcement on > > the platform-dev list. I am confident about this change and without > > objections, I will release it at the end of the day. > I plan to take a look this week. Please wait with merging. Also, we > contribute to M2 this week and this change might cause issues (just a > feeling, as said, I did not yet look at it in detail). I suggest to take it in M2 so that we have time to play with it. E.g. we (our company) have several products build on milestones.
This breaks an important scenario that works in 3.8.2 and latest 4.11 build (I20190129-1800): 0. Start with a new workspace and close the Welcome page 1. Uncheck 'Show 'Referenced Libraries' Node' (option) on Package Explorer (PE) 2. Close PE 3. Reopen PE ==> option is unchecked [3.8.2] ==> option is unchecked [4.11 build] ==> option is unchecked [4.11+fix] 4. Window .> New Window ==> option in PE is unchecked [3.8.2] ==> option in PE is unchecked [4.11 build] ==> option in PE is unchecked [4.11+fix] 5. Close PE in first window 6. Check option in in the second window and close the window 7. Open PE in the first window ==> option is checked [3.8.2] ==> option is checked [4.11 build] ==> BUG: option is *** unchecked *** [4.11+fix]
(In reply to Dani Megert from comment #21) Thanks, Dani. Where did you get this scenario from? Is this written down somewhere? Manual scenarios can be really valuable when doing cases like these where unit tests seem to fail.
(In reply to Wim Jongman from comment #22) > (In reply to Dani Megert from comment #21) > > Thanks, Dani. Where did you get this scenario from? Is this written down > somewhere? > > Manual scenarios can be really valuable when doing cases like these where > unit tests seem to fail. I created it to verify this fix. Another test case is to have two windows where the PEs don't have the same option value and then restart. The values must be preserved, which is the case with the fix.
(In reply to Dani Megert from comment #21) > This breaks an important scenario that works in 3.8.2 and latest 4.11 build > (I20190129-1800): > ... > ==> BUG: option is *** unchecked *** [4.11+fix] Thanks, Dani. With a slightly modified scenario, this can also be reproduced in the latest 4.11 build: 0. Start with a new workspace and close the Welcome page 1. Uncheck 'Show 'Referenced Libraries' Node' (option) on Package Explorer (PE) *** Restart the workbench *** 2. Close PE Etc. Alternatively, let at least 5 minutes to pass before step 5 for the workbench auto-save job to kick in. Looking at the PE code, it seems to be a consequence of storing the 'Show Libraries Node' option both in IDialogSettings and in IMemento.
(In reply to Vladimir Piskarev from comment #24) You say this happens anyway after the workspace has done a save.
(In reply to Wim Jongman from comment #25) > (In reply to Vladimir Piskarev from comment #24) > > You say this happens anyway after the workspace has done a save. Yes, you just need a saved memento passed to the view's init method to reproduce it.
(In reply to Vladimir Piskarev from comment #26) > (In reply to Wim Jongman from comment #25) > > (In reply to Vladimir Piskarev from comment #24) > > > > You say this happens anyway after the workspace has done a save. > > Yes, you just need a saved memento passed to the view's init method to > reproduce it. Just to be clear: that would not justify this fix but surface a bug in the auto-save feature.
(In reply to Dani Megert from comment #27) > Just to be clear: that would not justify this fix but surface a bug in the > auto-save feature. Yes, that would not justify this fix. But I don't think it surfaces a bug in the auto-save feature. It seems to the good old code in the Package Explorer that could work fine in Eclipse 3.x, but is now broken in 4.x. > Looking at the PE code, it seems to be a consequence of storing the 'Show Libraries Node' option both in IDialogSettings and in IMemento.
(In reply to Vladimir Piskarev from comment #28) > (In reply to Dani Megert from comment #27) > > Just to be clear: that would not justify this fix but surface a bug in the > > auto-save feature. > >It seems to the good old code in the Package Explorer > that could work fine in Eclipse 3.x, but is now broken in 4.x. Can you name a view in the SDK where my given scenario also works with your fix? Anyway, given the PE gets broken it will for sure also break other views, so, -2 for any fix that breaks my given steps.
(In reply to Vladimir Piskarev from comment #24) > Alternatively, let at least 5 minutes to pass before step 5 for the > workbench auto-save job to kick in. Not really. I've set the interval to 1 and then tested on 3.8.2 and latest 4.11 build and it works like in my test case. Maybe you had your "fix" loaded.
(In reply to Dani Megert from comment #29) > Anyway, given the PE gets broken it will for sure also break other views, > so, -2 for any fix that breaks my given steps. I respectfully get it. Just a bit more comments. Please see the extended discussion above for why views that might work in Eclipse 3.x can now be broken in Eclipse 4.x, regardless of this patch. A short version is: the damage has occurred when E4 was introduced. It was indeed a regression since, in Eclipse 3.x, a non-null memento can be passed to the IViewPart.init(IViewSite, IMemento) method only in the case of restoring an open view from the previous session; in all other cases, a view is always created in a default state (i.e., null memento is passed to init). It no longer works that way in 4.x. The PE is broken already in 4.x, as the modified scenario demonstrates. Just restart the workbench when the PE is visible with the 'Show Libraries Node' option unchecked (i.e., when it has a non-default value) to ensure it is saved in a memento before proceeding with steps 5-7. Your given steps work in 4.x only because no workbench save has occurred, i.e., saveState has not been called on the PE. In 3.x, they work only because a view that was closed and reopened in the same workbench session is always created in a default state (null memento is passed to init).
(In reply to Dani Megert from comment #30) > (In reply to Vladimir Piskarev from comment #24) > > Alternatively, let at least 5 minutes to pass before step 5 for the > > workbench auto-save job to kick in. > Not really. I've set the interval to 1 and then tested on 3.8.2 and latest > 4.11 build and it works like in my test case. Maybe you had your "fix" > loaded. Nope. I have double-checked it with 4.11. Probably, just a timing issue. You need the auto-save job to kick in before step 5 *and* when PE is visible in the first window and has the option unchecked. I set a breakpoint on the Workbench.persist method to make sure the method has executed when the above condition is satisfied.
(In reply to Vladimir Piskarev from comment #32) > (In reply to Dani Megert from comment #30) > > (In reply to Vladimir Piskarev from comment #24) > > > Alternatively, let at least 5 minutes to pass before step 5 for the > > > workbench auto-save job to kick in. > > Not really. I've set the interval to 1 and then tested on 3.8.2 and latest > > 4.11 build and it works like in my test case. Maybe you had your "fix" > > loaded. > > Nope. I have double-checked it with 4.11. > > Probably, just a timing issue. You need the auto-save job to kick in before > step 5 I waited long enough. If the save job did not do it's job after waiting more than 2 minutes we have another problem. Also, I tested with official builds and not out of my development workspace. Please post YOUR exact detailed steps and then maybe Wim can verify. Maybe it could also be an OS specific thing, but I doubt that.
(In reply to Dani Megert from comment #33) > Please post YOUR exact detailed steps and then maybe Wim can verify. OK, I will post the detailed steps tomorrow, but if this cannot be verified reliably, what about just restarting the workbench right after step 1? This has the same effect of saving the view state in a memento and should always be reproducible.
Just for the record, and to be absolutely clear: fundamentally, the patch does not make things any worse than they currently are in 4.x regarding the view state management. In fact, it brings consistency to the existing 4.x behavior, which is already a different behavior compared to 3.x (see comment 6 and the related discussion for more details about the difference in behavior of 3.x and 4.x). Some legacy views that were coded to 3.x assumptions can be broken in 4.x anyway. It's just that the patch can surface the breakage for a specific view in some additional scenarios, but the view would have already been broken, with or without the patch. Basically, either the legacy views needs to be fixed or the original 3.x view state management behavior needs to restored in 4.x (the latter can, of course, break other existing views). Whether this patch gets accepted or not.
(In reply to Dani Megert from comment #33) > > Please post YOUR exact detailed steps and then maybe Wim can verify. > > Maybe it could also be an OS specific thing, but I doubt that. I will take a look tomorrow as well. However, I just tested that if you annotate a method in your 3.x view with @persiststate it gets called when the view is closed. So if this is allowed in 3x then having @PersistState on a super might run in trouble according to the @PersistState Javadoc [1] which states that only one method will be called. [1] http://eclip.se/fm
(In reply to Vladimir Piskarev from comment #34) > (In reply to Dani Megert from comment #33) > > Please post YOUR exact detailed steps and then maybe Wim can verify. > > what about just restarting the workbench right after step 1? This > has the same effect of saving the view state in a memento and should always > be reproducible. In all 3 versions the option will be disabled as expected. So, I don't see what that helps here. (In reply to Vladimir Piskarev from comment #35) > Just for the record, and to be absolutely clear: fundamentally, the patch > does not make things any worse than they currently are in 4.x regarding the > view state management. It makes it worse, see my steps. Note that most views in the IDE are 3.x views. I don't deny that auto-save might have an undesired side effect, but I could not see it (yet).
(In reply to Dani Megert from comment #37) > (In reply to Vladimir Piskarev from comment #34) > > what about just restarting the workbench right after step 1? This > > has the same effect of saving the view state in a memento and should always > > be reproducible. > In all 3 versions the option will be disabled as expected. So, I don't see > what that helps here. > > > (In reply to Vladimir Piskarev from comment #35) > > Just for the record, and to be absolutely clear: fundamentally, the patch > > does not make things any worse than they currently are in 4.x regarding the > > view state management. > It makes it worse, see my steps. The Package Explorer is already broken in Eclipse 4.x, as can be seen from the following steps to reproduce using the latest 4.11 build (I20190131-0130): 0. Start with a new workspace and close the Welcome page 1. Uncheck 'Show 'Referenced Libraries' Node' (option) on Package Explorer (PE) 2. *** Restart the workbench *** 3. Close PE 4. Reopen PE ==> option is unchecked [4.11 build] 5. Window .> New Window ==> option in PE is unchecked [4.11 build] 6. Close PE in first window 7. Check option in in the second window and close the window 8. Open PE in the first window ==> BUG: option is *** unchecked *** [4.11 build] In other words, it is enough for the user to restart the workbench whenever the PE is visible in the first window and the option has a non-default value (i.e., is unchecked) to reproduce the exact same bug without the patch.
For completeness, here are the steps to reproduce the same bug even without restarting the workbench using the latest 4.11 build (I20190131-0130): 0. Start with a new workspace and close the Welcome page 1. Uncheck 'Show 'Referenced Libraries' Node' (option) on Package Explorer (PE) 2. Close PE 3. Reopen PE ==> option is unchecked [4.11 build] 4. Window .> New Window ==> option in PE is unchecked [4.11 build] 5. *** Wait for at least 5 minutes for the auto-save job to kick-in *** 6. Close PE in first window 7. Check option in in the second window and close the window 8. Open PE in the first window ==> BUG: option is *** unchecked *** [4.11 build]
Thanks for the steps. I can reproduce it in 4.x but in 3.x it works. So, you have found a severe bug in 4.x that needs to be fixed. The bug is that in a window that has been persisted, the reopened view gets that state instead of what he chose last. This makes it impossible for the user to change a setting and later open it with the last chosen state. This does not affect newly opened windows (until persisted). ==> We don't need two windows, simply changing the option and restart destroys the behavior. Would you be willing to fix that bug? We can then see whether the proposed fix doesn't introduce new problems.
I can confirm that the minimal steps to reproduce it in the latest 4.11 build (I20190131-0130) would be: 0. Start with a new workspace and close the Welcome page 1. Uncheck 'Show 'Referenced Libraries' Node' (option) on Package Explorer (PE) 2. *** Restart the workbench *** 3. Check the option on PE 4. Close PE 5. Reopen PE ==> BUG: option is *** unchecked *** [4.11 build] Interestingly, ==> option is unchecked in [4.11+fix] The PackageExplorerPart.init(IViewSite, IMemento) method contains the following code: super.init(site, memento); if (memento == null) { String persistedMemento= fDialogSettings.get(TAG_MEMENTO); if (persistedMemento != null) { try { memento= XMLMemento.createReadRoot(new StringReader(persistedMemento)); } catch (WorkbenchException e) { // don't do anything. Simply don't restore the settings } } } fMemento= memento; if (memento != null) { restoreLayoutState(memento); ... } While the PackageExplorerPart.dispose() method contains the following code: XMLMemento memento= XMLMemento.createWriteRoot("packageExplorer"); saveState(memento); StringWriter writer= new StringWriter(); try { memento.save(writer); fDialogSettings.put(TAG_MEMENTO, writer.getBuffer().toString()); } catch (IOException e) { } Ironically, this code essentially tries to "manually" replicate the behavior that is already available "for free" in Platform UI in [4.11+fix]: save the view state on close and restore it on reopen. In 3.x, it works because null memento is always passed to IViewPart.init when a view is reopened in the same session, so the PE state is obtained from the dialog settings where it was "manually" saved on disposal of the last closed instance. In 4.x, it no longer works because a "wrong" memento (a saved memento from the previous workbench session) is passed to init. In 4.11+fix, it works because the "right" memento (a saved memento from the last closed instance in the given window) is passed to init. I just realized that your original steps with two windows actually work as expected in [4.11+fix], since in E4, the view state is always managed (saved/restored) separately for each window. Hence, the state of the closed view instance is also saved *per window* in [4.11+fix]. So, when PE is closed in the first window and then reopened in the first window, it is restored to the state saved for the first window (with the option unchecked), regardless of the state of the second window. It's an E4 feature; the fix just uses the standard mechanism. The same mechanism is used by workbench save. If you would prefer to preserve the current PE behavior of saving the view state per the workbench and not separately for each window, the only option seems to be to remove the initial check "if (memento == null)" in the PackageExplorerPart.init and always manage the state "manually" via the dialog settings. That's because, in E4, the memento passed to the init method is always *per window*, with or without the fix. In summary, if you would find it acceptable for the PE state to be managed separately for each workbench window, there is nothing to fix; in [4.11+fix] all the given scenarios work as expected. Otherwise, a small fix would be needed in PE.
(In reply to Vladimir Piskarev from comment #41) > Interestingly, > ==> option is unchecked in [4.11+fix] Interestingly, ==> option is *checked* in [4.11+fix] I'm sorry.
OK, if we accept the 4.x per window state, then things work much better with your fix. The "bug" I reported in comment 40 also affects other views, like e.g. the Packages view. With your fix that problem is gone. Wim, please also have another/final look before we merge the fix.
(In reply to Dani Megert from comment #43) > OK, if we accept the 4.x per window state, then things work much better with > your fix. The "bug" I reported in comment 40 also affects other views, like > e.g. the Packages view. With your fix that problem is gone. > > Wim, please also have another/final look before we merge the fix. I have played out these scenarios with the same result. I guess it is too late to get this in M2? I have one concern left which I described in comment #36. I will do some finals tests for that. I expect it will be ok since the patched CompatibilityView is not in the inheritance path of ViewPart.
(In reply to Wim Jongman from comment #44) > I have one concern left which I described in comment #36. I will do some > finals tests for that. I expect it will be ok since the patched > CompatibilityView is not in the inheritance path of ViewPart. I'm sorry I have not answered before, Wim. That is correct. The CompatibilityView is not in the inheritance path of ViewPart, so there should be no problem. When creating the patch, I also verified that CompatibilityView.persistState extending CompatibilityPart.persistState, both annotated with @PersistState, is also no problem. Only CompatibilityView.persistState is automatically called by the DI framework on a CompatibilityView; the super, CompatibilityPart.persistState, is not called automatically. When called, CompatibilityView.persistState explicitly invokes super.
(In reply to Wim Jongman from comment #44) > I have played out these scenarios with the same result. I guess it is too > late to get this in M2? If you're OK with the fix you can merge it. I will review the Git change log since the Wed build and then decide whether we use the latest I-build for M2.
Gerrit change https://git.eclipse.org/r/135377 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4df2870b3c78ab8d4e70bec0e3894feaa966f71b
(In reply to Dani Megert from comment #46) > I will review the Git change log > since the Wed build and then decide whether we use the latest I-build for M2. That would be nice. Maybe cherry-pick it if there is too much other noise?
It's your honor to close as Resolved/Fixed Vladimir.
(In reply to Wim Jongman from comment #49) > It's your honor to close as Resolved/Fixed Vladimir. Yes, thanks!
That was a long road. Now back to my smaller pet project, https://www.eclipse.org/handly (a shameless plug). Thanks all.
(In reply to Vladimir Piskarev from comment #51) > That was a long road. Now back to my smaller pet project, > https://www.eclipse.org/handly (a shameless plug). Thanks all. It was a great ride. Thank you, Vladimir. You're more than welcome to stick around.
(In reply to Wim Jongman from comment #48) > (In reply to Dani Megert from comment #46) > > > > I will review the Git change log > > since the Wed build and then decide whether we use the latest I-build for M2. > > That would be nice. There were too many changes. We submitted the build we tested. > Maybe cherry-pick it if there is too much other noise? That would have been too much overhead for just on non-critical/blocker fix. You can try to update M2 from our I-build repo or patch your installation by replacing 'org.eclipse.ui.workbench' with one from our I-builds.
