GeezerVison

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

Thanks again to your contribution and we look forward to looking at it!

I see a "Contributor Form Received: Yes" now on my account on bpo. So CLA has arrived.

@the-knights-who-say-ni , what is missing? Why it is not accepted yet?

ERROR: test_ignore (test.test_multiprocessing_forkserver.TestIgnoreEINTR)
FAIL: test_stdin_broken_pipe (test.test_asyncio.test_subprocess.SubprocessSafeWatcherTests)

Those erros are not related to tkinter

I have not seen any news about this issue after the migration, but it looks like this is the most complete patch. @serhiy-storchaka could anything else be done to help with this review?

This PR is stale because it has been open for 30 days with no activity.

This PR is stale because it has been open for 30 days with no activity.

This PR is stale because it has been open for 30 days with no activity.

Any progress here? @serhiy-storchaka I think your review is requested?

This PR is stale because it has been open for 30 days with no activity.

Ping.

This PR is stale because it has been open for 30 days with no activity.

Ping.

Thank you for reminder @ZelphirKaltstahl. I'm going to finish this. Sorry that it took so much time, @mkiever.

For now, I have the following comments:

• Now there are tests for events in Lib/test/test_tkinter/test_misc.py. Please move new tests there, preferably following the style of the existing tests. Maybe it is enough to add few lines to existing tests.
• This is the only substitution that is exposed as two fields. It may be confusing. Could we only set data for virtual events and detail for other events? BTW, since even generate uses -data, not -user_data, I think that the name data is more preferable.
• There are other subsitutions, like %a and %i. Should they also be supported?

If you have no time, @mkiever, or lost interest, I'll continue this work, preserving your credits.

@serhiy-storchaka

BTW, since even generate uses -data, not -user_data, I think that the name data is more preferable.

man n event talks about user_data field. If you are about to change it to just data field of the event object it will cause additional friction.

There are also mentioning of detail field

Both user_data and detail should be, or at least behave as, object fields. It would be nice for the event object to have access to the args directly and perform substitution lazily. Anyway, because %d corresponds to 2 different fields the question of "single source of truth arrives". I'd prefer implementation with lazy field access or some private __d variable that corresponds to substitution with @property. And type checking to prevent access to user_data and detail fields (if they act as union). This issue has to be resolved or it is technical dept.

The data you are referring to is more like "argument name for the event constructor", not the name of the field of the event object itself.

UPD: WidgetViewSync is one of the predefined virtual events but it has detail field instead of user_data.

       <<WidgetViewSync>>
              This is sent to a text widget when its internal data become obsolete, and again when these
              internal data are back in sync with the widget view. The detail field (%d substitution) is
              either true (when the widget is in sync) or false (when it is not).

Well, lets keep user_data. There is already precedence -- send_event, x_root. These names are field names in the XEvent union.

As for the WidgetViewSync event, this is an error in the documentation. All virtual events have field user_data, not detail. You can look at the code of GenerateWidgetViewSyncEvent and Tk_SendVirtualEvent for generating the event and
at the code of ExpandPercents in generic/tkBind.c for substitution:

	case 'd':
	    if (flags & (CROSSING|FOCUS)) {
		int detail = (flags & FOCUS) ? evPtr->xfocus.detail : evPtr->xcrossing.detail;
		string = TkFindStateString(notifyDetail, detail);
	    } else if (flags & CONFIGREQ) {
		if (evPtr->xconfigurerequest.value_mask & CWStackMode) {
		    string = TkFindStateString(configureRequestDetail, evPtr->xconfigurerequest.detail);
		} else {
		    string = "";
		}
	    } else if (flags & VIRTUAL) {
		XVirtualEvent *vePtr = (XVirtualEvent *) evPtr;
		string = vePtr->user_data ? Tcl_GetString(vePtr->user_data) : "";
	    }
	    break;

These names are field names in the XEvent union.

If you are talking about this XEvent all fields are hidden behind respected event types, send_event and x_root aren't exposed directly.

As for class Event it can be just a dictionary. It would be nice to have inheritance similar to XEvent (abstract class) with XKeymapEvent etc. as concrete implementations (proper inheritance would allow to use object.__slots__) but it is not the scope of this pull request.

Yes, this XEvent. It is a union of different structs that correspond different event types. Each substitution usually corresponds to fields with the same name in different structures. Except "%d", which correspond to detail or user_data, depending on the event type. So, I think that %d should be mapped back to either detail or user_data field of tkinter.Event depending on the event type.

This PR is stale because it has been open for 30 days with no activity.