Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 328499 - [UI] Tighten up the handling for an MElementContainer's 'selectedElement'
Summary: [UI] Tighten up the handling for an MElementContainer's 'selectedElement'
Status: RESOLVED FIXED
Alias: None
Product: e4
Classification: Eclipse Project
Component: UI (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 4.1 M3   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 298192 328633 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-10-22 14:12 EDT by Eric Moffatt CLA
Modified: 2011-01-19 13:40 EST (History)
1 user (show)

See Also:

Attachments
Patch that enforces the new rules (18.32 KB, patch)
2010-10-25 10:20 EDT, Eric Moffatt CLA 		no flags Details | Diff
Patch to remove the 'visible' flag tests (4.97 KB, patch)
2010-10-25 13:25 EDT, Eric Moffatt CLA 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Moffatt CLA 2010-10-22 14:12:19 EDT 
We've burnt ourselves at least a few times in the past by producing a model with an invalid value for a container's 'selectedElement' field. See bug 327933 for at least one case.

A valid selectedElement for a container must match the following constraints:

- It must be child of the container
- It must be visible in the UI Presentation (i.e. its TBR *and* Visible flags must be true)

The first condition is obvious. The second is a practical necessity, if the element is not visible in the UI then the container has no way to indicate the element (i.e. if its a Part in a Stack then there would be no tab item to select in the widget).

There are two requirements needed in order to enforce this:

1) It should be an exception to attempt to set a container's selected element to one that does not pass these criteria. We can do this at the lowest level by hand-modifying the generated 'setSelectedElement' method.

2) Any subsequent model change to a selected element that would break the rule should result in the container's selected element being set to 'null'.
Comment 1 Eric Moffatt CLA 2010-10-25 10:20:50 EDT 
Created attachment 181638 [details]
Patch that enforces the new rules


This is enforced in two ways:

1) The generated model code in ElementContainerImpl has been modified to throw an IllegalArgumentException if the incoming new SE fails any of the following tests:

- It must be a child of the container whose SE is being set
- It must be TBR == true
- It must be Visible == true

As a result of these changes a number of adjustments had to be made to the existing tests, they're all green now and Remy will make any necessary adjustments.
Comment 2 Eric Moffatt CLA 2010-10-25 10:22:10 EDT 
Committed in >20101025. Applied the patch.
Comment 3 Eric Moffatt CLA 2010-10-25 13:22:40 EDT 
Ooops!! the 'visible' checks cause minimized stacks to fail because the stack's 'visible' flag is false (to remove it from the regular presentation. See bug 328633.

Thanks to Remy for picking this up before it got released...
Comment 4 Eric Moffatt CLA 2010-10-25 13:23:44 EDT 
*** Bug 328633 has been marked as a duplicate of this bug. ***
Comment 5 Eric Moffatt CLA 2010-10-25 13:25:05 EDT 
Created attachment 181666 [details]
Patch to remove the 'visible' flag tests


Everything else is the same but we'll no longer test the 'visible' flag, it's only necessary that the TBR be == true...
Comment 6 Eric Moffatt CLA 2010-10-25 13:26:22 EDT 
Committed in >20101025. Applied the second patch.
Comment 7 Remy Suen CLA 2011-01-19 13:40:09 EST 
*** Bug 298192 has been marked as a duplicate of this bug. ***