Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 355228 - Modifications to the EModelService.cloneElement() API
Summary: Modifications to the EModelService.cloneElement() API
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.1   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 4.2 M2   Edit
Assignee: Eric Moffatt CLA
QA Contact: Eric Moffatt CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 339130
  Show dependency tree
 
Reported: 2011-08-19 10:46 EDT by Dean Roberts CLA
Modified: 2011-09-15 13:48 EDT (History)
3 users (show)

See Also:


Attachments
API changes (2.17 KB, patch)
2011-08-29 11:31 EDT, Dean Roberts CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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