Community
Participate
Working Groups
Build Identifier: I20110613-1736 The adopters now does not have handle to the Selected row/item of the List Property Editor. The API should allow the user to have an handle to the selected item, something like "SelectedItemAction" which will give the user the handle to the ModelElement(s) that has been currently selected by the user int the List Property backed table Reproducible: Always
Created attachment 213750 [details] Patch v1 Is this somewhat in the right direction?
Access to selection should be at part level rather than presentation. In this case PropertyEditorPart. There is a slight difficulty in that the concept of selection isn't appropriate for all property editors, so adding the API directly to PropertyEditorPart isn't going to be appropriate. The approach that I'd like to see here is a part-level service. Something like this... ListSelectionService { List<IModelElement> selection(); void select( List<IModelElement> elements ); void select( IModelElement element ); } The service would serve as a conduit between the presentation layer (such as DefaultListPropertyEditorRenderer) and anything else that may want to see or change the selection.
Created attachment 213786 [details] Patch v2 Here is my first draft of implementation for that service. Some notes: - new service is put in org.eclipse.sapphire.ui instead of modeling because the context is Sapphire.Part - In the StandardListSelectionService only selection() works, still not sure what should happen when select(...) is called. I assume the List property renderer needs to respond when someone invokes select on this part but I need further guidance here on how to procede. - from the producer side of the selection events only the DefaultListPropertyEditorRender is updating the selection - from the consumer side the SapphirePropertyEditorHandler is listener for changes from the selection service and firing off a refreshEnablementState This rough draft is at least working for my "edit" action usecase in my adopter produt on both the enablement (only when 1 item is selected) and on the execute(). Konstantin I went ahead and added your details in the copyrights to save you the trouble when you review these changes.
> - new service is put in org.eclipse.sapphire.ui instead of modeling because the > context is Sapphire.Part Yep. That's right. > - In the StandardListSelectionService only selection() works, still not sure > what should happen when select(...) is called. I assume the List property > renderer needs to respond when someone invokes select on this part but I need > further guidance here on how to procede. The select() method should be used by the producer of the selection (DefaultListPropertyEditorRender) and any party wishing to externally modify the selection. So DefaultListPropertyEditorRender should call select() instead of updateSelection(). It should also listen on this service. When it gets a selection changed event it should first check to see if the selection shown by the service is the same as the one it already has. If not, it should modify table selection. This would handle both sides. > - from the consumer side the SapphirePropertyEditorHandler is listener for > changes from the selection service and firing off a refreshEnablementState Not all property editor action handlers are sensitive to selection, so this logic (while correct) doesn't belong in this class, but rather in a particular action handler implementation. > Konstantin I went ahead and added your details in the copyrights to save you > the trouble when you review these changes. Thanks.
Created attachment 213837 [details] Patch v3
(In reply to comment #4) > > - new service is put in org.eclipse.sapphire.ui instead of modeling because > the > > context is Sapphire.Part > > Yep. That's right. > > > - In the StandardListSelectionService only selection() works, still not sure > > what should happen when select(...) is called. I assume the List property > > renderer needs to respond when someone invokes select on this part but I need > > further guidance here on how to procede. > > The select() method should be used by the producer of the selection > (DefaultListPropertyEditorRender) and any party wishing to externally modify the > selection. So DefaultListPropertyEditorRender should call select() instead of > updateSelection(). It should also listen on this service. When it gets a > selection changed event it should first check to see if the selection shown by > the service is the same as the one it already has. If not, it should modify > table selection. This would handle both sides. > > > - from the consumer side the SapphirePropertyEditorHandler is listener for > > changes from the selection service and firing off a refreshEnablementState > > Not all property editor action handlers are sensitive to selection, so this > logic (while correct) doesn't belong in this class, but rather in a particular > action handler implementation. > > > Konstantin I went ahead and added your details in the copyrights to save you > > the trouble when you review these changes. > > Thanks. Thanks for initial review. Just submitted Patch v3 with those changes. I've tested both directions of the selection service and works ok.
Review comments for Patch v3: 1. It occurs to me that ListSelectionService isn't something that I would expect someone to provide a custom implementation for. Let's collapse StandardListSelectionService implementation into ListSelectionService. When you do this, please pull out the factory into a separate class so that it can be placed into an internal package and not become part of ListSelectionService API. 2. The implementation of ListSelectionService.select() needs to check to see if the selection it is holding is different from incoming selection before updating. Make sure to explicitly perform '==' comparison as IModelElement.equals() can employ model specific logic for element equality that isn't applicable for purposes of selection tracking. 3. In DefaultListPropertyEditorRenderer change, selection is compared using .equals(). It needs to be a '==' comparison on model elements. See above. 4. In DefaultListPropertyEditorRenderer, the listener on ListSelectionService is never removed. It should be removed when the table control is disposed. The same part can be rendered multiple times.
Created attachment 213876 [details] Patch v4
(In reply to comment #7) > Review comments for Patch v3: > > 1. It occurs to me that ListSelectionService isn't something that I would expect > someone to provide a custom implementation for. Let's collapse > StandardListSelectionService implementation into ListSelectionService. When you > do this, please pull out the factory into a separate class so that it can be > placed into an internal package and not become part of ListSelectionService API. > Done, removed StandardListSelectionService and added ListSelectionServiceFactory to internal package. > 2. The implementation of ListSelectionService.select() needs to check to see if > the selection it is holding is different from incoming selection before > updating. Make sure to explicitly perform '==' comparison as > IModelElement.equals() can employ model specific logic for element equality that > isn't applicable for purposes of selection tracking. > Done, guarded the select() methods with '==' comparisons. > 3. In DefaultListPropertyEditorRenderer change, selection is compared using > .equals(). It needs to be a '==' comparison on model elements. See above. > Fixed, had to create a special method that would check the individual elements of a list for '==' because the method getSelectedElements() will return a brand new list object after each call. > 4. In DefaultListPropertyEditorRenderer, the listener on ListSelectionService is > never removed. It should be removed when the table control is disposed. The same > part can be rendered multiple times. Added onDisposeListener to detach listSelectionService Reattached these changes as Patch v4
Getting close... 1. The if statement in ListSelectionService.select() should use isListContentsEquals() logic as well. The lists themselves aren't the important bit. Let's move this helper function to CollectionsUtil class (I just created it, you will likely need to update to see it). 2. In isListContentsEquals, a.size() is re-computed 1+n times. It should be stored in a variable instead. 3. ListSelectionService should guard against someone changing its internal selection list. That is, during select call, it should copy the list and wrap the copy in Collections.unmodifiableList(). Make sure to avoid copying/wrapping the singleton list from the single element method. Likely will need to stop delegating from one version of the method to another to achieve this. 4. With technical details largely worked out, it is time to write the doc content. We need javadoc on ListSelectionService, a section for the service in the services doc and a note in the enhancements doc.
Created attachment 213932 [details] Patch v5
(In reply to comment #10) > Getting close... > > 1. The if statement in ListSelectionService.select() should use > isListContentsEquals() logic as well. The lists themselves aren't the important > bit. Let's move this helper function to CollectionsUtil class (I just created > it, you will likely need to update to see it). > > 2. In isListContentsEquals, a.size() is re-computed 1+n times. It should be > stored in a variable instead. > > 3. ListSelectionService should guard against someone changing its internal > selection list. That is, during select call, it should copy the list and wrap > the copy in Collections.unmodifiableList(). Make sure to avoid copying/wrapping > the singleton list from the single element method. Likely will need to stop > delegating from one version of the method to another to achieve this. > > 4. With technical details largely worked out, it is time to write the doc > content. We need javadoc on ListSelectionService, a section for the service in > the services doc and a note in the enhancements doc. New patch is available with all of these changes.
Created attachment 213975 [details] Patch v6 Same as patch5 but with some editing of documentation and also merged the changes from commit 0.5.x #193
Accepted Patch v6 with some changes. Also converted existing places where list selection is tracked to use the new mechanism. Please verify.
Verified with build 0.5.x#196 Thanks for merging this.
Hmm, I can't seem to mark as Verified anymore. Did something change in permissions?
Thanks. Closing. > Hmm, I can't seem to mark as Verified anymore. Did something change in > permissions? I don't think anything has changed, but permissions do vary depending on whether you are the one to have opened the bug.
FYI, the last changes for this bug where other controls started using SelectionService has caused a slight regression. I added it to a new bug here: bug 377329
