Bug 330859

Comment 1 Alexander Nyßen 2010-11-22 16:46:04 EST

*** Bug 118394 has been marked as a duplicate of this bug. ***

Comment 2 Alexander Nyßen 2010-11-22 16:49:45 EST

*** Bug 114447 has been marked as a duplicate of this bug. ***

Comment 3 Alexander Nyßen 2010-11-23 14:33:40 EST

(In reply to comment #0)

> To separate the aspect of whether an EditPart may only obtain focus or whether
> it is also selectable, an additional isFocusable() method could be introduced
> to the EditPart interface, which could then be evaluated by the
> GraphicalViewerKeyHandler when searching for EditParts that accept focus.
> AbstractEditPart could specify that the EditPart does accept focus by default
> (in case it is active), and it could enforce (within
> AbstractEditPart#isSelectable()) that the edit part at least has to be
> focusable in order to be selectable. Similar to in case of
> AbstractGraphicalEditPart#isSelectable(),
> AbstractGraphicalEditPart#isFocusable() could ensure that the edit part does
> only accept focus in case its figure is visible.

Having investigated this in detail, focus and selection are so intertwined that separating the aspect of whether an edit part is selectable from the aspect of whether it may obtain focus does not seem to be a valid option.

However, there is an inconsistency between the handling of focus and selection - I have also found while investigating - that should indeed be corrected. I.e. while a primary selected edit part only gains focus implicitly (setFocus() will not get called), it may explicitly loose the (implicitly) gained focus again, if being deselected or replaced with a new primary selection (SelectionManager#appendSelection() or SelectionManager#deselect()). 

To overcome this I think the best option would be that SelectionManager explicitly passes the focus to a new primary selected edit part as well. This way clients could always rely on the fact that setFocus() is called in case focus is gained or lost by an edit part (without any implicit conventions and independent of whether this is related to a selection change or not). Furthermore, the handling of focus would be completely encapsulated within SelectionManager. To preserve the current "look & feel", SelectionEditPolicy would have to show focus feedback only in case the host is not also primary selected.

Comment 4 Alexander Nyßen 2010-11-28 14:10:24 EST

(In reply to comment #0)
> While MarqueeSelectionTool checks that only EditParts with visible figures
> (which means that the figure is showing and not completely clipped) are
> selectable, SelectionTool only checks if the EditPart is selectable (via
> EditPart#isSelectable(), which is considered to be correct), which is always
> the case by default. GraphicalViewerKeyHandler does not even take this into
> account and assumes all edit parts are selectable and may obtain focus.
> 
> As isSelected() is the interface that should be evaluated by tools to infer
> whether an EditPart is selectable or not, MarqueeSelectionTool should only rely
> on the interface alone to decide whether a given target edit part is selectable
> or not. SelectionTool may stay unchanged, GraphicalViewerKeyHandler will have
> to be adopted to also use the interface method to determine whether an edit
> part may get selected. The code currently used in MarqueeSelectionTool to check
> whether a figure is visible should be moved to
> AbstractGraphicalEditPart#isSelectable() so that a GraphicalEditPart by default
> may only be selectable in case its figure is currently visible (further each
> EditPart should only be selectable in case it is active(), so
> AbstractEditPart#isSelectable() should insure this as well). 

Well, I think I rather jumped to a conclusion here. As SelectionTool is a TargetingTool, it does not have to check for visibility of a figure, as no EditParts without actually visible figures may be targeted. And as MarqeeSelectionTool is no TargetingTool, it has to simulate this targeting behavior, thus, the check for a figure's (screen) visibility within MarqueeSelectionTool is quite fine (and is needed there alone). 

While the GraphicalViewerKeyHandler should ensure that only selectable EditParts are selected or may obtain focus (it does not do this in all situations yet; see e.g. bug 118394), it should not check for actual (screen) visibility of the navigation target's figure, as the key handler may also be used to navigate to hidden edit parts (in a sense that they are not revealed yet). The GraphicalViewerKeyHander may however  check for the figure's visibility state (isShowing) to decide whether a given edit part is a valid navigation target (as its a key handler for a graphical viewer). This however is something that should hold for all GraphicalEditParts, so it could be made the default behavior of AbstractGraphicalEditPart#isSelectable() to return true only in case the edit part's figure is showing.

Comment 5 Alexander Nyßen 2010-11-28 15:38:30 EST

(In reply to comment #3) 
> However, there is an inconsistency between the handling of focus and selection
> - I have also found while investigating - that should indeed be corrected. I.e.
> while a primary selected edit part only gains focus implicitly (setFocus() will
> not get called), it may explicitly loose the (implicitly) gained focus again,
> if being deselected or replaced with a new primary selection
> (SelectionManager#appendSelection() or SelectionManager#deselect()). 

I think this will have to be clarified as well (also because it is not fully correct). 

Currently, AbstractEditPartViewer and SelectionManager handle focus as follows:
1) select(EditPart) will not change focus if the edit part to be selected already is the focus part; it will unset the focus part (and notify the former focus part about loss of focus) otherwise.
2) appendSelection(EditPart) will not change focus if the appended edit part already is the focus part; it will unset the focus part (and notify the former focus part about loss of focus) otherwise.
3) deselect does not affect focus at all.
4) deselectAll unsets focus part, notifying the former focus part about loss of focus.

While this is not really an inconsistency I think it may be hard to understand for clients that a primary selected edit part may either explicitly (if it was the focus part before being selected) as well as implicitly (if there is no focus part) have focus, and that it may or may not get notified about gain/loss of focus (if the viewer gains or looses focus) dependent on its former state. Further, an explicit focus part may not only be set in case it differs from the primary selection, but also in case it corresponds to it; while on the other side, a primary selected edit part may also have (implicit) focus while not being the focus part.

To improve understandability I think it would be better that an explicit focus part is only set in case it differs from the primary selection (or in case there is no selection and an edit part other than the contents or root should have focus). This way, an edit part that has keyboard focus may either be the explicit focus part (in case not being selected primary) or it may be the implicit focus part (when being selected primary and no other edit part is set as explicit focus part) and both states could be clearly separated. 

Concerning the notification of edit parts about gaining and loosing focus (via EditPart#setFocus()), I think the two following options would be consistent:
a) every edit part that gains/looses focus should be notified about this (via EditPart#setFocus()), independent of whether it is the explicit or implicit (being the primary selection) focus part of the viewer.
b) only the explicit focus part should be notified about gain/loss of focus (via EditPart#getFocus()). An implicit focus part (primary selected) should not get notified.

While b) is the current behavior, which pretty much corresponds to what is needed for how focus is displayed today (implicit focus part do only show their primary selection state and no explicit focus border), I think a) would be the more consistent option. This way, the fact that the edit part viewer gains or looses focus could e.g. be graphically displayed by implicit (primary selected) focus parts as well. Further, clients could rely on the fact that - in case an edit part gains or looses focus - it always gets notified about this.

Comment 6 Alexander Nyßen 2010-11-29 14:03:07 EST

-Fixed that GraphicalViewerKeyHandler did not always evaluate isSelectable on edit parts during navigation. Now, only selectable edit parts will be navigated to. 
-Fixed that CTRL + SPACE was not properly recognized (at least on MacOSX) as a keystroke to perform selection.

Changes committed to cvs HEAD (3.7).

Comment 7 Alexander Nyßen 2010-11-29 17:36:10 EST

Changed implementation of AbstractEditPart#isSelectable() and AbstractGraphicalEditPart#isSelectable() so that only active edit parts are selectable, and furthermore graphical edit parts have to have a showing figure. Clarified that isSelectable() is not only the precondition for edit parts to get selected, but also to obtain focus (as focus is considered part of the selection state). Added assertions to AbstractEditPart#setFocus(boolean) and AbstractEditPart#setSelected(int) to ensure the receiver is selectable in case focus is gained or the part is to be selected. Adjusted javadoc comments for setFocus(boolean), setSelected(int), and isSelectable().

Changes committed to cvs HEAD (3.7).

Comment 8 Alexander Nyßen 2010-12-10 16:06:35 EST

Opened new bug #332341 to keep track of the remaining inconsistencies, which are not primarily addressed in this bug. As edit parts with invisible figures are no longer selectable, closing this one as fixed.