Hi Sylwester, I took a look at your repo and I have a few comments on your changes. I did not test it so there may be functional issues as well.
Please simplify your logic in DIALOG_SIM_SETTINGS::TransferDataToWindow(). It's way more complicated than it needs to be. Get rid of DIALOG_SIM_SETTINGS::disableCtrlOnCheckboxEvent() and use the appropriate wxUpdateUIEvent[1] handler to enable and/or disable any controls. Manually doing this will almost certainly lead to broken control states. We have done this poorly so many times in the past that I am not allowing it in new code. If you want to submit your changes for consideration, you should squash you changes into a single patch and submit the output of `git format-patch` to the mailing list. Cheers, Wayne [1]: https://docs.wxwidgets.org/3.0/classwx_update_u_i_event.html#aa25df58e7047f819f5dd0520eb2cc8ea On 6/21/19 3:00 PM, Sylwester Kocjan wrote: > Hi, > > I prepared some changes in KiCad simulation dialog. They are about > additional simulation parameters. According to tutorial, which I've > found on website, I store them on GitHub repository in branch > "Sim_InitialConditions_SK" here: > > https://github.com/skocjan/kicad_initialconditions.git > > Could you please have a look and do some review if possible? I'd be > grateful for feedback if these changes are OK. In future I'd like to > implement OP analysis using some controls I've added > (https://bugs.launchpad.net/kicad/+bug/1821360). > > Additionally I also prepared polish translation for new strings. > > My aim was to make simulation with arbitrary initial conditions > possible (additionally I added other options). Right now it is possible > to enable checkbox UIC on TRAN tab, but there is no possibility to set > IC to any arbitrary value in element properties. I'm afraid it will > involve change in .sch format. Please take note that it is also > possible to define initial conditions for entire nodes instead of > capacitors or inductors (for example: ".ic v(11)=5 v(4)=-5 v(2)=2.2", > see ngspice manual, chapter 15.2.2). It is also a challenge. > > So in current state we can use for initial conditions results of .OP > analysis or default zero. > > From my point of view these are the topics, which should be taken into > account during review: > - in simulation code sometimes there is used '\n', sometimes "\r\n". > Maybe it should be unified? Is there a common definition for newline in > wxWidgets? > - I added .option savecurrents: someone added TODO that it doesn't > work. Maybe should we hide this control? > - something happened to colours on dialog windows, see attached image. > I don't know what is it and how to fix it. seems to be unrelated to my > changes, but maybe I'm wrong? (this is visible on Ubuntu 18.04) > - some feedback regarding how to add IC field to capacitors, inductors > or nodes will be very appreciated ;) > > Limitations of my changes: > - parsing SPICE .options is not implemented (in bool > DIALOG_SIM_SETTINGS::parseCommand( const wxString& aCommand )) > > Best regards, > Sylwek > > > _______________________________________________ > Mailing list: https://launchpad.net/~kicad-developers > Post to : [email protected] > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
