Community
Participate
Working Groups
In an Eclipse-based application, there are currently three "layers" of definition about the user interface. 1. The application's default presentation and setup This would be synonymous with the perspective defined in the factory or the e4 XMI file. 2. Contributed customizations from external plug-ins New plug-ins add views and leverage the perspectiveExtensions extension point. 3. The user's own bidding The user can move views around and show/hide them as they please. ---------- The merged...blob...of all of the above is the final result of what's presented to the user. At the moment, the 3.x story for this is not very good. Anyone that's ever used the perspectiveExtensions extension point has probably hit bug 48060 (such as yours truly). We will need to provide a _predictable_ story for this in e4. Whether it is "good" or "bad" is a different story but the exact behaviour should be documented and well defined. Since we use EMF at the moment for serializing our models, we will begin by testing the o.e.emf.ecore.change.util.ChangeRecorder class to see if it can do what we want by recording ("all") the deltas that has been caused (by user actions).
The ChangeRecorder (unsurprisingly) batches all the changes instead of recording every single detail. That is, if your MPart's name was originally "abc" and you changed it five bajillion times and finally settled on "def", only one change would ever be recorded and the change would simply record that the name was originally "abc", it does not seem to have information about the fact that the part's name is now "def". Moving a part from one stack to another produces three changes. stack1.getChildren().remove(part1); stack2.getChildren().add(part1); 1. stack1's children has changed, one part was removed. 2. part1's parent has changed. 3. stack2's children has changed, one part was added. The fact that stack1's children count went down by one and stack2's went up by one is not actually recorded. I suppose we could perform a diff between the recorded and existing list and figure out what exactly happened.
(In reply to comment #1) > The fact that stack1's children count went down by one and stack2's went up by > one is not actually recorded. I suppose we could perform a diff between the > recorded and existing list and figure out what exactly happened. Note that since part stacks generally don't have an id (although they can since MPartStack ultimately extends MApplicationElement), I don't think we can really persist these diffs in a meaningful manner.
Note that the idea of making ui elements like stacks and sashes that get arbitrarily created or destroyed by the UI (as a result of user changes) have id's is dubious at best. The 3.6 approach of naming a particular stack 'left' and using that id to extend the perspective is dubious at best. We can't properly manage perspectiveExtensions at any time other than after a 'reset' because that's the only time we can be sure that the 'left' stack exists... Note that for save/restore purposes EMF already (I believe) supports its own unique ids (for managing refs).
(In reply to comment #3) > Note that for save/restore purposes EMF already (I believe) supports its own > unique ids (for managing refs). Did you mean something like this? <children xsi:type="application:PartStack" activeChild="//@children.0/@children.0/@children.1/@children.0">
(In reply to comment #4) > <children xsi:type="application:PartStack" > activeChild="//@children.0/@children.1"> If this is the format you speak of then I do not think it is sufficient. If I persist that to disk and the application's XMI changes the source position of the part then I cannot apply this delta. Consider the case below... Program defined like so... MWindow -MPartStack (//@children.0) --ViewA (//@children.0/@children.0) --ViewB (//@children.0/@children.1) -MPartStack (//@children.1) --ViewC (//@children.1/@children.0) ...and the user changes it like so... MWindow -MPartStack (//@children.0) --ViewB (//@children.0/@children.0) -MPartStack (//@children.1) --ViewA (//@children.1/@children.0) --ViewC (//@children.1/@children.1) ...so the delta would be... //@children.0/@children.0 -> //@children.1/@children.0 Now if the program is switched to be laid out like so... MWindow -MPartStack (//@children.0) --ViewB (//@children.0/@children.0) --ViewA (//@children.0/@children.1) -MPartStack (//@children.1) --ViewC (//@children.1/@children.0) ...and we read the new definition and applied the delta, we would get... MWindow -MPartStack (//@children.0) --ViewA (//@children.0/@children.1) -MPartStack (//@children.1) --ViewB (//@children.0/@children.0) --ViewC (//@children.1/@children.0) If there were unique ids for "everything" then we could just define what elements an element container has and the rest should fall into place. Forcing everything in the model to have a unique id doesn't make for a very pleasant development experience though I suppose. And of course, if the ids change the delta's not going to be applicable anymore.
(In reply to comment #1) > That is, if your MPart's name was originally "abc" and you changed it five > bajillion times and finally settled on "def", only one change would ever be > recorded and the change would simply record that the name was originally "abc", > it does not seem to have information about the fact that the part's name is now > "def". That sorta makes sense (if you think it's a transaction). It's only the starting point that matters, as you will already have the end point. So do they expect you to create multiple change recorders "per transaction"? Although I guess I don't see how you could take the starting model and replay the change recorder and get the end point, then, unless you take the end model and the change and there's some "create reverse change" ... PW
(In reply to comment #6) > So do they expect you to create multiple change recorders "per transaction"? > Although I guess I don't see how you could take the starting model and replay > the change recorder and get the end point, then, unless you take the end model > and the change and there's some "create reverse change" ... Don't quite follow here but this is what I have right now. MWindow window = MApplicationFactory.eINSTANCE.createWindow(); window.setId("windowId"); window.setName("name"); // begin recording changes ModelReconciler reconciler = createModelReconciler(window); // change occurred window.setName("name2"); // serialize all changes thus far Object serializedState = reconciler.serialize(); window = MApplicationFactory.eINSTANCE.createWindow(); window.setId("windowId"); window.setName("name"); // apply the delta, i.e. "name" -> "name2" reconciler.applyDeltas(window, serializedState); // verify assertEquals("name2", window.getName()); This is what I have serialized right now. <changes> <MWindow id="windowId"> <name name="name2"/> </MWindow> </changes> As hinted at above, my current implementation relies on the fact that _everything_ has a unique id.
Some abstract cases to consider: ----------- Application v1.0 part name defined as "name". User changes name to "customName". Application v2.0 part name defined as "name2". Result: part's name is "customName" / USER wins ----------- Application v1.0 part has a part in a stack. User changes an attribute of the part. Application v2.0 part no longer shows up in the stack by default. Result: part is not shown in the stack / APPLICATION wins ----------- Application v1.0 part has a part in a stack. User hides the part so it is no longer in the stack. Application v2.0 part has a change in one of its attributes. Result: part is not shown in the stack / USER wins ----------- Application has three parts in a stack, A, B, and D. User hides part B. Application now has four parts in a stack, A, B, C, and D. Result: stack should show A, C, and D / USER wins
Created attachment 153189 [details] Model merging patch v1 Only a very small group of changes are recorded at the moment. For example, stuff like names, icon URIs, part visibility, part positions, keybinding/command references, window positions and sizes are recorded. There are a 198 tests at the moment which really just barely scratches the surface of the ways a user may interact with the model. The vast majority of these tests are isolated and merely verifies one single change. The "play-by-play" tests are captured in ModelReconcilerScenarioTest. The tests should be completely interchangeable with another implementation of the ModelReconciler class so it should be possible for clients to provide their own serialization mechanism.
We can do diffs of models with EMF Compare but I haven't quite yet got a handle on how to perform the merge. We can diff the original model with the user's copy for the deltas but then I'm not sure how we can get the deltas mapped onto the new model correctly. It sounds to me like it's ripe for the disaster described in comment 5. I'll have to look into EMF Compare's 3-way merge capabilities as the notion of an ancestor model should hopefully provide sufficient context for a predictable merged result.
At the moment, EMF Compare is able to pass 132/224 tests if the model has no ids (for anything) and 137/224 if the model has ids (for everything). It's not clear to me at the moment why there is so little gain but there you have it. Note that I did not manually intervene to produce the results above. I merely called the APIs and ran the tests. MatchModel match = MatchService.doMatch((EObject) localModel, (EObject) newModel, (EObject) ancestorModel, null); DiffModel diffModel = DiffService.doDiff(match); for (DiffElement diff : diffModel.getDifferences()) { MergeService.merge(diff, true); }
Created attachment 153281 [details] Model merging patch v2 Just to be clear, the tests don't pass. Going forward, I'll be taking a no-id policy approach although it is prone to failure. With regards to using indices to identify parts, the problem becomes complicated once you move a part from one stack to another because now it is no longer sufficient to just id it by its index because that index is invalid since its position has changed from the original model. The part's original parent would have to be found first and then we'd have to recursively go up the chain and check all of those parent containers (in case any of those had its parent changed). :|
Created attachment 153302 [details] Model merging patch v3 (In reply to comment #12) > With regards to using indices to identify parts, the problem becomes > complicated once you move a part from one stack to another because now it is no > longer sufficient to just id it by its index because that index is invalid > since its position has changed from the original model. The part's original > parent would have to be found first and then we'd have to recursively go up the > chain and check all of those parent containers (in case any of those had its > parent changed). :| Implementing this has not been very fun. We're now at 127/228. The current failures are all related to keybindings. I'll have to refactor all reference-related operations later to batch them up so they can all be run in one go. Processing them one at a time is a recipe for disaster because (I'm pretty sure) the current lookup strategy will fail if I start moving something that others depend on. I'm pretty sure it will cause a cascade of failures because of the outdated information.
Created attachment 153304 [details] Model merging patch v4 237/237 tests pass.
Created attachment 153310 [details] Model merging patch v5 508/508 tests pass (tests have doubled to consider the case where all MApplicationElements have ids). MPart's tool bar (getToolbar()) and menus (getMenus()) don't seem to return the MPart itself when eContainer() is invoked on them. I've added extra code to consider these two cases.
Created attachment 153313 [details] Model merging patch v6 Added support for preserving MGenericTile/MTrimContainer horizontal orientations. Also placed some simple tests for MHandledItem, will need to buff it up some more. Non-exhaustive list of unimplemented features: -sash weights are not considered, I think Eric said he was going to change this to not use a List -child creation handling, if an application has one window then another one is created "in front" of the original, the id lookup will fail since it'll look for window0 which is now actually window1, no idea how to solve this at the moment -error reporting to notify of failed merges
(In reply to comment #16) > -child creation handling, if an application has one window then another one is > created "in front" of the original, the id lookup will fail since it'll look > for window0 which is now actually window1, no idea how to solve this at the > moment So I kind of tried to explain this in words today at the UI call. Perhaps composing it in words instead of being forced to blabber on-the-spot will produce better results. ;) Say you have the 'Problems' view and 'Console' in a stack and their parent stack container is currently identified by the (generated) id partContainer.0. Now if you were to select one of the views and drag it towards the left area of the stack and release, you would now be able to see both views in their own part containers. The problem of course is that the id information is now stale because partContainer.0 now refers to the newly created container (on the left) instead of the original (on the right). I suppose one solution here might be to "force" these user generated elements to have a UUID assigned to it. I presume the renderer will be the one that handles the DND action and subsequently constructs the model element so there's nothing stopping us from assigning a random UUID to it for bookkeeping purposes.
Created attachment 153537 [details] Model merging patch v7 (In reply to comment #16) > -sash weights are not considered, I think Eric said he was going to change this > to not use a List Threw in a few tests and a basic implementation for this (since it'll be canned when the "weights" attribute is rewritten, see bug 296478). I'm going to do some testing and then I hope to actually commit to HEAD...after I speak with Paul to get the build sorted out, that is. ;) > -error reporting to notify of failed merges Returning an IStatus seems like a good way to do this in my opinion. Though it got me thinking that maybe the applyDeltas(Object, Object) method should actually return an array/collection of "operations" that can be applied one-by-one (instead of having the delta application be performed in the applyDeltas method). Then the client can actually pick and choose what to apply and what not to apply, some food for thought I suppose...
(In reply to comment #17) > I suppose one solution here might be to "force" these user generated elements > to have a UUID assigned to it. I presume the renderer will be the one that > handles the DND action and subsequently constructs the model element so there's > nothing stopping us from assigning a random UUID to it for bookkeeping > purposes. I talked to Boris about this and he was wondering if there was an EMF way to do it. It sounds suspiciously like bug 161599.
Created attachment 153639 [details] Model merging patch v8 Just committed this to HEAD. If the demos starts going crazy, then you should be able to just switch the ResourceHandler.RESTORE_VIA_DELTAS flag to 'false' to get the regular behaviour back. Note that while it's 'true', only the deltas are persisted to disk. So the first time you flick it to 'true', it will be loading the original XMI and not the version you "last opened".
(In reply to comment #20) > So the first time you flick it to 'true' That should be 'false'. ~_~
Why don't use the EMF changer Recorder? It provides a transaction concept to valid whole operations or nothing. It is the solution we'll use for PMF.
(In reply to comment #22) > Why don't use the EMF changer Recorder? It provides a transaction concept to > valid whole operations or nothing. It is the solution we'll use for PMF. Yves, that is what I'm using at the moment. However, it doesn't capture a sufficient amount of information because it just records the state of the application before anything happened. This is not sufficient for more complex operations.
With the org.eclipse.emf.ecore.extension_parser extension point, we can hook an EMF Factory implementation for the XMI. This will enable us with the ability to provide an identifier for the EMF objects via overiding XMLResource's setID(EObject, String) and getID(EObject) methods. By default, we can tell EMF to use UUIDs by overriding useUUIDs() to return 'true'. One issue I've discovered with the "default" behaviour is that if I remove an object, say, I have a window and I remove a part, the mapping between that part and its id is lost. This presents a major problem when I try to do a diff to see which part was removed because I can no longer identify said removed part. We can solve this problem if we spin our own thing and keep these entries around but it sounds like a memory leak waiting to happen to me.
(In reply to comment #24) > With the org.eclipse.emf.ecore.extension_parser extension point, we can hook an > EMF Factory implementation for the XMI. This requires a file extension. Currently, we use XMI but we obviously don't want to hijack everyone's XMI files. What file extension should we use? e4x? exmi? e4xmi?
(In reply to comment #25) > This requires a file extension. Currently, we use XMI but we obviously don't > want to hijack everyone's XMI files. What file extension should we use? e4x? > exmi? e4xmi? I like e4xmi ... not likely to clash with something else. PW
I've added code to use the e4xmi resource factory. The reconciler has been _disabled_ for the time being since our model files all use the xmi file extension at the moment. The constants are now retrieved from the EMF features so we should be able to react to model changes much easier. I've also made a bug fix where the code would inadvertently alter the model during delta construction, yikes! :/
(In reply to comment #25) > (In reply to comment #24) > > With the org.eclipse.emf.ecore.extension_parser extension point, we can hook an > > EMF Factory implementation for the XMI. > This requires a file extension. Currently, we use XMI but we obviously don't > want to hijack everyone's XMI files. What file extension should we use? e4x? > exmi? e4xmi? Thanks, this could be the solution to read/write XWT for EMF.
Just hit a roadblock today, consider the following... 1. Window has a stack, originalStack, with parts A, B, and C. window -originalStack --A --B --C 2. User adds a stack in front, userStack, and puts part A in it. window -userStack --A -originalStack --B --C 3. A new version of the program comes out with a new layout. window -originalStack --A --B -newStack --C 4. The expected merge result (I suppose?) would be the following. window -userStack --A -originalStack --B -newStack --C The current 3-way merge algorithm does not handle this case properly. Original: A, B, C User: B, C New: A, B Expected: B Actual: B, C In this case it would merely be the intersection of all three of these which I suppose doesn't really sound like much of a "merge". I suppose in general one would expect the outcome to be B and C (it is how I envisioned and implemented it and is naturally the reason why the actual outcome is B and C at the moment) but this only makes sense if the user had actually _specifically_ set B and C to be there. Let's consider the problem from a different angle... Original: A, B, C User: -A New: -C (A + B + C) - A - C Expected: B Actual: B, C ...now it makes perfect sense for B to be the last man standing! Currently, I merely record the original list (A, B, C) and the "end" list upon shutdown (B and C). It seems like I need more metadata in my delta to capture this better...
What would the following merge into? Original: window -stack --partA User: window -stack --partA --partB New: window -stack --partA --partC Would it be A, B, C? Would the user intend B to follow A? Or would it be A, C, B, where the user intended B to be the final part? I would argue that A, B, C makes sense on the basis that new parts should get thrown at the end...but then maybe C isn't actually a "new" part, ooo... This list of "what goes where" is going to grow exponentially, fast, as I draft up more scenarios so I'm starting to wonder what the _general_ strategy/contract should be with these cases?
(In reply to comment #30) > This list of "what goes where" is going to grow exponentially, fast, as I draft > up more scenarios so I'm starting to wonder what the _general_ > strategy/contract should be with these cases? I think going forward I'll just adopt a policy of "whatever the user had and then throw everything at the end".
(In reply to comment #31) > > I think going forward I'll just adopt a policy of "whatever the user had and > then throw everything at the end". I guess we have a couple of options here. I would say our guiding principles should be 1) do what makes sense if we can understand the corner case and 2) at least provide predictability. We would want to support our main usecases under #1, plus if there's a couple corner cases that show up in our upgrade path and cause pain we should definitely handle that gracefully under #1. The Devil's in the details, however :-) PW
(In reply to comment #31) > I think going forward I'll just adopt a policy of "whatever the user had and > then throw everything at the end". In hindsight, this is an acceptable idea for parts and stacks but a horrible one for top-level menus in a window and perhaps to a lesser extent, the menu items of said menus.
(In reply to comment #33) > (In reply to comment #31) > > I think going forward I'll just adopt a policy of "whatever the user had and > > then throw everything at the end". > > In hindsight, this is an acceptable idea for parts and stacks but a horrible > one for top-level menus in a window and perhaps to a lesser extent, the menu > items of said menus. Although, to be fair, I'm not sure it's possible for an end user to add menus randomly. In 3.x you can turn them on and off but those menus are technically already understood by the system and the user is not arbitrarily adding completely _new_ menus. Merging visibility flags is trivial (by comparison), actual 3-way merging, mm, not so much.
(In reply to comment #27) > I've added code to use the e4xmi resource factory. The reconciler has been > _disabled_ for the time being since our model files all use the xmi file > extension at the moment. The reconciler has been reenabled and copy/paste versions of the contacts and photo demo's xmi files has been created with new e4xmi counterparts. Sash weights are being restored properly but not the window's size. This is a consequence of the renderer IIRC, investigating...
(In reply to comment #35) > Sash > weights are being restored properly but not the window's size. This is a > consequence of the renderer IIRC, investigating... Resolved by bug 298411.
A RCP developer can set almost everything in the EMF model. What it cannot capture is _context_. Right now the delta calculation only analyzes the EMF changes. Active part information certainly seems like its within the scope of the delta reconciler but it just all sounds kinda fishy to me...
There was a shutdown problem that was caused by my usage of "xmi:id" as an attribute name. I've swapped it to be "xmiId" now. Dave told me about some DOM APIs that would perform the saving (instead of using XSLT) so I'll look into switching that post-M3.
The "strategy" has been devised (use ChangeRecorder). We'll use new bugs to address new problems.
