Bug 353850

Comment 1 Lan Phan 2011-08-04 02:58:07 EDT

Created attachment 200879 [details]
BasicModelEditActionProvider.patch

Patch file contains fixes for all bugs and enhancements above

Comment 2 Stephan Eberle 2011-08-05 08:39:14 EDT

(In reply to comment #0)
Thank you for submitting these bugs/enhancements and patches.

OK with 1) and 2).

For 3) we have to be careful: the call to updateActions() in both fillActionBars() and fillContextMenu() had been introduced for making sure that the Undo/Redo menu items in the Edit main menu get appropriately enabled/disabled/updated when the selection changes within the same view or across different views. The idea is that the edit menu reflects the local undo/redo stack of each view, i.e. only operations done in the active view are listed in the Edit/Undo/Redo menus but not those that have been previously done in other views.

Could you please check if this is still working with you patch?

Comment 3 Lan Phan 2011-08-05 23:42:30 EDT

> For 3) we have to be careful: the call to updateActions() in both
> fillActionBars() and fillContextMenu() had been introduced for making sure that
> the Undo/Redo menu items in the Edit main menu get appropriately
> enabled/disabled/updated when the selection changes within the same view or
> across different views. The idea is that the edit menu reflects the local
> undo/redo stack of each view, i.e. only operations done in the active view are
> listed in the Edit/Undo/Redo menus but not those that have been previously done
> in other views.
> 
> Could you please check if this is still working with you patch?
Test is ok for my patch (show different undo/redo menus for each view). As I said, fillActionBars() and fillContextMenu() are always called each selection changes, so that it's not necessary to have updateActions() in both 2 functions.
Please review if I have any mistake.

Comment 4 Romain Sezestre 2011-08-19 09:49:03 EDT

Hi Lan, could you please attach the patch you're mentioning?

Comment 5 Stephan Eberle 2011-08-23 02:23:48 EDT

(In reply to comment #4)
> Hi Lan, could you please attach the patch you're mentioning?

As far as I understand, the patch is already available:

https://bugs.eclipse.org/bugs/attachment.cgi?id=200879

Please correct me if I'm wrong.

Comment 6 Lan Phan 2011-08-23 12:33:27 EDT

(In reply to comment #5)
> (In reply to comment #4)
> > Hi Lan, could you please attach the patch you're mentioning?
> 
> As far as I understand, the patch is already available:
> 
> https://bugs.eclipse.org/bugs/attachment.cgi?id=200879
> 
> Please correct me if I'm wrong.

Yes, it's my patch.

Comment 7 Stephan Eberle 2011-08-25 02:58:40 EDT

Incorporated enhancement #1 (missing shortcut key in menu labels) in BasicModelEditActionProvider. Did the same thing in org.eclipse.sphinx.emf.explorer.actions.providers.BasicModelUndoRedoActionProvider and org.eclipse.sphinx.emf.editors.forms.BasicTransactionalEditorActionBarContributor.

Comment 8 Lan Phan 2011-08-29 04:03:03 EDT

(In reply to comment #0)
> 2. updateActions() is called in both fillActionBars() and fillContextMenu()
> It's duplication. Each time selection changed, fillActionBars() is called first
> then fillContextMenu() is called too.
> Propose to remove updateActions() calling in fillActionBars() method.

In fact, if we placed updateActions() in fillContextMenu() only, calling cut, copy, paste from hotkey (Ctrl+C, Ctrl+V and Ctrl+X) is not correct because its selection is not updated for these 3 actions.

Solution:
keep updateActions() in fillActionBars() method, remove updateActions() in fillContextMenu() method

Comment 9 Lan Phan 2011-08-29 04:15:27 EDT

(In reply to comment #1)
> Created attachment 200879 [details]
> BasicModelEditActionProvider.patch
> 
> Patch file contains fixes for all bugs and enhancements above

In this patch file, I think we can remove updateActions() calling in doInit() method. It's not necessary (fillActionBars() will be called later -> updateAction() will be called)

Comment 10 Stephan Eberle 2011-09-01 16:31:46 EDT

Fixed now also bug #1 and enhancement #2 by applying proposed patch and honoring comment #8 and comment #9.

Rejected proposed additional call to setContext(null); in dispose() - the same invocation happens already in super implementation on ActionGroup().

Applied changes for enhancement #2 also to BasicModelUndoRedoActionProvider where the same simplifications were possible.

Thank you very much for this highly reasonable contribution!

Comment 11 Lan Phan 2011-09-06 06:53:07 EDT

> 2. updateActions() is called in both fillActionBars() and fillContextMenu()
> It's duplication. Each time selection changed, fillActionBars() is called first
> then fillContextMenu() is called too.
> Propose to remove updateActions() calling in fillActionBars() method.

> In fact, if we placed updateActions() in fillContextMenu() only, calling cut,
> copy, paste from hotkey (Ctrl+C, Ctrl+V and Ctrl+X) is not correct because its
> selection is not updated for these 3 actions.

Sorry, this enhancement is INVALID. updateActions() is responsible of updating selection for all actions (cut, copy, paste and all other customized actions provided by user later) --> it should be placed in fillActionBars() (for hotkey) and in fillContextMenu() too (for calling context menu several times continously)
I'm so sorry about this enhancement, please revert it.

Comment 12 Stephan Eberle 2011-09-06 17:02:58 EDT

(In reply to comment #11)
> Sorry, this enhancement is INVALID. updateActions() is responsible of updating
> selection for all actions (cut, copy, paste and all other customized actions
> provided by user later) --> it should be placed in fillActionBars() (for hotkey)
> and in fillContextMenu() too (for calling context menu several times
> continously)

Not calling updateActions() in fillContextMenu() can only lead to problems when fillActionBars() is not called shortly before or after. Before incorporating your patch, I'd debugged a little bit and found out that this happens, as you mention too, only when you reselect an already selected element. 

But I'm wondering why it should be necessary to call updateActions() in this situation. It was already called when the element was selected for the first time (because fillActionBars() is called when a new element is selected) and all actions should therefore be perfectly in sync with the element being just reselected. So I wouldn't expect that calling updateActions() in fillContextMenu() for this element leads to any new results.

Comment 13 Lan Phan 2011-09-07 12:37:38 EDT

(In reply to comment #12)
> But I'm wondering why it should be necessary to call updateActions() in this
> situation. It was already called when the element was selected for the first
> time (because fillActionBars() is called when a new element is selected) and
> all actions should therefore be perfectly in sync with the element being just
> reselected. So I wouldn't expect that calling updateActions() in
> fillContextMenu() for this element leads to any new results.

My explanation is so short, sorry. The reason to call updateActions() in fillContextMenu is to refresh "newChildDescriptors" and "newSiblingDescriptors". "newChildDescriptors" contains information about CommandParameter used to create child for the target. CommandParameter has 3 main attributes: 'owner', 'feature' and 'child'. If we don't call updateActions() in fillContextMenu(), 'child' of CommandParameter is always the same object. But, the first time fillContextMenu() is called, we already used this 'child' and added it into model --> the second time fillContextMenu() called, 'child' is already in model but it's forced to add into model again --> exception in transaction.
The same for "newSiblingDescriptors".

I hope my explanation is clearer now. I think we can add comment before calling updateActions() in order not to receive again the same proposition.

Comment 14 Stephan Eberle 2011-09-09 01:23:16 EDT

(In reply to comment #13)
> ...
> I hope my explanation is clearer now. 

It clearly is, thank you.

> I think we can add comment before calling
> updateActions() in order not to receive again the same proposition.

So, I've added back (and committed) the call to updateActions() from fillActionBars() and also added a comment so as not to forget these non-obvious circumstances.

But what I actually want to do is to move the whole block that takes care of creating new child/sibling descriptors and actions from updateActions() to fillActionBars(). Afaics, this needs only to be done when the context menu is created/recreated but not when the selection changes. We could then remove the call to updateActions() from fillActionBars() again because there is no need to execute the remainder of updateActions() when the context menu is created/recreated. What do you think?

Comment 15 Stephan Eberle 2011-09-22 11:54:18 EDT

I'll finally leave things as they currently are, i.e., not move the block that takes care of creating new child/sibling descriptors and actions from updateActions() to fillContextMenu(). Reason: clients may want to override fillContextMenu() for customizing the context menu content and this would become much harder (because it would force clients to copy a lot more code) if the new child/sibling descriptor and action initialization were mixed in there.

Given that this bug can be deemed fixed now.