kicad-developers team mailing list archive

[Jeff Young <jeff@xxxxxxxxx>]

Hi Alex,

No need for apologies.  It’s a steep learning curve.

Some more comments:

1) Dialog items use sentence-capitalization and colons; dialog headers use title-capitalization and no colons.  So “Components:” should be “Components”, “Front side:” should be “Front Side”, etc.

2) Right-align the StdButtonSizer (and give it right and left margins).

3) I don’t think this ever made it into the coding guidelines, but we prefer ctor initializers each on their own line (with the colon on the line above):

        padsType_t( PAD_ATTR_T aAttribute, wxString aTitle ) :
                attribute( aAttribute ), 
		title( aTitle ), 
		qty( 0 )

4) And yes, I would space-align the inspectMenu->AddItem calls that the Git checker messed up.

If you don’t have time to make these fixes just holler and I can do it.

Cheers,
Jeff.


> On 17 Jul 2019, at 12:47, Alexander Shuklin <jasuramme@xxxxxxx> wrote:
> 
> Hi,
> I'm very sorry. I know, that's stupid, but it takes so much time to get myself familiar with kicad code
> If it's not too late, can you look for newer patch, not one I sent this morning? Of course, I can make commit on top of it, but I think it doesn't make sense
> There only few changes, such as
> - code formatting
> - I used   <>FinishDialogSettings() instead of just Fit() as written in KiCad documentation
> - I changed tool name for "pcbnew.InspectionTool.ShowStatisticsDialog" so it would correspond to PCB_INSPECTION_TOOL::ShowStatisticsDialog method
> - And I changed wxString ( _( "some string " ) ) to just _( "some string" )
> That's link to github:
> https://github.com/jasuramme/kicad-source-mirror/commit/7846cc554fa13c27159d6b1ba4693ac96707c559 <https://github.com/jasuramme/kicad-source-mirror/commit/7846cc554fa13c27159d6b1ba4693ac96707c559> 
> and patch uploaded to this mail
> Sorry for that, but I've just understood that, during further doxygen documentation reading
> Среда, 17 июля 2019, 9:01 +03:00 от Alexander Shuklin <jasuramme@xxxxxxx>:
> 
> Hi!
> Sorry for delay, there to much to do for my primary job.
> I hope that code will be alright.
> That's commit on the github:
> https://github.com/jasuramme/kicad-source-mirror/commit/9f9ff463ac679dd79a85a88c9b4156dcec05b337 <https://github.com/jasuramme/kicad-source-mirror/commit/9f9ff463ac679dd79a85a88c9b4156dcec05b337> 
> There's one thing. Git coding standards checkers writes following
> 
> +++ b/pcbnew/menubar_pcb_editor.cpp
> @@ -436,7 +436,7 @@ void PCB_EDIT_FRAME::ReCreateMenuBar()
>  
>      inspectMenu->AddItem( PCB_ACTIONS::listNets,             SELECTION_CONDITIONS::ShowAlways );
>      inspectMenu->AddItem( ACTIONS::measureTool,              SELECTION_CONDITIONS::ShowAlways );
> -    inspectMenu->AddItem( PCB_ACTIONS::boardStatistics,      SELECTION_CONDITIONS::ShowAlways );
> +    inspectMenu->AddItem( PCB_ACTIONS::boardStatistics, SELECTION_CONDITIONS::ShowAlways );
> 
> 
> 
> But I would leave whitespaces, for better code looking as all functions around use it
> 
> 
> 
> Понедельник, 8 июля 2019, 17:03 +03:00 от Jeff Young <jeff@xxxxxxxxx <mailto:jeff@xxxxxxxxx>>:
> 
> Hi Alex,
> 
> Have a look at the second half of DIALOG_GLOBAL_EDIT_TEXT_AND_GRAPHICS::TransferDataToWindow().  That code builds a read-only grid.  You could  then have columns for Top, Bottom, and Total.
> 
> (BTW, I think we use Front/Back nomenclature rather than Top/Bottom.)
> 
> Cheers,
> Jeff.
> 
> 
>> On 8 Jul 2019, at 13:48, Alexander Shuklin <jasuramme@xxxxxxx <http://e.mail.ru/compose/?mailto=mailto%3ajasuramme@xxxxxxx>> wrote:
>> 
>> Hi Jeff,
>> that bug reports are some job to do)
>> 
>> Well, reason why I went up with that top-side/bottom-side checkboxes is if you will want to count let's say SMD components on the bottom side, you can manage it with setting top one off. I'm not sure, how important it is. On the another hand, we could write down something like: 
>> THT components: top: 15, bottom: 32, total: 47
>> SMD components: top: 14, bottom: 31, total: 45
>> But that would be ugly as well.
>> 
>> If there would be more options and more components properties, maybe better to write all data to wxTextCtrl then? Just in big text field, which you could copy. And that will be easy to change and expand.
>> 
>> Понедельник, 8 июля 2019, 14:24 +03:00 от Jeff Young <jeff@xxxxxxxxx <http://e.mail.ru/compose/?mailto=mailto%3ajeff@xxxxxxxxx>>:
>> 
>> Hi Alex,
>> 
>> I don’t like the top-side/bottom-side checkboxes (because I want to see both numbers at once), but I do like the other three.
>> 
>> I think we need to move to a list or grid presentation, though.  One of the other features we’ve talked about is user-defined attributes, so there might be more things than just THT, SMD and Virtual.
>> 
>> Cheers,
>> Jeff.
>> 
>> [1] https://bugs.launchpad.net/kicad/+bug/1789363 <https://bugs.launchpad.net/kicad/+bug/1789363>
>> [2] https://bugs.launchpad.net/kicad/+bug/980919 <https://bugs.launchpad.net/kicad/+bug/980919> 
>> 
>> 
>> 
>>> On 8 Jul 2019, at 11:20, Alexander Shuklin <jasuramme@xxxxxxx <>> wrote:
>>> 
>>> Hi!, I added stuff for footprints THT,SMD,virtual properties and now it's calculate area.
>>> But for me dialog is not very straight forward now. Can you please advice, what better to do?
>>> I attached 2 pics. I think, maybe right way is to add few checkboxes which will change dialog data as soon as you pressed them?
>>> Just look at pics and say what would be better, as I'm in doubt.
>>> Second one is in WxFormBuilder so it looks a bit different.
>>> 
>>> 
>>> Пятница, 5 июля 2019, 0:05 +03:00 от Jeff Young <jeff@xxxxxxxxx <>>:
>>> 
>>> I agree that it shouldn’t be its own tool, but PCB_EDITOR_CONTROL is getting too big.
>>> 
>>> Let’s just name this tool the PCB_INSPECTION_TOOL, and then I’ll move the highlight and list nets stuff, and the DRC dialog to it later.  That’ll nicely parallel the EE_INSPECTION_TOOL.
>>> 
>>> Cheers,
>>> Jeff.
>>> 
>>>> On 4 Jul 2019, at 21:56, Ian McInerney <Ian.S.McInerney@xxxxxxxx <>> wrote:
>>>> 
>>>> This looks nice, but a few comments from my initial usage of it:
>>>> 
>>>> Usability:
>>>> 1) When I use english units, your statistics dialog gives the dimensions in mils. It should probably give those in inches instead, since that is what the grid panel at the bottom gives.
>>>> 2) Including the board area would also be useful, definitely the area of the bounding box, and possibly the area of the actual PCB (but that is more complicated). I know of several board houses that charge based on area, so this could provide a quick way to estimate price for the user.
>>>> 3) I am not sure I am a fan of the icon used for the menu item, since this dialog focuses on board statistics rather than footprint statistics and that icon is used mainly for footprint actions. Could a variant of the pcbnew icon be used?
>>>> 
>>>> 
>>>> While the code as you gave works, I think the following changes could be beneficial:
>>>> 1) It seems that this action could just be added to PCB_EDITOR_CONTROL as the action "pcbnew.Control.BoardStatistics" rather than creating a new tool just for this.
>>>> 2) I think the launching of the window could be simplified if you used a modal window instead (see https://docs.wxwidgets.org/3.0/classwx_dialog.html <https://docs.wxwidgets.org/3.0/classwx_dialog.html>, the Modal and Modeless section). Then you don't have to keep track of the window pointer, and could actually just make launching this a single function instead of an entire class (this would go nicely with point #1).
>>>> 3) The function to start the window should be TransferDataToWindow, not TransferDataFromWindow. FromWindow is called when the window is destroyed and ToWindow is called when it is created. Also, it is called automatically when the window is started, so there is no need to manually call it.
>>>> 
>>>> 
>>>> Other code parts:
>>>> 1) There are some lines that are too long in dialog_board_statistics.cpp (max 100 characters per line)
>>>> 2) There are a few whitespace errors when applying it:
>>>> Applying: added board statistics dialog, which shows info for production and assembly
>>>> /home/imcinerney/Downloads/0001-added-board-statistics-dialog-which-shows-info-for-p.patch:158: trailing whitespace.
>>>>                 /* if pin has drill with width==0 and height==0, we 
>>>> /home/imcinerney/Downloads/0001-added-board-statistics-dialog-which-shows-info-for-p.patch:1612: new blank line at EOF.
>>>> +
>>>> warning: 2 lines add whitespace errors.
>>>> 
>>>> Overall though it looks pretty nice.
>>>> 
>>>> -Ian
>>>> 
>>>> On Thu, Jul 4, 2019 at 3:07 PM Шуклин Александр <jasuramme@xxxxxxx <>> wrote:
>>>> Hi, that's first time I try to contribute to KiCad and write to Launchpad mailing lists, so please, don't beat me to hard )))
>>>> I really miss some board statistic dialog, where you can see quantity of SMD pads, THT pads, board dimensions, all the stuff, you need for PCB production and assembly. There was also issue in the bug tracker 
>>>> https://bugs.launchpad.net/kicad/+bug/1817232 <https://bugs.launchpad.net/kicad/+bug/1817232>
>>>> And like guy from bug issue, I moved from Altium Designer and miss that dialog as well. 
>>>> Can you please look at that and commit if you think it's useful or tell me what to change.
>>>> That's my commit in the github:
>>>> https://github.com/jasuramme/kicad-source-mirror/commit/6290375c1d41ddb89d4b08067593f170c7d344c5 <https://github.com/jasuramme/kicad-source-mirror/commit/6290375c1d41ddb89d4b08067593f170c7d344c5>
>>>> and branch:
>>>> https://github.com/jasuramme/kicad-source-mirror/tree/statistic_dialog <https://github.com/jasuramme/kicad-source-mirror/tree/statistic_dialog>
>>>> and there's also patch and dialogs pics in the attachment.
>>>> 
>>>> _______________________________________________
>>>> Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <>
>>>> Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>>> More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
>>>> _______________________________________________
>>>> Mailing list: https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <>
>>>> Unsubscribe : https://launchpad.net/~kicad-developers <https://launchpad.net/~kicad-developers>
>>>> More help   : https://help.launchpad.net/ListHelp <https://help.launchpad.net/ListHelp>
>>> 
>>> 
>>> 
>>> -- 
>>> Alexander Shuklin
>>> <1.png><2.png>
>> 
>> 
>> 
>> -- 
>> Alexander Shuklin
> 
> 
> 
> -- 
> Alexander Shuklin
> 
> 
> -- 
> Alexander Shuklin
> <0001-added-board-statistics-dialog-which-shows-info-for-p.patch>