Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 353328 - ModelService.cloneElement() should not clone widget and renderer fields
Summary: ModelService.cloneElement() should not clone widget and renderer fields
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.1   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 4.2 M2   Edit
Assignee: Eric Moffatt CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 339130
  Show dependency tree
 
Reported: 2011-07-28 10:21 EDT by Dean Roberts CLA
Modified: 2011-09-15 13:48 EDT (History)
4 users (show)

See Also:


Attachments
Add derived=true to all transient fields (8.30 KB, patch)
2011-07-28 15:12 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-07-28 10:21:00 EDT
The cloned snippets API used by the upcoming SavePerspectiveAs functionality makes a deep copy that includes system resources such as widget and renderer (and possibly others?).

It should not copy these types of fields.
Comment 1 Dean Roberts CLA 2011-07-28 10:23:07 EDT
The EMF default copier will not copy EStructureFeatures with isDerived set to false.  I will submit a patch today (as soon as I figure out GIT) :-)
Comment 2 Dean Roberts CLA 2011-07-28 15:12:53 EDT
Created attachment 200551 [details]
Add derived=true to all transient fields

The .ecore file adding derived=true tags to all transient fields in the model.

I believe the right work flow is to only include the .ecore file in the patch so that the committer can apply the patch and generate the model before commit.
Comment 3 Paul Webster CLA 2011-07-29 21:36:41 EDT
Just a comment, when regenerating the workbench model, make sure you regenerate org.eclipse.e4.ui.model.workbench.edit as well

OW
Comment 4 Eric Moffatt CLA 2011-08-30 16:10:43 EDT
Committed in >20110830. Applied the patch (+ regenerated code including 'edit')
Comment 5 Remy Suen CLA 2011-08-31 09:22:19 EDT
Reopening this bug as the changes have been reverted, see bug 356285.
Comment 6 Remy Suen CLA 2011-08-31 09:25:38 EDT
Per comment 2, we make the assumption that any transient EMF fields should also be derived. Is this assumption valid?

While this assumption broke Eclipse and caused bug 356285, it is not to say that the assumption is invalid (but an indication that perhaps our menu rendering story needs work).

Should we take the approach where most fields are transient and derived except a select few? Most fields are transient but only a few are derived?
Comment 7 Eric Moffatt CLA 2011-08-31 14:08:04 EDT
It turns out that the initial idea of 'no transient fields should be cloned' (i.e. 'transient' == 'derived') is naive (if not outright wrong). The 'clone' implementation just uses ECoreUtil's 'copy' mechanism (and I'd like to keep it hat way).

The feature this work started out for is to allow the implementation of the Perspective's 'Save As...' and related functionality. In this case the clone most not contain rendering info (it will be rendered when it is opened).

It turns out that parts of the command handling use EMF's copy mechanism as well, leading to today's broken build (sorry about that) so a new approach is needed.

For now I'm going to make a revised version of the patch that restricts the use of 'derived' to specifically those fields that are used to contain instance-specific data; 

Contribution: 'object'
Context:      'context'
Dirtyable:    'dirty'
UIElement:    'widget' & 'renderer'
Placeholder:  'curSharedRef'
Comment 8 Eric Moffatt CLA 2011-09-01 13:45:50 EDT
Committed in >20110901. Make the changes as per the last comment.

commit 128b649c05426407399735d4d6d3a3f64173a758
Comment 9 Dean Roberts CLA 2011-09-15 13:40:13 EDT
Verified with I20110705-1340 by exercising the save perspective code.
Comment 10 Dean Roberts CLA 2011-09-15 13:48:04 EDT
Cut and paste error in previous comment.

This was verified in I20110915-0200