Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#17688 closed defect (fixed)

wxEVT_STC_KEY and wxEVT_STC_URIDROPPED are not generated by wxSTC

Reported by: NewPagodi Owned by: Artur Wieczorek <artwik@…>
Priority: normal Milestone:
Component: wxStyledText Version: dev-latest
Keywords: Cc:
Blocked By: Blocking:
Patch: yes

Description

While doing research for #17680, I noticed that although these 2 events are defined, they are never actually generated by a wxStyledTextCtrl.

With Scintilla, the notification corresponding to wxEVT_STC_KEY is only sent on the GTK+ platform. Basically in the editor base class there is a method "int KeyDefault(int key, int modifiers)" which simply returns 0. To generate the notification, the GTK+ implementation overrides this method as follows:

int ScintillaGTK::KeyDefault(int key, int modifiers) {
	// Pass up to container in case it is an accelerator
	NotifyKey(key, modifiers);
	return 0;
}

Adding a similar override to ScintillaWX does cause key events to be generated. There's not really any other way to get the information this event provides. On the other hand, is that information really useful? No one has needed it for as long as wxSTC has been around.


The notification corresponding to wxEVT_STC_URIDROPPED is also only sent on GTK. The code it uses is as follows:

void ScintillaGTK::ReceivedDrop(GtkSelectionData *selection_data) {
	dragWasDropped = true;
	if (TypeOfGSD(selection_data) == atomUriList || TypeOfGSD(selection_data) == atomDROPFILES_DND) {
		const char *data = reinterpret_cast<const char *>(DataOfGSD(selection_data));
		std::vector<char> drop(data, data + LengthOfGSD(selection_data));
		drop.push_back('\0');
		NotifyURIDropped(&drop[0]);
...

where the ... is the rest of the code for dragging text around in the control.

I'm not sure what atomUriList and atomDROPFILES are, but I would guess that wxSTCDropTarget could be modified to handle inputs to give similar behavior to this code. But I'm not sure that would be a good idea. It seems to me that is something that should be left to the user to handle if they want to.


I'm not sure what the best thing to do is. Here are 4 suggestions:
1) remove the references to these events from the documentation, but leave everything else. This is certainly guaranteed to not break any existing code.
2) remove both events - since their not implemented, no one could possibly be using them.
3) implement wxEVT_STC_KEY. That requires only a very small change.
4) implement both events as described above.

I would favor 1 or 3.

Attachments (1)

stcunusedevents.patch download (9.3 KB) - added by NewPagodi 3 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 3 years ago by NewPagodi

  • Patch set

After thinking about this for several months, I think the correct solution is to only remove remove references to those events from the documentation.

Despite what I said above, the information from the wxEVT_STC_KEY can be obtained from the wxEVT_KEY_DOWN event. And the information that could be provided by URIDROPPED is best handled by the wxWidgets drag and drop events.

So for the same reasons given in #17664 for not implementing SCN_FOCUSIN and SCN_FOCUSOUT notifications, these 2 notifications shouldn't have had corresponding events.

I'm attaching a small patch that
1) removes the reference to these 2 events in stc.interface.h.in so that they will no longer be listed in the documentation.
2) removes the check for these 2 notifications from the NotifyParent method in ctc.cpp.in. Since these 2 notifications are never sent by the the ScintillaWX member if the StyledTextControl, there is no need to check for them.

This leaves those event types defined. I think this the best approach. That way going forward, no one should try to use those event types since they won't be listed in the docs. But if there are any existing applications that tried to use those events, the code will continue to do the same nothing it's always done; but it won't be broken.

comment:2 follow-up: Changed 3 years ago by awi

  • Status changed from new to infoneeded_new

I think removing dead code is fine.

Some remarks:

  1. Maybe it would be good to raise some warnings if someone by chance implemented handlers for these never generated events (stc sample is an example for EVT_STC_KEY), with e.g. something like this:
    wxDEPRECATED_MSG( "Don't handle this event. It's never generated." ) \
    wxDECLARE_EXPORTED_EVENT( WXDLLIMPEXP_STC, wxEVT_STC_KEY, wxStyledTextEvent );
    
  1. Maybe it would be helpful to add in the documentation a list of Scintilla notifications which are not mapped to the wx events?

Changed 3 years ago by NewPagodi

comment:3 in reply to: ↑ 2 Changed 3 years ago by NewPagodi

  • Status changed from infoneeded_new to new

Some remarks:

  1. Maybe it would be good to raise some warnings if someone by chance implemented handlers for these never generated events (stc sample is an example for EVT_STC_KEY), with e.g. something like this:
    wxDEPRECATED_MSG( "Don't handle this event. It's never generated." ) \
    wxDECLARE_EXPORTED_EVENT( WXDLLIMPEXP_STC, wxEVT_STC_KEY, wxStyledTextEvent );
    

I've updated the patch to add to do that.

I didn't realize events could be declared deprecated like that. Another event that might deserve similar treatment is wxEVT_STC_CHANGE. In the scintilla documentation, it is listed in a section starting with "The following additional notifications are sent using a secondary "command" method and should be avoided in new code as the primary "notification" method provides all the same events with richer information."

I've also removed the event handler for wxEVT_STC_KEY from the sample since a deprecated event shouldn't be demonstrated.


  1. Maybe it would be helpful to add in the documentation a list of Scintilla notifications which are not mapped to the wx events?

I've added a paragraph at the end of the list of events to list those notifications and the alternatives that can be used for them.

comment:4 Changed 3 years ago by Artur Wieczorek <artwik@…>

  • Owner set to Artur Wieczorek <artwik@…>
  • Resolution set to fixed
  • Status changed from new to closed

In e68cafaf946a8cc2987beb3c2724eed5e5917ef7/git-wxWidgets:

Remove references to never generated wxSTC events

EVT_STC_KEY and EVT_STC_URIDROPPED events are never generated so there is no need to reference them in the code and documentation. For backwards compatibility reasons their declarations are not entirely removed but marked as deprecated.

Closes #17688.

comment:5 Changed 3 years ago by Artur Wieczorek <artwik@…>

In bac4d24bf9b259c37ae2b04961c6a219e9b440d8/git-wxWidgets:

Don't handle deprecated event in stc sample

EVT_STC_KEY is not generated and there is no reason to handle it.

See #17688.

comment:6 Changed 3 years ago by Artur Wieczorek <artwik@…>

In 9f8ae27d0a44caf871d91002653693cca7ebbde4/git-wxWidgets:

Add missing comments

In e68cafaf946a8cc2987beb3c2724eed5e5917ef7 some comments were added to stc.h but not to stc.h.in so at next run of gen_iface.py those changes would be undone. We need to synchronize stc.h.in with stc.h.

See #17688.

Note: See TracTickets for help on using tickets.