Bugzilla – Full Text Bug Listing
| Summary: | API proposal: IWorkbenchPartReference.persist() | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Vladimir Piskarev <pisv> |
| Component: | UI | Assignee: | Vladimir Piskarev <pisv> |
| Status: | CLOSED WONTFIX | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | daniel_megert, Lars.Vogel, loskutov, wim.jongman |
| Version: | 4.10 | ||
| Target Milestone: | 4.11 M3 | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: |
https://bugs.eclipse.org/bugs/show_bug.cgi?id=30042 https://bugs.eclipse.org/bugs/show_bug.cgi?id=307319 https://bugs.eclipse.org/bugs/show_bug.cgi?id=541438 https://bugs.eclipse.org/bugs/show_bug.cgi?id=543609 |
||
| Whiteboard: | |||
|
Description
Vladimir Piskarev
(In reply to Vladimir Piskarev from comment #0) > For example, a view might register an IPartListener2 so that when the view > has been closed or deactivated it could save the view settings by calling > IWorkbenchPartReference.persist(). Why? (In reply to Dani Megert from comment #1) > (In reply to Vladimir Piskarev from comment #0) > > For example, a view might register an IPartListener2 so that when the view > > has been closed or deactivated it could save the view settings by calling > > IWorkbenchPartReference.persist(). > > Why? To make sure that the view state is saved in saveState(IMemento) method when the view gets closed and then restore the saved state in init(IViewSite, IMemento) method on reopening. Otherwise, the view being reopened might receive a stale memento (if saveState had been called before the view was closed but the view state has changed after the call) or no memento (if saveState had not been called). Or am I missing something? Is init(IViewSite site, IMemento memento) called when you manually open a view? If so then saveState(IMemento memento) should also be called when it is closed manually. My understanding (I could be wrong) is that the methods with IMemento are only called when the workbench exits and starts. (In reply to Wim Jongman from comment #3) Debugging shows that: > Is init(IViewSite site, IMemento memento) called when you manually open a > view? Yes, with the previously saved memento (if any). > If so then saveState(IMemento memento) should also be called when it > is closed manually. No, saveState(IMemento) is not called "automatically" by the workbench in this case. > My understanding (I could be wrong) is that the methods with IMemento are > only called when the workbench exits and starts. saveState(IMemento) is called when the workbench exits and also periodically from the workbench's Auto-Save Job. init(IViewSite, IMemento) is called each time a view is being initialized (if there is no previously saved memento, null will be passed). (In reply to Vladimir Piskarev from comment #4) > (In reply to Wim Jongman from comment #3) > Debugging shows that: > > > Is init(IViewSite site, IMemento memento) called when you manually open a > > view? > > Yes, with the previously saved memento (if any). Then I think it would be reasonable to also call saveState when the view closes. (In reply to Wim Jongman from comment #5) > Then I think it would be reasonable to also call saveState when the view > closes. Yes, it would probably be ideal for solving that specific case, but would not it be a breaking change with regard to existing behavior? That's why this request to provide an API so that the view itself could initiate the saveState call when the view closes. Perhaps other clients might also be interested in such explicit management of saving a workbench part's state (though I'm not able to provide a specific example now). (For the record, if it would be possible to alter the existing behavior so that saveState is called automatically by the workbench when a view closes, it would solve the problem for me, personally.) (In reply to Vladimir Piskarev from comment #6) > (In reply to Wim Jongman from comment #5) > > Then I think it would be reasonable to also call saveState when the view > > closes. > > Yes, it would probably be ideal for solving that specific case, but would > not it be a breaking change with regard to existing behavior? I don't think so because we also introduced saving the workbench at specific intervals. The view must always be ready to save the state. > > That's why this request to provide an API so that the view itself could > initiate the saveState call when the view closes. Perhaps other clients Yes. I understand. That makes sense as well and is probably less invasive. Can you provide a patch? I will help you get this in if there are no objections. (In reply to Wim Jongman from comment #7) > I don't think so because we also introduced saving the workbench at specific > intervals. I agree. Not calling saveState(IMemento) "automatically" when the view closes looks like a bug, since it creates an asymmetry with init(IViewSite, IMemento) being called each time the view opens. Earlier, I discovered bug 30042 and bug 307319 that were both closed as WONTFIX, which made me head in a different rout and led to this request. Now I realize that those two had probably been resolved before saving the workbench at specific intervals was introduced, so there are now different forces at play. As you stated, the view must always be ready to save the state. I would be happy to try to provide a patch, but now have my doubts about whether this API request is premature and there is just a bug in view management that should be fixed. What do you think would be the best course of action? I believe we want something like a new API (and attribute in extension point) allowing to save on close for views, because we can't just change existing behavior. Have no IDE at hand right now to be more specific, but I was also very surprised why it was not done by default first time I've stumbled upon this. (In reply to Andrey Loskutov from comment #9) > I believe we want something like a new API (and attribute in extension > point) allowing to save on close for views, That sounds a bit heavy. I like promoting the idea of promoting the persist method better. We can also introduce a new property like in [1]. PROP_PERSIST on IWorkBenchPart that would call the persist method. [1] https://wiki.eclipse.org/FAQ_How_do_I_enable_the_Save_and_Revert_actions%3F > because we can't just change existing behavior If the method is already called during workbench save then I don't see objections to also call it at on view close. Was workbench save also not introduced later? Think about the use case where a user presses close on the view 1 second before the method would have been called by workbench save. What could be the harm? Alternatively, we would only call the savestate method on close when the workbench save interval is > 0. See also bug 541438 comment 5 and bug 30042 comment 7. I personally would just save on close, but we have to consider the entire eco system and legacy software existing. I can imagine someone does some weird things on save in the assumption it happens only on shutdown, so if we change default behavior we should at least give people some fail back possibility. (In reply to Andrey Loskutov from comment #11) > See also bug 541438 comment 5 and bug 30042 comment 7. > > I personally would just save on close, but we have to consider the entire > eco system and legacy software existing. I can imagine someone does some > weird things on save in the assumption it happens only on shutdown, so if we > change default behavior we should at least give people some fail back > possibility. But according to comment 4 it is already called at an interval. Right. So let save state on close if workspace auto-save is enabled. IIRC saving the workspace state can take significant time, I think we had once a user in which saving it took several minutes. (In reply to Lars Vogel from comment #14) > IIRC saving the workspace state can take significant time, I think we had > once a user in which saving it took several minutes. I'm not sure if this is the same thing. I also sometimes see a progress monitor when closing a very large workspace that can take a lot of time. But by then the GUI is already down so it could not be the savestate method called in the views. But in any case, calling the savestate method for every part is already done in the UI thread. It is a miracle that this has not yet caused any major issues. When it takes a lot of time, people might misinterpret the savestate intention. Maybe they are saving to disk or DB. We might want to consider writing error log for parts that misbehave in this method (e.g. take > 100ms) or even excluding them from additional workbench save calls (e.g. which take > 500ms). (In reply to Andrey Loskutov from comment #13) In the meantime, I have debugged this case and I can confirm see that savestate is called on workbench save intervals for every part. > Right. So let save state on close if workspace auto-save is enabled. +1 Vladimir, can you give this a try? (In reply to Wim Jongman from comment #16) > +1 Vladimir, can you give this a try? Sure, with pleasure. I should have some time next week to work on this, but could we first discuss a little bit what exactly I'm supposed to do about it? Will we introduce the new API as have been originally proposed, or just save state on view close when the workbench auto-save interval is > 0? If the latter, should I enter a new bug, so this one could be closed? (Personally, I would opt for the latter case, since introducing a new API has an inherent risk, and there seems to be no strong reason for that at this time.) Also, could you give me a hint about what would be the appropriate place to implement the new behavior of saving state on view close? I'm far from being a Platform UI expert :) (In reply to Vladimir Piskarev from comment #17) We are going to call the saveState method when a part is closed. I have been looking at E4 and the @PersistState annotation and interestingly this _is_ called when the part closes. > should I enter a new bug, so this one could be closed? Yes create two new bugs. One for calling savestate on part close and one for making persist API. We will then use this bug as the umbrella bug for these two. > Also, could you give me a hint about what would be the appropriate place You need to setup a dev enviornment first. Lars has a great tutorial about this [1]. It should take you 10 minutes. Then just create a part with a dispose method and put a breakpoint. You will see the chain of events. [1] http://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html (In reply to Wim Jongman from comment #18) OK, thanks for the great hints, Wim! > We are going to call the saveState method when a part is closed. But only if the workbench auto-save interval is > 0? (Just to clarify.) > I have been looking at E4 and the @PersistState annotation and interestingly > this _is_ called when the part closes. I have debugged the part destroy sequence as you suggested and it looks like CompatibilityPart.persistState() method (annotated with @PersistState) would be the right place to call WorkbenchPartReference.persist(). (In reply to Vladimir Piskarev from comment #19) > (In reply to Wim Jongman from comment #18) > > OK, thanks for the great hints, Wim! > > > We are going to call the saveState method when a part is closed. > > But only if the workbench auto-save interval is > 0? (Just to clarify.) > > > I have been looking at E4 and the @PersistState annotation and interestingly > > this _is_ called when the part closes. > > I have debugged the part destroy sequence as you suggested and it looks like > CompatibilityPart.persistState() method (annotated with @PersistState) would > be the right place to call WorkbenchPartReference.persist(). Yes, that sounds like music. (In reply to Vladimir Piskarev from comment #19) > But only if the workbench auto-save interval is > 0? (Just to clarify.) I changed my mind. Just call the method like it is done in E4 and secure the symmetry with restoreState. (In reply to Wim Jongman from comment #21) > Just call the method like it is done in E4 and secure the symmetry Thanks, Wim! I concur with you! Sorry for the delay in my activity on this bug; I caught the terrible flu, but I am starting to recover and hope to be able to work on this soon. Have a nice weekend! Thanks again! As far as I can see we can close this one as wontfix, because all what is needed was done via bug 543609? (In reply to Andrey Loskutov from comment #23) > As far as I can see we can close this one as wontfix, because all what is > needed was done via bug 543609? Yes, I think so. |