Bugzilla – Full Text Bug Listing
| Summary: | Unexpected focus behaviour with Parts containing a ToolControl and a @Focus method | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Brian de Alwis <bsd> | ||||||||||||
| Component: | UI | Assignee: | Brian de Alwis <bsd> | ||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | bokowski, ob1.eclipse, remy.suen | ||||||||||||
| Version: | 1.0 | Flags: | bsd:
review+
remy.suen: review+ |
||||||||||||
| Target Milestone: | 1.0 RC3 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 173827 [details]
revised patch, including tests
Added UI tests for the three different conditions:
(1) activating a part when another part is active should cause the part to be activated and trigger @Focus
(2) setting the focus to a constituent widget of a part when another part is active should cause the part to be activated but not trigger @Focus
(3) setting the focus to a constituent widget of the active part should not cause any activation change
All tests pass (well, except for an unrelated test of the model processor), and I've been running with this change with my own code and behaviour seems fine.
The patch looks good and I couldn't find any bad effects while running the 4.0 SDK with it. +1 for releasing to HEAD. Sorry for taking so long to review this. I'v released the patch to HEAD. Some of the new JUnits fail. Created attachment 174771 [details]
JUnit failure list
The following JUnits fail for me: PartFocusTest:
testFocusChangesOnExplicitPartActivation()
testNoFocusChangeOnExplicitWidgetSelection()
testNoActivationOnExplicitInPartWidgetSelection()
*** Bug 320190 has been marked as a duplicate of this bug. *** Brian? Created attachment 174812 [details] Fix bug introduced in Bug 318847 Sorry, we're moving house, and I haven't had any time until tonight to do any computer work. This problem actually occurs due to an oversight in the changes for bug 318847: MToolControls are rendered using ToolItem SEPARATORs, which is not reflected in ToolBarRenderer.hasOnlySeparators(). As a a part with just a MToolControl (like the parts in the PartFocusTest) has its toolbar destroyed. This patch corrects ToolBarRenderer.hasOnlySeparators() to take into account MToolControls. (In reply to comment #8) > Created an attachment (id=174812) [details] > Fix bug introduced in Bug 318847 Would it be safer to check that it's really an instance of an MToolControl? Given that the model is extensible, this check for an MUIElement seems kind of risky. The check is also "at odds" with the inlined comment which may cause confusion to the reader. Created attachment 175164 [details] Fix bug introduced in Bug 318847 v2 Implemented Remy's suggestion (In reply to comment #10) > Created an attachment (id=175164) [details] > Fix bug introduced in Bug 318847 v2 Tests confirmed to be passing, +1. "Fix bug introduced in Bug 318847 v2" committed. |
Created attachment 173607 [details] Patch to implement expected focus behaviour The Problem: consider a Part with a ToolBar that contains a ToolControl implemented as a simple text field (e.g., a text search box), and where focus is currently set on a different part. Currently, selecting the ToolControl's text field will cause unexpected behaviour: 1. the part will be activated, 2. activation leads to the execution of the part's @Focus method 3. if that @Focus method sets the focus to another widget, then the original focus be removed from the ToolControl's text field and switched to another widget. The same situation arises in a part with multiple widgets (e.g., multiple text fields): selecting any text field may lead to the field losing focus from the execution of @Focus. This change of focus is disconcerting. The attached patch adds a new variant of activate() to EPartService: activate(MPart part, boolean requiresFocus); activate(MPart) simply calls activate(part, true), always requiring focus to be set. The primary caller of activate(MPart) is in AbstractPartRenderer. We change this so that when a part is to be activated, we check whether the current SWT control with focus is actually a constituent of the part-to-be-activated. If it is, then we don't need to change the focus and pass requiresFocus = false. We check whether the control is a constituent part by mapping the current-control-with-focus to its model element and then check to see if the model element is part of the part-to-be-activated's hierarchy. With this patch, I now see the expected focus behaviour. Still to do: add some tests.