Feedback needed for change in plugins' event handling

[mandrav]

We are considering changing the way C::B events are forwarded to plugins for processing so as to cut down the number of events flowing around.
In case you hadn't noticed it, with the current system, a single event might end up many times in the event handler of one plugin. This is what we 're trying to solve.

The proposed approach looks like this:

• A new function is added: PluginManager::RegisterEventSink(const wxEventType evtType, void(cbPlugin::*handler)(CodeBlocksEvent&))
• Plugins remove the old entries from their event tables and use the above function in OnAttach() to register their event handlers

The above process needs minimum code changes in both the SDK as well as the plugins. Basically, from the plugin writer's point of view, we just move the event handling registration from the plugin's event table to its OnAttach() method. Really a minimum change.

By going with the above change, we can guarantee that each registered event handler will be called exactly once for each event entering the queue.
We also minimize the overhead because a normal wxEvent handled by the event table will have to enter the event processing queue and be delivered where it is needed. With this change, we 're talking about a single loop over only the registered handlers and that's it.

We should be able to gain in speed and responsiveness.
One more great advantage is that by using this approach, we could even get feedback from the event handlers because the event is passed along synchronously. Which means after we return from a call to an event handler, we could examine the event for raised flags, changed contents etc.

We 'd like to have feedback on this proposal. Please share your views

[thomas]

It's important to point out that we're talking of CodeBlocksEvents here, i.e. not of wxWidgets GUI or timer events, for example.
The latter might cause serious issues with the GUI hanging for half a second or things like that, if run synchronously.

My solution to the problem would have been to implement something like callbacks (rather this-pointer calls, actually) and kicking events for this matter alltogether. While that would probably be more efficient, it would require a lot of code changes, so Yiannis' solution is likely preferrable.

[mandrav]

It's important to point out that we're talking of CodeBlocksEvents here, i.e. not of wxWidgets GUI or timer events, for example.
The latter might cause serious issues with the GUI hanging for half a second or things like that, if run synchronously.

Correct.

My solution to the problem would have been to implement something like callbacks (rather this-pointer calls, actually) and kicking events for this matter alltogether. While that would probably be more efficient, it would require a lot of code changes, so Yiannis' solution is likely preferrable.

Well, this-pointer calls will be used so we 're really talking about the same thing .

[JGM]

The proposed approach looks like this:

• A new function is added: PluginManager::RegisterEventSink(const wxEventType evtType, void(cbPlugin::*handler)(CodeBlocksEvent&))

Could you give a concrete example of how the code will look like on the attach function, with an actual event?

[mandrav]

The proposed approach looks like this:

• A new function is added: PluginManager::RegisterEventSink(const wxEventType evtType, void(cbPlugin::*handler)(CodeBlocksEvent&))

Could you give a concrete example of how the code will look like on the attach function, with an actual event?

Old sample code:

Code

BEGIN_EVENT_TABLE(SomePlugin, cbPlugin)
...
EVT_EDITOR_SAVE(SomePlugin::OnSave)
...
END_EVENT_TABLE()

New sample code:

Code

BEGIN_EVENT_TABLE(SomePlugin, cbPlugin)
...
// remove EVT_EDITOR_SAVE entry
...
END_EVENT_TABLE()

void SomePlugin::OnAttach()
{
...
PluginManager* pm = Manager::Get()->GetPluginManager();
pm->RegisterEventSink(cbEVT_EDITOR_SAVE, &SomePlugin::OnSave);
...
}

That's more or less what needs to change in plugins. Let me stress once more that we 're only talking about Code::Blocks events here...

[David Perfors]

It is indeed a lot better then the macro stuff of Code::Blocks. It is strange that wx can't guarantee that the event isn't fired once... (I think you are saying that indirectly..)

[dmoore]

sounds good to me.

will you also set this up for all of the plugin virtuals (other than OnAttach and OnRelease)? The registration of methods like BuildMenu and BuildModuleMenu could then go into the boiler plate for the plugin wizard, which will be a nice signpost for new users.

[mandrav]

sounds good to me.

will you also set this up for all of the plugin virtuals (other than OnAttach and OnRelease)? The registration of methods like BuildMenu and BuildModuleMenu could then go into the boiler plate for the plugin wizard, which will be a nice signpost for new users.

BuildMenu and friends are not events so no, they won't be handled this way.
But after this system is implemented, we could add a new page to the plugin wizard to allow the user to (multiple-)select from all the core events and add code appropriately.

[dmoore]

BuildMenu and friends are not events so no, they won't be handled this way.

isn't the distinction artificial? (i.e. buildmenu, buildmodulemenu etc could be setup as events, passing the menu data in the event structure). However, given the time and minimal breakage constraints, I understand the decision (I just like consistency).

But after this system is implemented, we could add a new page to the plugin wizard to allow the user to (multiple-)select from all the core events and add code appropriately.

[rickg22]

I support the motion. For one reason: Rogue plugins.

Have you noticed, for example, that many plugins have the if(isAttached()) code included in their handlers? What if i make a plugin virus and i don't care if the plugin's attached or not, and execute the event handling? So yes, we need to activate the event handling only AFTER the plugin's attached. But please unregister the events automatically so the plugin writer won't have to unregister their plugins.

One thing, tho. I don't think "RegisterEventSink" is an appropriate name. How about "RegisterCBEventHandler"?

[mandrav]

So yes, we need to activate the event handling only AFTER the plugin's attached.

That's how the EXISTING implementation works, a few years now.

[rickg22]

Oh, ok

[killerbot]

sounds good, ok for me; let the performance jump forward

[mandrav]

OK people,

I just committed the changes. You should note no difference on the operation of Code::Blocks (except maybe from speed in some places - but then again you should know where to look for). The important part is that everything in our repository is updated (sdk, app, core and contrib plugins).

For plugin writers (and anyone else wanting to know about the changes), I 've written a small article on the subject: Code::Blocks SDK events.

[dmoore]

do you want this in the wiki entry?

Code

'''Events the must be registered with the RegisterEventSink'''
''app events''
* cbEVT_APP_STARTUP_DONE
* cbEVT_APP_START_SHUTDOWN
* cbEVT_APP_UPDATE_TITLE

''plugin events''
* cbEVT_PLUGIN_ATTACHED
* cbEVT_PLUGIN_RELEASED
* cbEVT_PLUGIN_INSTALLED
* cbEVT_PLUGIN_UNINSTALLED

''editor events''
* cbEVT_EDITOR_CLOSE
* cbEVT_EDITOR_OPEN
* cbEVT_EDITOR_ACTIVATED
* cbEVT_EDITOR_DEACTIVATED
* cbEVT_EDITOR_SAVE
* cbEVT_EDITOR_MODIFIED
* cbEVT_EDITOR_TOOLTIP
* cbEVT_EDITOR_TOOLTIP_CANCEL
* cbEVT_EDITOR_BREAKPOINT_ADD
* cbEVT_EDITOR_BREAKPOINT_EDIT
* cbEVT_EDITOR_BREAKPOINT_DELETE
* cbEVT_EDITOR_UPDATE_UI

''project events''
* cbEVT_PROJECT_CLOSE
* cbEVT_PROJECT_OPEN
* cbEVT_PROJECT_SAVE
* cbEVT_PROJECT_ACTIVATE
* cbEVT_PROJECT_FILE_ADDED
* cbEVT_PROJECT_FILE_REMOVED
* cbEVT_PROJECT_POPUP_MENU
* cbEVT_PROJECT_TARGETS_MODIFIED
* cbEVT_PROJECT_RENAMED
* cbEVT_WORKSPACE_CHANGED

''dockable windows''
* cbEVT_ADD_DOCK_WINDOW: request app to add and manage a docked window
* cbEVT_REMOVE_DOCK_WINDOW: request app to stop managing a docked window
* cbEVT_SHOW_DOCK_WINDOW: request app to show a docked window
* cbEVT_HIDE_DOCK_WINDOW: request app to hide a docked window
* cbEVT_DOCK_WINDOW_VISIBILITY: app notifies that a docked window has been hidden/shown to actually find out its state use IsWindowReallyShown(event.pWindow);

''layout related events''
* cbEVT_SWITCH_VIEW_LAYOUT: request app to switch view layout
* cbEVT_SWITCHED_VIEW_LAYOUT: app notifies that a new layout has been applied

''main menubar creation''
* cbEVT_MENUBAR_CREATE_BEGIN: app notifies that the menubar is started being (re)created
* cbEVT_MENUBAR_CREATE_END: app notifies that the menubar (re)creation ended

''compiler-related events''
* cbEVT_COMPILER_STARTED
* cbEVT_COMPILER_FINISHED

''debugger-related events''
* cbEVT_DEBUGGER_STARTED
* cbEVT_DEBUGGER_PAUSED
* cbEVT_DEBUGGER_FINISHED

'''Events processed on wxWidgets EVENT TABLES and not registered using RegisterEventSink'''
''pipedprocess events''
* cbEVT_PIPEDPROCESS_STDOUT
* cbEVT_PIPEDPROCESS_STDERR
* cbEVT_PIPEDPROCESS_TERMINATED

''thread-pool events''
* cbEVT_THREADTASK_STARTED
* cbEVT_THREADTASK_ENDED
* cbEVT_THREADTASK_ALLDONE