Community
Participate
Working Groups
3.8 / 4.2. Projects like JDT or Mylyn cannot benefit from the IDE.openEditors(...) API because they also need to set state (especially the selection) when opening their editors. We need a new API that allows this in order to get improved performance when opening multiple editors at once.
Dani, I can't understand the difference between what we currently get using 'openEditors' and a restart. In both cases we end up with only one editor rendered and no restored state for the others. What am I missing ?
Eric, a way to pass in an IMemento through openEditors(*) would be one way to address this, although I'm not commenting if it's the best or the worst way, but it would use consistent infrastructure with what we have today. In the restart case, we save an IMemento the editor produced until the next time that editor is instantiated ... then they extract their info from their IMemento. PW
Just to note - in e4 we provide a way to store information not only on shutdown but on unrender via @PersitState maybe that's something we should provide to people instead of allow them to pass the memento! But maybe i got the useage of IMemento here completely wrong.
Tom, hopefully that's @PersistState not @PersitState...;-) The issue here seems not to really be one of managing specialized state in e4 (here the implementation could simply store any state directly in the MPart's 'persistentData' map) but rather on interacting with the legacy IMemento mechanism. I'm still unclear on why it's considered necessary in the multi-open case but not on a restart. I'll ask the question this way: If a user has never seen a file why is the editor's state necessary ?
Right it is @PersistState - but i think the problem for Mylyn/JDT is that it need to reopen editors who've been opened before and restore their state while you are in the same IDE session. The problem in 3.x is that if the editor is closed the memento is not available on restore in the same session because it is only synced back on shutdown, hence on restart you get the correct memento but on reopen you don't - maybe I'm completely off track but that's how understand the situation.
Tom, thanks for the clarification...I'm sure you've hit on the actual issue ! OK, it *is* different, but a quick test shows that within a given session a JDT editor doesn't maintain any state (i.e. always opens with the cursor at the start of the file and no selection...). On a restart the editor correctly shows whatever selection was there on shutdown. Seems to me that this is really a different defect though. As it stands the only editors that have persisted state are those that were open when the session last ended, everything else always comes up with no state. Note that even selecting an editor and closing / re-opening it loses its state...no changes required. I think that this is what they (correctly) want but don't see why this is tied together with the 'openEditors' API.
I'm guessing that Mylyn actually uses some local memento to get this right but I really can't be sure...does it do this right now ? What happens if I have a few editors open with various selections in each and switch tasks to one with no editors and switch back within one session ?
(In reply to comment #1) > Dani, I can't understand the difference between what we currently get using > 'openEditors' and a restart. In both cases we end up with only one editor > rendered and no restored state for the others. > > What am I missing ? No idea, that is *not* what I see. When I use our latest 4.3 M2 candidate (I20120919-0330) and do this: 1. Use 'Open Type' to open two types, let's say 'A' and 'B' 2. restart ==> one of the editors is active and the type is selected 3. click the tab of the other editor ==> editor is activated and the type is selected ==> state *is* restored. I have no clue how you get to the conclusion that you posted in comment 1. (In reply to comment #6) > Tom, thanks for the clarification...I'm sure you've hit on the actual issue ! > > OK, it *is* different, but a quick test shows that within a given session a > JDT editor doesn't maintain any state (i.e. always opens with the cursor at > the start of the file and no selection...). Correct, the editor does not store state during a session but when you open a Java element (via 'Open Type', via 'Package Explorer' or via 'Project Explorer') that element is selected in the Java editor. Only if you open the *file* the caret is at position 0. So, if you select 3 types and use the new API then we need to have a way to store that selection, so that it can be applied later when that editor gets activated. Also, that state/selection must be preserved for not yet activated editors, so that it gets preserved when restarting!
(In reply to comment #2) > Eric, a way to pass in an IMemento through openEditors(*) would be one way > to address this, although I'm not commenting if it's the best or the worst > way, but it would use consistent infrastructure with what we have today. > > > In the restart case, we save an IMemento the editor produced until the next > time that editor is instantiated ... then they extract their info from their > IMemento. > The memento approach would be the first thing I'd give a try. Currently, the Java editor stores there exactly that state which we would need.
Dani, wouldn't we also have to add the memento parameter to all the single openEditor API's ? I'm all for having the editor's state be persistable within a session, I'm quite surprised that we did the work to save it across sessions before this functionality was there... The mechanics are fairly clear; if a memento is supplied then we 'apply' it before rendering the editor and persist into it when it closes. If no memento is supplied we get the current situation. Note that in this case it's up to the owner of the mementos to persist them across sessions. The implication is that the code calling any 'openEditor(s)' variant has to arbitrate with some common source to get the correct memento to use for a given input. How do Open Type and Open Resource arrange to use the same momento ? I'm hoping that I can get in touch with Steffen Pingle so I can figure out what their technique was in 3.x. Perhaps we'll get lucky and just make that API...;-).
There *may* be an e4 way to do this. The only reason we lose the state now is that we actually remove the MPart representing the editor from the model when it's closed. The equivalent of passing in the memento in the scenario above would be to simply leave closed editors in the model (perhaps moving them to the 'sharedElements' list) when they are closed. The downside is that we over time 'pollute' the model with potentially many 'saved' editors that the user will never open again (likely why we didn't do this already).
(In reply to comment #10) > Dani, wouldn't we also have to add the memento parameter to all the single > openEditor API's ? Not really, because there the editor is created and hence one can set the state e.g. set the selection. This is what currently happens when opening the Java editor on a Java element. > I'm all for having the editor's state be persistable within a session, I'm > quite surprised that we did the work to save it across sessions before this > functionality was there... To repeat what I said in comment 8: this is not about storing state within a session, but about allowing to pass state which can then be used when the editor gets created. Nothing needs to be stored when the editor gets closed. > The implication is that the code calling any 'openEditor(s)' variant has to > arbitrate with some common source to get the correct memento to use for a > given input. How do Open Type and Open Resource arrange to use the same > momento ? They would definitely not use the same memento. Actually, 'Open Resource' does not need that at all since it does not set any state on the opened editor(s).
Got it...each pathway to opening multiple editors can choose which (if any) momentos apply. So would a new API like this be OK ? public IEditorReference[] openEditors(IEditorInput[] inputs, String[] editorIDs, IMemento[] mementos, int matchFlags) With javadoc that says: momentos This array contains the momentos to use when opening the editor. It is valid to have null entries in this array to indicate that there is no state to restore. I'd likely promote the current 'openEditor' that has an IMemento parameter to API at the same time...
(In reply to comment #13) > Got it...each pathway to opening multiple editors can choose which (if any) > momentos apply. So would a new API like this be OK ? > > public IEditorReference[] openEditors(IEditorInput[] inputs, String[] > editorIDs, IMemento[] mementos, int matchFlags) What are the 'matchFlags'? > > With javadoc that says: > > momentos This array contains the momentos to use when opening the editor. It > is valid to have null entries in this array to indicate that there is no > state to restore. It's not about "restore", but setting an initial state. Otherwise it goes into the right direction. Before putting it in, we should confirm with Mylyn whether that also helps them. > I'd likely promote the current 'openEditor' that has an IMemento parameter > to API at the same time... Why would you add it? I'm reluctant of adding APIs which have no client.
(In reply to comment #14) > > What are the 'matchFlags'? It's a modifier that determines whether (and how) we should find existing open editors (at least that's what I get from the javadoc...) > It's not about "restore", but setting an initial state. Otherwise it goes > into the right direction. Before putting it in, we should confirm with Mylyn > whether that also helps them. +1 it should just say that the momento is used to set the editor's initial state. I'm getting in touch with Steffen today to discuss this... > > I'd likely promote the current 'openEditor' that has an IMemento parameter > > to API at the same time... > > Why would you add it? I'm reluctant of adding APIs which have no client. Just for completeness (and because it was already there). Actually someone smart could just use the new API with only one editor if they want to pass in a momento.
(In reply to comment #15) > (In reply to comment #14) > > > > What are the 'matchFlags'? > > It's a modifier that determines whether (and how) we should find existing > open editors (at least that's what I get from the javadoc...) +1. > > > I'd likely promote the current 'openEditor' that has an IMemento parameter > > > to API at the same time... > > > > Why would you add it? I'm reluctant of adding APIs which have no client. > > Just for completeness (and because it was already there). Actually someone > smart could just use the new API with only one editor if they want to pass > in a momento. I'd still -1 that addition. It's simply not needed.
I'm ok with that, folks can always down cast to WorkbenchPage if they need it (perhaps this is what Mylyn does?). The other question I'm not sure of is managing the IMemento's... 1) Should we update the provided momento when the editor closes ? +1 for me 2) If the momento slot for a given editor is null when it's opened (i.e. it's never been opened before) how do you want to get the initial momento when it closes ? Steffen may be able to fill in how Mylyn currently handles this...
Dani and Eric, thanks for driving the discussion! (In reply to comment #17) > I'm ok with that, folks can always down cast to WorkbenchPage if they need it > (perhaps this is what Mylyn does?). > > The other question I'm not sure of is managing the IMemento's... > > 1) Should we update the provided momento when the editor closes ? +1 for me For Mylyn it's common that users start working without having a task active, then activate a "new" task (that does not have persisted state, yet) and want to save the state of the open editors on task deactivation. In that scenario Mylyn does not control opening of the editors so that wouldn't help. > 2) If the momento slot for a given editor is null when it's opened (i.e. it's > never been opened before) how do you want to get the initial momento when it > closes ? Yes, for Mylyn we would actually also need an API to retrieve the mementos for all open editors. The current way of doing is by invoking persist() on editor references returned by WorkbenchPage.getInternalEditorReferences() and then grabbing the memento from the references. In Eclipse 3.x that was done through invoking WorkbenchPage.saveState().
(In reply to comment #17) > I'm ok with that, folks can always down cast to WorkbenchPage if they need > it (perhaps this is what Mylyn does?). > > The other question I'm not sure of is managing the IMemento's... > > 1) Should we update the provided momento when the editor closes ? +1 for me > > 2) If the momento slot for a given editor is null when it's opened (i.e. > it's never been opened before) how do you want to get the initial momento > when it closes ? OK, I have to repeat this a third time ;-). This is not about storing state inside the same session. If this is desired then we can deal with this later in another bug. This bug here is purely about providing a new API to open editors and allow to inject state when the editor actually gets created (in this or a later session). On close *nothing* must happen - like it does now. A remembered memento must be thrown away at this point. Only when the *workbench* is closed the editor is asked to persist its state via 'IPersistable.saveState(IMemento)'. Of course for those editors that got opened via the new API, but which did not yet get created, the memento passed via the new API will be used here. > Yes, for Mylyn we would actually also need an API to retrieve the mementos for > all open editors. I assume the key (new part) here is to get the memento for opened but not yet activated/created editors. > mementos This array contains the mementos to use when opening the editor. It > is valid to have null entries in this array to indicate that there is no state > to restore. We should also allow 'null' for the array because in most cases one will either pass state to all editors or pass no state to all (e.g. 'Open Type' vs. 'Open Resource'). When creating the editor the same call sequence should be used as when restarting the workbench: 1. constructor 2. IEditorPart.init(IEditorSite, IEditorInput) 3. IPersistableEditor.restoreState(IMemento) 4. IWorkbenchPart.createPartControl(Composite)
OK, it's getting clearer... 1) The current persistence management will stay in place exactly as is for storing cross-session state. 2) The momentos for the new openEditors API will apply, possibly over-riding any state saved across sessions. 3) The 'mementos' array may be null (which it will be if the existing api is called). 3) We will need another new API to support Mylyn...also a lot more efficient that calling WorkbenchPage#saveState...;-). IMomento[] getPersistedState(IEditorReference[] editorRefs); editorRefs must be non-null; to get all editors call it passing in 'getEditorReferences()' as the argument. Comments ?? I'll be working on this for the next few days in order to get something out that both of you can test with and provide feedback. Initially I'll do the 4.x implementation and once we're OK with it I'll make it work in 3.8 as well.
(In reply to comment #20) > OK, it's getting clearer... > > 1) The current persistence management will stay in place exactly as is for > storing cross-session state. Yep. > 2) The momentos for the new openEditors API will apply, possibly over-riding > any state saved across sessions. Applying both makes no sense. We should simply throw away the session state in this case. A more sophisticated solution would try to merge them but I think we can skip that for now and look at it in case we find a problem with the simpler solution. > Comments ?? I'll be working on this for the next few days in order to get > something out that both of you can test with and provide feedback. Bug 178229 has a patch that allows to test the (old) API via 'Open Resource'. You can use that, tweak it to call the new API with some state (e.g. selection). To see how to create the selection in the memento see AbstractTextEditor.saveState(IMemento). > Initially > I'll do the 4.x implementation and once we're OK with it I'll make it work > in 3.8 as well. Wouldn't it be easier to target 3.8.2? I would expect that chances are higher that you can just use that code than the other way around. Also note that we still have blocking bug 296477 on the old API. The new API should take care of this.
(In reply to comment #21) > Applying both makes no sense. We should simply throw away the session state > in this case. A more sophisticated solution would try to merge them but I > think we can skip that for now and look at it in case we find a problem with > the simpler solution. I just mean that if the editor isn't already open that the next time it does it'll have the state as supplied in the call, no merging... > Bug 178229 has a patch that allows to test the (old) API via 'Open > Resource'. You can use that, tweak it to call the new API with some state > (e.g. selection). To see how to create the selection in the memento see > AbstractTextEditor.saveState(IMemento). +1 I did that anyways...;-) Do you think I should commit this change against this defect or open a new one ? > Wouldn't it be easier to target 3.8.2? I would expect that chances are > higher that you can just use that code than the other way around. Too late, I'm about to commit the 4.2.2 code... > Also note that we still have blocking bug 296477 on the old API. The new API > should take care of this. Could you check ? At the end of the 'openEditors' call I call 'activate' on the part which *should* make it the same as the other opens.
Created attachment 222195 [details] Here's a view that I use to test with... Create a new plug-in project using the 'With a view' template and replace the view's code with this one. It uses the new API's to mimic a task switch in Mylyn (as near as I can guess...;-). The 'Snap!' button will ask all editors to report their current state and creates a snapshot of the parameters it needs to call 'openEditors'. The 'name' will be whatever is in the edit control. Double-clicking on an entry will call the api using the snapshot and applying the current states of the 'match' fields.
http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=03c9fa277d316e9f063f51d1dc3167acfc3c88e9 This implements the initial version for the two new APIs: openEditors (with IMementos) getEditorState Kick this around and let me know if there's anything that need tweaking. Also feedback on the javadoc would be appreciated.
(In reply to comment #22) > (In reply to comment #21) > > Applying both makes no sense. We should simply throw away the session state > > in this case. A more sophisticated solution would try to merge them but I > > think we can skip that for now and look at it in case we find a problem with > > the simpler solution. > > I just mean that if the editor isn't already open that the next time it does > it'll have the state as supplied in the call, no merging... Right, does is exactly what I proposed, no? > > Bug 178229 has a patch that allows to test the (old) API via 'Open > > Resource'. You can use that, tweak it to call the new API with some state > > (e.g. selection). To see how to create the selection in the memento see > > AbstractTextEditor.saveState(IMemento). > > +1 I did that anyways...;-) Do you think I should commit this change against > this defect or open a new one ? I think we already have a bug for that. > > > Also note that we still have blocking bug 296477 on the old API. The new API > > should take care of this. > > Could you check ? At the end of the 'openEditors' call I call 'activate' on > the part which *should* make it the same as the other opens. Yes, I will but the steps in bug 296477 would be easy enough to verify for you too ;-).
More to the point...once the next I-build comes out could both you and Steffen take the time to use it in whatever way you intend to to make sure it's sufficient ? Report back any issues here...
Thanks, Eric! I'll take a look.
I've fixed the @since tags in master since the current ones cause API errors. It looks like this got introduced when the bundle version was changed. Please make sure that you have API Tools correctly setup to detect such things before pushing any changes. Thanks.
(In reply to comment #26) > More to the point...once the next I-build comes out could both you and > Steffen take the time to use it in whatever way you intend to to make sure > it's sufficient ? Report back any issues here... I only quickly tested the 'Open Resource' case (see patch in bug 178229) and one can easily see that the activated editor is the left-most editor instead of the right-most one.
(In reply to comment #26) > More to the point...once the next I-build comes out could both you and > Steffen take the time to use it in whatever way you intend to to make sure > it's sufficient ? Report back any issues here... 1) The code cannot work as it only applies the memento on not yet activated editors (via EditorReference.initialize(IWorkbenchPart). 2) By only reading the API it is impossible to create a memento that will work. One has to read the code and use the internal constant 'IWorkbenchConstants.TAG_EDITOR_STATE' or directly use its string. The API should accept a simple memento with attributes and then wrap it with whatever is internally expected. 3) The thing is called "memento(s)" not "momento(s)" 4) The mementos array mat be null but mat == might?
Besides the already reported problems I implemented an initial fix for 'Open Type' (see attachment in bug 178231). This reveals that the selection is not correctly restored due to bug 375576 and/or bug 296477. Those have to be fixed before we can make progress on this one.
http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R3_8_maintenance&id=4c06ff9bd60a676b15c29c90e3afceefa95d77b2 This is an initial commit on this for 3.8. It brings the API into alignment so that folks who use the new API in 4.x won't break a 3.x build but it's implementation does *not yet work*. It handles everything correctly (i.e does not regress the current API) but the saved state doesn't seem to be re-injected correctly when the file is opened using the new openEditors API. Now I'll check 4.2.2 against Dani's comments...
Dani, the '104' Api version isn't a mistake. Paul and John discussed the options and decided it was easier to maintain the version and add an API filter for it later...
Thanks Dani, I'll be committing a small revision to include the spelling errors. Agreed that when you switch from the old OpenResourceHandler code to the new one there is a change in which editor is activated once it's done; old code -> Last one...new code -> First one. This matches the javadoc for 'openEditors' which states that the first one is the one that gets activated. Since the old code activated all of them the last one was 'winning'. Perhaps we could extend the *new* API with an argument indicating which of the resulting editors should be activated ? I've never considered that the IMemento could be generated. I was expecting that clients would use the new 'getEditorState' API to retrieve the state at some particular time and then re-use that memento in a subsequent call to openEditors. I don't really see how anybody except the author of the editor code could possibly know what should be in the memento (or should know for that matter since that would make the format of the memento API for that editor type). I'm not quite sure I understand the last point about selection not being reset correctly. You may need to pick up a later build, see: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=358512f39dc0be2f7af3ed06229c9aa4c9db5074 This is where I've move the firing of the partActivated event until *after* we set the focus...
http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=83ba7ccb0cf51d436e219e92d286b525e1219537 This fixes the 'mat' and 'momento' typos...
(In reply to comment #33) > Dani, the '104' Api version isn't a mistake. Paul and John discussed the > options and decided it was easier to maintain the version and add an API > filter for it later... Why not add the API filter right away? This would have saved me an error in my workspace and time to deal with it ;-). Anyway, we have to adjust the @since tags now that the API is also in 3.8.2 and hence it must be @since 3.8.2 everywhere. I prefer 3.8.2 over 3.9.0 because it makes it easier to detect the bundle (and API) as being modified in the maintenance stream. After a long discussions we also did that for our Java 7 support last year where we added the new stuff into 3.7.1. If you disagree with the chosen version we can discuss it in a separate bug. I've changed that now everywhere and also updated the bundle version in R3_8_maintenance (was still the one for 3.8.1).
(In reply to comment #35) > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?h=R4_2_maintenance&id=83ba7ccb0cf51d436e219e92d286b525e1219537 > > This fixes the 'mat' and 'momento' typos... Please also fix this in the other branches.
(In reply to comment #34) > Agreed that when you switch from the old OpenResourceHandler code to the new > one there is a change in which editor is activated once it's done; old code > -> Last one...new code -> First one. > > This matches the javadoc for 'openEditors' which states that the first one > is the one that gets activated. Since the old code activated all of them the > last one was 'winning'. Perhaps we could extend the *new* API with an > argument indicating which of the resulting editors should be activated ? We must fix the Javadoc and the implementation then and define the order in which the not yet present editors (tabs) are placed in relation to the order in the array and which one gets activated. To be concrete, I expect this: - Editors/tabs that do not yet exist are placed from left to right as they appear in the array - The last editor in the array is activated. Activating the left-most would be a reason not to use the new API. > I've never considered that the IMemento could be generated. Now you lost me. The API wants an IMemento with which clients are expected to pass arguments. How should a client use it without generating a memento? > I was expecting > that clients would use the new 'getEditorState' API to retrieve the state. Editors which implenent , already restore its state via restoreState(...). We should not force them to add more code. Using the new openEditors(...) API should simply work for them out of the box and it actually does like the 'Open Type' patch from bug 178231 proofs - except for the focus issue. > I'm not quite sure I understand the last point about selection not being > reset correctly. You could read bug 375576 and use a test case listed there or in one of the many duplicates, but I will provide you with detailed steps here later. > You may need to pick up a later build, see: > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=358512f39dc0be2f7af3ed06229c9aa4c9db5074 No, I worked off 'master', but it looks like some people only make (performance) fixes in the maintenance stream, so maybe 'master' was not containing the latest. I will retest again on 'master' and 'R4_2_maintenance' and report here with concrete steps to reproduce the problem.
(In reply to comment #34) > I'm not quite sure I understand the last point about selection not being > reset correctly. You may need to pick up a later build, see: 1. have 'master' or 'R4_2_maintenance' ui.workbench imported 2. have jdt.ui imported 3. apply the patch from bug 178231 to jdt.ui 4. start a new target workspace and perform the next steps in there 5. paste this into 'Package Explorer': "class ArrayListX {}" 6. use 'Open Type' and select 'ArrayList' and 'ArrayListX', click 'OK' 7. click on the 'ArrayList' tab ==> scroll bars are wrongly scrolled, e.g. there should be no horizontal scrolling and the selection should not be at the top. You can verify the correct behavior by doing the same in a fresh 3.8.1 or 4.2.1 workspace.
(In reply to comment #36) > Java 7 support last year where we added the new stuff into 3.7.1. If you > disagree with the chosen version we can discuss it in a separate bug. I've opened bug 392317 to discuss. PW
(In reply to comment #38) > > No, I worked off 'master', but it looks like some people only make > (performance) fixes in the maintenance stream, so maybe 'master' was not > containing the latest. I will retest again on 'master' and > 'R4_2_maintenance' and report here with concrete steps to reproduce the > problem. I merge 4.2.2 regularly into master before each week's I build (4.2.2 to 4.3 is true up-version as opposed to a complete split stream like 3.8.2/4.2.2). We only have to cherry-pick changes between 4.2.2 and 3.8.2. I can merge more frequently if you want. PW
(In reply to comment #41) > (In reply to comment #38) > > > > No, I worked off 'master', but it looks like some people only make > > (performance) fixes in the maintenance stream, so maybe 'master' was not > > containing the latest. I will retest again on 'master' and > > 'R4_2_maintenance' and report here with concrete steps to reproduce the > > problem. > > I merge 4.2.2 regularly into master before each week's I build (4.2.2 to 4.3 > is true up-version as opposed to a complete split stream like 3.8.2/4.2.2). > We only have to cherry-pick changes between 4.2.2 and 3.8.2. > > I can merge more frequently if you want. > > PW I expect master to be the latest, so actually I'd prefer that we work in there and then decide which fixes to backport.
Some minor detail: there's a System.out.println in the new code.
Dani, I have a suggestion for the problem with you creating your own memento but I'm not sure if it's right. Right now I'm constructing the memento using the complete storage memento as we use when closing down. This includes both the "input" and "editorState" children. My understanding is that since we're being supplied with the IEditorInput we shouldn't need the "input" part. I can easily modify the code to only expect the "editorState" information in the IMemento and use that to call the 'restoreState' directly with the info. I've looked quickly at the 'doRestoreState' and this seems like it would be enough...is it ?
(In reply to comment #44) > Dani, I have a suggestion for the problem with you creating your own memento > but I'm not sure if it's right. Right now I'm constructing the memento using > the complete storage memento as we use when closing down. This includes both > the "input" and "editorState" children. > > My understanding is that since we're being supplied with the IEditorInput we > shouldn't need the "input" part. I can easily modify the code to only expect > the "editorState" information in the IMemento and use that to call the > 'restoreState' directly with the info. I've looked quickly at the > 'doRestoreState' and this seems like it would be enough...is it ? Yes, I only need to set the 'editorState' to make it work: I would like to create and pass the following memento: XMLMemento memento= XMLMemento.createWriteRoot("selection"); memento.putInteger("selectionOffset", nameRange.getOffset()); memento.putInteger("selectionLength", nameRange.getLength()); Currently, I have to wrap it as follows: /* This should go into the framework */ XMLMemento root= XMLMemento.createWriteRoot("root"); IMemento child= root.createChild("editorState"); child.putMemento(memento);
Dani, rather than changing the existing API for 'openEditors' we've decided to go with adding a new parameter to the new API in order to enforce which (if any) of the editors that are opening get activated: int activateIndex - The index of the IEditorInput to be activated once the editors have been opened. This must be within the range of the supplied 'inputs' array or -1 to indicate that no activation should occur. That way I can just make the current API forward to this one with mementos == null and activateIndex == 0 no changes required.
(In reply to comment #46) > Dani, rather than changing the existing API for 'openEditors' we've decided > to go with adding a new parameter to the new API in order to enforce which > (if any) of the editors that are opening get activated: > > int activateIndex - The index of the IEditorInput to be activated once the > editors have been opened. This must be within the range of the supplied > 'inputs' array or -1 to indicate that no activation should occur. > > That way I can just make the current API forward to this one with mementos > == null and activateIndex == 0 no changes required. I don't see the point here. There is no existing API, we newly add this in 3.8.2/4.2.2, so we can define it at will. Adding yet another parameter is just a waste.
(In reply to comment #47) > (In reply to comment #46) > > Dani, rather than changing the existing API for 'openEditors' we've decided > > to go with adding a new parameter to the new API in order to enforce which > > (if any) of the editors that are opening get activated: > > > > int activateIndex - The index of the IEditorInput to be activated once the > > editors have been opened. This must be within the range of the supplied > > 'inputs' array or -1 to indicate that no activation should occur. > > > > That way I can just make the current API forward to this one with mementos > > == null and activateIndex == 0 no changes required. > > I don't see the point here. There is no existing API, we newly add this in > 3.8.2/4.2.2, so we can define it at will. Adding yet another parameter is > just a waste. I suggest you wait for the feedback from Mylyn whether they really need this flexibility. In case we really add an additional parameter, then it must not only allow to set the active editor but allow clients to specify the complete activation order. So, the current array would specify the order of the tabs in the stack and the second array would specify in which order they get activated/traversed.
Hi Eric, Sorry for the slow reply on this. I just grabbed the last integration build and gave the new API a quick try. It looks like it will work well for us, but I do wonder how we should be getting the editor input and id. Since we save the entire state of the editor on task deactivation, we have the editor input and editor id portions of the memento and previously we have passed this to the workbench to deal with. With this new API, we will need a way to consistently get the editor id and add some code to restore the editor input. Is there some API that we could use to get this information easily or could the API that you are adding handle null inputs and editor ids and attempt to restore it all directly from the memento?
(In reply to comment #49) > Hi Eric, > > Sorry for the slow reply on this. I just grabbed the last integration build > and gave the new API a quick try. It looks like it will work well for us, > but I do wonder how we should be getting the editor input and id. Since we > save the entire state of the editor on task deactivation, we have the editor > input and editor id portions of the memento and previously we have passed > this to the workbench to deal with. With this new API, we will need a way > to consistently get the editor id and add some code to restore the editor > input. Is there some API that we could use to get this information easily > or could the API that you are adding handle null inputs and editor ids and > attempt to restore it all directly from the memento? Shawn, no problem. I'm not sure how mylyn handles this now...The view I supply uses the page.getEditorReferences() then calls 'getEditorInput' on each but I realize that you have to persist these across session restarts. Note that as currently implemented the memento I return through the API includes both the 'input' and 'editorState' children so it (in theory) shouldn't be too difficult for us to re-create the input if needed (i.e. your last suggestion should be do-able I think (it's how *we* persist / re-create the inputs across sessions). Dani, no to worry. I can determine whether the supplied memento is the composite one (with both the serialized input and editor state) or only the editorState contents and take the appropriate action in both cases. This means that I should also be able for you to just pass me the mementos you construct and have them passed directly on to the editor itself. The only time that the 'composite' memento *must* be defined is when you want the API itself to reconstruct the input and editorId from the memento.
It looks like getEditorState() saves the entire state including the input which is what we need. In order to support restore we would need an API that didn't require editor inputs as Shawn pointed out in comment 49 but worked based on the mementos that were returned by getEditorState(), e.g. something like this: public IEditorReference[] openEditors(IMemento[] mementos) It seems that the trickiest part is to match open editors without an input. We implemented a naive approach that uses the "path" attribute of the editor memento and compares it against IPathEditorInput.getPath() of existing references but it doesn't work reliably (bug 389122). On 3.x matching of existing editors in Mylyn works based on the "partName" attribute which works better even though there are some edge cases that fail as well. We couldn't use that on e4 since it wasn't part of the memento but if getEditorState() included that we could use the same matching strategy as on 3.x even though mementos still wouldn't be portable due to other attributes missing (bug 391564) . Eric, I have o.e.ui.workbench checked out if you want me to try anything out.
I pushed a quick crude hack here as a poc to demonstrate what I was after: https://github.com/spingel/eclipse.platform.ui/commit/eca0d0cb5725b92023be79bc96e9037fe2d47c59 . Essentially this is re-implementing EditorManager.restoreState(IMemento memento) in the 4.x stream which is what Mylyn invokes on 3.x.
Steffen, thanks for the input(s), pun intended...;-). I'll take a look at the patch tomorrow, we're testing M3 today. I'll also take a look at the other defects you mention to see if I can determine a safe(r) mechanism. Note: It's possible that we can have a complete fix for this on 4.x but the 3.x version may have to live with one or the other of the existing defects (i.e. it won't be a regression in 3.x but may not be as good as 4.x).
(In reply to comment #53) > Note: It's possible that we can have a complete fix for this on 4.x but the > 3.x version may have to live with one or the other of the existing defects > (i.e. it won't be a regression in 3.x but may not be as good as 4.x). For 3.8.x I could also live with the API being there and simply opening all the editors one by one like 'Open Resource' and 'Open Type' currently do. This would mean no performance gain in 3.8.x which I'd consider acceptable.
http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=b2345531c8f4a1a8848c6ea284b4e11c567ae73d OK, this looke (to me) to work fairly well. As well as changing the code for the new APIs I also had to correct some of the existing code: MultiPartInitException: This code was clearly broken since it would throw an exception on *any* array containing more than one exception. getEditorReferences was returning the editors in an arbitrary order, I switched to having it return the editor references in their visual order (i.e. as they appear on the tabs in the folder). This makes it appropriate for Steffen's use I think. Dani: you should now simply be able to construct the editor state memento, no need to have "editorState" as its ID, the code doesn't care. You will have to change your current implementation of 'restoreState' though to account for changing the state of an already opened editor. Steffen: You should simply be able to call 'getEditorState' and passing in the result of a call to 'getEditorReferences' and setting 'includeInputs' to true. Then you can call 'openEditors' without having either the 'inputs' or 'editorIDs' arrays defined. Watch out that there may be editors (i.e. compare editors...) that cannot be restored. On the first call the mementos that had issues will have non-null entries in the MultiPartInitException that will be thrown after the processing is complete. I'll attach my newer version of the 'SampleView' so you can see the code in action...
Created attachment 223195 [details] New version that tests the new API
(In reply to comment #56) > Created attachment 223195 [details] > New version that tests the new API This still does not work. You either have to change the activation order as I requested in previous comments or you have to introduce an activation array as mentioned in comment 48. With your new activation parameter I can make the last one being activated i.e. A B [C] BUT: when I close C it activates A instead of B, which is wrong. To repeat: the simplest thing would be to just fix the order and add the additional parameter only when needed/requested by Mylyn. > Dani: you should now simply be able to construct the editor state memento, no > need to have "editorState" as its ID, the code doesn't care. This does not work with the code from 'master' and I can't see any code in #openEditors that would do the wrapping. > You will have to change your current implementation of 'restoreState' though > to account for changing the state of an already opened editor. Yes I know and it this works (constructing the memento with the wrapper).
I'm seeing output on stderr when using the new API: OE: b.txt OE: a.txt OE: b.txt OE: a.txt Not a big deal at all but it would be nice to remove that before the Juno SR2 goes out.
Done ! Thanks Steffen http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=9f5ddb53438dae8eddb2ccb169f54f4810737fe5
Dani, just a note that I've looked into maintaining the history but ran into internal issues; we're storing the history in two different ways, once in the WorkbenchPage and again in the PartService. Before fixing this I want to see if I can make things simpler by changing the page code to use the e4 history. This will (should) prevent us from getting out of synch in the future.
Eric, the wrong history could be the reason for #397579
Eric, can this be moved to 4.3 now? PW
As far as I'm concerned...Steffen, you're the main consumer...are you OK with the current behavior until 4.3 goes out ?
(In reply to comment #63) > As far as I'm concerned...Steffen, you're the main consumer...are you OK with > the current behavior until 4.3 goes out ? Yes. Thanks very much!
Moving to 4.4 to see if we can clean it up enough to replace some other multi open editor cases.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. If the bug is still relevant, please remove the stalebug whiteboard tag.
