kicad-developers team mailing list archive

[Ian McInerney <Ian.S.McInerney@xxxxxxxx>]

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,
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
> 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
> and branch:
> 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
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>