Grid Editor seems to assume client's view is current; should it?

Vaadin 6 / 7 / 8
1

Consider this code in com.vaadin.ui.components.grid.EditorImpl (Vaadin 8):

        public void bind(String key) {
			// [...]

            doClose();
            doEdit(getData(key));
            rpc.confirmBind(true);
        }
					
protected void doEdit(T bean) {
    Objects.requireNonNull(bean, "Editor can't edit null");
	// [...]

}

It is usually triggered by a double click on a data element, identified by key, to start editing it.

The code seems to take it for granted that the data element identified by key (still) exists. But in fact it is fairly easy to provoke a NullPointerException in doEdit, most naturally by putting a Delete Button into the row and then double clicking it: The first click will delete the data object, the second one will try to edit it.

I would just create an issue for it, but it prompts a more general question:

Should server-side components just assume that the client was looking at the most recent update from the server when the request was sent? Is there a higher-level mechanism dealing with this, or is it left to the component developer? There seems to be some mechanics in place which looks like it is meant to address this. Most notably, a variable called lastSyncIdSeenByClient, but I can only trace it to ServerRpcHandler.parseInvocation, where it is left unused.

Is this a problem specific to what I am doing, i.e. the single-click vs. double-click situation, or is this a proper family of race conditions that can happen any time grid content is changed? Note that in my case, there is nothing explicitly asnychronous, anywhere.

Or am I getting something totally wrong?

2

The code seems to take it for granted that the data element identified by key (still) exists. But in fact it is fairly easy to provoke a NullPointerException here, most naturally by putting a Delete Button into the row and then double clicking it: The first click will delete the data object, the second one will try to edit it.

Yes. I have seen this happening. The reason is that button renderer lets the click event propagate to Grid. This was one of the things, why I made DeleteButtonRenderer in Renderers Collection add-on (i.e. stopping propagation in that button, and doing other things)

https://vaadin.com/directory/component/grid-renderers-collection-for-vaadin7

Also in Grid Column we have added setHandleWidgetEvents(…) method (since 8.3), which could be used also to prevent this happening

https://vaadin.com/download/release/8.4/8.4.4/docs/api/com/vaadin/ui/Grid.Column.html#setHandleWidgetEvents-boolean-

3

Tatu Lund:
setHandleWidgetEvents(…);

Thank you so much, that totally solved my problem.

I’d still be curious about your opinion who’s misbehaving here: Should the code in EditorImpl just check for null, because (technically) anything could have happened in the meantime? Or should the client not send a double-click event before having waited for the response to the single-click-event? Or neither? Or both? :slight_smile:

Just in case I want to try and write a small Component myself at one point.

4

Michael Zilske:

Tatu Lund:
setHandleWidgetEvents(…);

Thank you so much, that totally solved my problem.

Have to take that back (about solving the problem), it’s the other way round: The Grid is not supposed to receive events from buttons by default, and that’s true for single clicks (the row is not selected), but it is not true for double-clicks (the editor is opened). So none of this is meant to be happening in the first place.

Seems I’ve encountered a chain of nested issues…

5

This was one of the things, why I made DeleteButtonRenderer in Renderers Collection add-on (i.e. stopping propagation in that button, and doing other things)

Hmmm, in 2.2.10 of your add-on and 8.4.3 of Vaadin, the exact same thing happens: Double-click goes straight to the editor, also with your custom delete button. Maybe this was introduced after you wrote the button?

6

Double-click goes straight to the editor, also with your custom delete button.

I did some testing and I can confirm this, it is weird.