Bug 541680 - Editing in TableCell of TableControlSWTRenderer sets the value twice
Summary: Editing in TableCell of TableControlSWTRenderer sets the value twice
Status: RESOLVED FIXED
Alias: None
Product: ECP
Classification: Modeling
Component: EMF Forms (show other bugs)
Version: 1.16.0   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 1.20.0   Edit
Assignee: Eugen Neufeld CLA
QA Contact:
URL:
Whiteboard:
Keywords: test
Depends on:
Blocks:
 
Reported: 2018-11-29 02:34 EST by Nicole Behlen CLA
Modified: 2019-01-16 03:49 EST (History)
2 users (show)

See Also:


Attachments
First call of set (6.78 KB, text/plain)
2018-11-29 02:34 EST, Nicole Behlen CLA
no flags Details
Second call of set (4.78 KB, text/plain)
2018-11-29 02:35 EST, Nicole Behlen CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole Behlen CLA 2018-11-29 02:34:25 EST
Created attachment 276756 [details]
First call of set

Hi,

I noticed that after editing in a tablecell of the TableControlSWTRenderer the value is set twice and to undo the change, I have to call the undo twice.

I attached two thread stacks, one of the first and one of the second set.

Looks like that the first set is invoked due to notifyIfChanged of the getValue invocation and the second call the "good one" invoked by the setValue of the observable.
Comment 1 Nicole Behlen CLA 2018-11-29 02:35:36 EST
Created attachment 276757 [details]
Second call of set
Comment 2 Eugen Neufeld CLA 2018-11-29 03:41:21 EST
Hi Nicole, 

Thank you for the report. 
We will look into this.

Cheers,
Eugen
Comment 3 Eclipse Genie CLA 2018-12-03 07:42:32 EST
New Gerrit change created: https://git.eclipse.org/r/133389
Comment 4 Lucas Koehler CLA 2018-12-04 10:25:08 EST
Hello Nicole,

we investigated the issue and it is pretty complicated to fix (see notes below). Does the double setting of the value cause any problems for you? In regards to performance, the double setting should not make a difference because the second setting is filtered out by services like the validation service.
In case, you need to filter the second notification, you can do this with Notification.isTouch(). If this is true, the model did not change.

----------
----------

After testing the initial fix in the Gerrit change linked above, it became clear that this might be quite complicated to fix:
If the explicit target to model update in ECPTableEditingSupport.saveCellEditorValue is removed, value changes are no longer applied when closing the cell editor by clicking outside of it. Applying still works when pressing the Return or Tab keys.

Further observations when debugging the behavior:

* Deactivating the cell editor by clicking does trigger a Focus Out event but the target observable value is not triggered: For some reason the widget listener of the target observable is not notified when the Focus Out event is fired by the cell editor's control
* Deactivating the cell editor with Return triggers the DetailObservable value which is (most likely) created in TableControlSWTRenderer.addColumns. In this case widget listener of the target observable is notified when the Focus Out event is fired by the cell editor's control 
* If the explicit target to model in ECPTableEditingSupport.saveCellEditorValue is active, setting a new value in the cell editor sets the value two times independently of applying the value with Return or mouse click. This is unexpected because you would expect that it's set (at least) once more when using Return.
Comment 5 Nicole Behlen CLA 2018-12-04 11:51:17 EST
It doesn't cause problems of performance, but in the editor, the commandstack will contain two commands, and if a user edits in the table, and then wants to undo the changes with CTRL + Z or Undo via menu, the first undo doesn't do anything. The user needs to call undo twice.
Comment 6 Eclipse Genie CLA 2019-01-11 15:36:30 EST
New Gerrit change created: https://git.eclipse.org/r/134978
Comment 8 Lucas Koehler CLA 2019-01-14 08:21:46 EST
The merged fix does not work for enum table cells. For this a follow up bug was created: Bug 543417
However, the fix works great for text based table cells. Thank you for the contribution.
Comment 9 Nicole Behlen CLA 2019-01-14 08:24:13 EST
Thanks for accepting my patch :)
Comment 10 Jonas Helming CLA 2019-01-16 03:39:39 EST
Should we add the "test" task here?