error handling, fix #2

[Andrei-Pozolotin]

No description provided.

[qu1ck]

Thanks for the invite.
Few comments below.

[Andrei-Pozolotin]

Thanks for the invite.

I have another idea I wanted to run by you.

I want to propose to "the community" to create common plugin repository
https://github.com/kicad-extra
and move all plugin projects there, so that all devs can own all projects, similar to
https://github.com/KiCad

what do you think?

[qu1ck]

I don't think having common plugin repository will work well. People have too different coding standards, styles, processes. And it's ok but it won't work well in a single repo with shared ownership.

I had ideas about plugin manager in KiCad that would provide central discovery point as well as manage updates, compatibility checks. List of plugins showed there would be centrally managed by KiCad team and to apply to be listed a plugin would have to pass some conformity checks like have predefined format that plugin manager can parse to extract description, version info and other metadata.

I think that will work much better in the long run. Once I finalize my proposal I will present it on kicad dev list.

[Andrei-Pozolotin]

I don't think having common plugin repository will work well.

got it, thank you. one vote down :-) kicad-extra/arkon#1

[Andrei-Pozolotin]

I squashed the commits, perhaps it is easier to live test this pr now.

[qu1ck]

I did some testing on both linux and win. Both platforms have their issues with current implementation that I outlined in comments above.

I believe this should work:

1. Get rid of wx.App(). I tested this on both platforms, it doesn't break wx.CallAfter() when run from kicad.
2. Make terminate dialog actual wx.Dialog instead of wxMessageDialog. It's a bit of tedious work but can be simplified a lot with wxFormBuilder.
3. Close dialog in ProcessThread with EndModal, destroy right after ShowModal()
4. I think you need to join() on the thread after it's done. You may be getting away with it because it's daemon thread but it may leak resources if you don't.

[Andrei-Pozolotin]

great stuff, thank you. applying your ideas now.

[Andrei-Pozolotin]

please test

results so far:

1. Get rid of wx.App(). I tested this on both platforms, it doesn't break wx.CallAfter() when run from kicad.

wx.CallAfter still crashes here w/o wx.App. likely due to use of phoenix

build options:
https://github.com/random-builder/kicad_freerouting-plugin/blob/master/doc/build-options.md

current work around is based on:
https://git.launchpad.net/kicad/tree/pcbnew/python/kicad_pyshell/__init__.py#n89

2. Make terminate dialog actual wx.Dialog instead of wxMessageDialog. It's a bit of tedious work but can be simplified a lot with wxFormBuilder.

works, except could not figure out how to auto-resize to fit text

3. Close dialog in ProcessThread with EndModal, destroy right after ShowModal()

works

wow, based on the diagram https://docs.wxwidgets.org/3.0/classwx_message_dialog.html
who would guess: wxMessageDialog is not actually a wxDialog so it ignores EndModal :-)

4. I think you need to join() on the thread after it's done. You may be getting away with it because it's daemon thread but it may leak resources if you don't.

works

[qu1ck]

I dug a bit deeper on the wx.App issue and found this:
KiCad/kicad-source-mirror@8b06079
which led me to this:
https://bugs.launchpad.net/kicad/+bug/1809913
and this:
wxWidgets/Phoenix#1126

It boils down to this. You can't do gui stuff from non-gui thread so you need wx.CallAfter, you can't do CallAfter without wx.App so you create one. But then you bork the global wxApp instance reference in c++ land and Kicad event loop never finishes.

I guess not much can be done about it on plugin side at the moment so we will have to live with this.

I tried to move wx.App() init into plugin method so that new App instance would be garbage collected. Plugin works but that makes things way worse with random crashes in c++ code after plugin finishes 🤣 which is not unexpected given the above.

wow, based on the diagram...

Yeah, misleading, I know :) I think it's because wxGtk (and probably wxMsw too) use native error boxes that just don't have the needed APIs to handle EndModal and some other things so it's ignored.

works, except could not figure out how to auto-resize to fit text

Use wx.StaticText instead of TextCtrl and it will autosize properly on sizer.Fit().

[qu1ck]

I just had an idea that instead of trying to coerce gui thread to do what we need with CallAfter() we could do it the old fashioned way - by sending it an event. Some quick googling on the topic popped this great collection of the solutions:
https://wiki.wxpython.org/LongRunningTasks

Very first one there is detailing the solution I am talking about and it has a great code example too.

Keep the dialog around so that you have something to send an event to and treat the event handler as CallAfter() function.
I'm pretty sure this would solve all the issues.

[Andrei-Pozolotin]

please review

I dug a bit deeper on the wx.App issue

cool, and @sethhillbrand seems to agree on the following work around
https://bugs.launchpad.net/kicad/+bug/1827742

kicad should automatically provision global wx.App instance by default,
similar to how it is done in kicad_pyshell

Use wx.StaticText instead of TextCtrl

works, thank you

I just had an idea that instead ... by sending it an event

ok, thinking. look at what wx.CallAfter is doing:
https://github.com/wxWidgets/wxPython/blob/master/src/_core_ex.py#L146

[Andrei-Pozolotin]

also: wxAnyThread
https://github.com/BrendanSimon/wxanythread/blob/master/wxAnyThread/__init__.py

[qu1ck]

ok, thinking. look at what wx.CallAfter is doing

Yeah, it's doing same thing underneath but it's sending the event to App. You can avoid that if you send it to a dialog, that way you don't have to create your own App at all.
wxAnyThread seems to be using same idea. But if you choose to use that I suggest you copy that code into your repo instead of adding it as dependency. Installing such deps is bothersome on all platforms except linux.

Also feel free to commit this as is and update to event posting code later if you prefer. Change looks good to me.

[Andrei-Pozolotin]

Yeah, it's doing same thing underneath ...

Result so far:

• wx.PostEvent works for ProcessThread -> ProcessDialog notification
• any attempt to bypass wx.App instance crashes kicad

Also feel free to commit this as is

Yes, lets do that for now.

Change looks good

Thank you so much for your support!