Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 355228

Summary: Modifications to the EModelService.cloneElement() API
Product: [Eclipse Project] Platform Reporter: Dean Roberts <dean.t.roberts>
Component: UIAssignee: Eric Moffatt <emoffatt>
Status: VERIFIED FIXED QA Contact: Eric Moffatt <emoffatt>
Severity: normal    
Priority: P3 CC: emoffatt, pwebster, remy.suen
Version: 4.1   
Target Milestone: 4.2 M2   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on:    
Bug Blocks: 339130    
Attachments:
Description Flags
API changes none

Description Dean Roberts CLA 2011-08-19 10:46:10 EDT 
1) The clone API should not make any assumptions about which clone container to save the clone too.  Instead, I caller should pass in the container to store the clone in, null if the clone need not be stored.  Although the current snippet container is an MUIElement, I suspect this should be changed to make clone containers a first class concept.

2) After cloning a perspective there could be an arbitrary number of elements that caller will want to modify in the clone, not just the clone's ID.  The clone API should just make, optionally store, and return the clone.  The caller will modify the clone as appropriate.
Comment 1 Dean Roberts CLA 2011-08-29 11:31:55 EDT 
Created attachment 202339 [details]
API changes

This patch includes the API changes discussed.
Comment 2 Eric Moffatt CLA 2011-08-30 16:19:08 EDT 
Dean, while looking at the patch I realized that we likely want to do something similar to 'cloneSnippet'. The original API had a presumption that the snippetContainer could only ever be MApplicaiton which is incorrect.

Do you think it makes sense to change:

public MUIElement cloneSnippet(MApplication app, String snippetId)

to

public MUIElement cloneSnippet(MUIElement snippetContainer, String snippetId)

If so could you make a revised patch with both ?
Comment 3 Dean Roberts CLA 2011-08-31 07:44:05 EDT 
Yes I think it makes sense.... However, overall, I think I would prefer the bigger change making SnippetContainer a real concept, and getting it off of MUIElement.  It seems to me that there are lots of live MUIElements in memory at any given time, and even saving the few bytes needed to hold an empty snippetsContainer slot might be a moderate savings.

This also made me think, and then confirm, that deleting user saved perspectives does not work.  And that, among other changes, we will need some API do delete snippets.  I created a new defect and assigned it to myself.  I'll work on this today.

https://bugs.eclipse.org/bugs/show_bug.cgi?id=356307
Comment 4 Eric Moffatt CLA 2011-09-01 14:11:38 EDT 
Committed in >20110901.

commit a22bb68dab89f4a1fa3ae84d05e3cff95fca0b16

While there is no explicit 'deleteSnippet' this functionality is trivial to implement using just the EMF calls.

We will be reviewing whether *every* ui element needs to be a snippet container later in 4.2...as per Bug 356307.
Comment 5 Dean Roberts CLA 2011-09-15 13:41:25 EDT 
Verified with I20110705-1340 by exercising the save perspective code.
Comment 6 Dean Roberts CLA 2011-09-15 13:48:41 EDT 
Cut and paste error in previous comment.

This was verified in I20110915-0200