kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #30098
Re: [PATCH] correct text inside two importantplot windows
Generally single entry controls will contain a colon in the label but
entire sections will not - thus "Output directory" and "Default line
thickness (mm)" should have colons, but "Messages" and "Page Size"
should not. This is because the single entry controls read as a single
line of text, where it's correct to separate a label with a colon,
whereas the section headings read as standalone labels that sit apart
from the thing they label.
On Fri, Jul 21, 2017 at 05:41:59PM +0200, Fabrizio Tappero wrote:
> Hi Wayne,
>
> this is an exmaple of the current plot schematic window.
>
> [image: Inline image 1]
>
>
> I think it is expected that "Output directory:" and "Default line thickness
> (mm):" and "Messages:" have a colon.
>
> Am I correct?
>
> cheers
> Fabrizio
>
>
>
> On Fri, Jul 21, 2017 at 4:23 PM, Wayne Stambaugh <stambaughw@xxxxxxxxx>
> wrote:
>
> > Fabrizio,
> >
> > I pushed your patch to the master branch. Thank you for your
> > contribution to KiCad.
> >
> > This is not aimed at your patch but one thing I notice is that there is
> > and inconsistency in some of our wxStaticBoxSizer() strings. There are
> > a few cases where the string ends with a colon (:) but it's rare. For
> > the sake of consistency, if you find any cases in the future where this
> > is true, please remove the trailing colon.
> >
> > Cheers,
> >
> > Wayne
> >
> >
> > On 7/20/2017 5:59 AM, Fabrizio Tappero wrote:
> > > Hi Wayne,
> > > you are right. Sorry about that. I have reviewed and changed all
> > instances.
> > >
> > > In attachment the new patch.
> > >
> > > cheers
> > > Fabrizio
> > >
> > >
> > > On Wed, Jul 19, 2017 at 3:59 PM, Wayne Stambaugh <stambaughw@xxxxxxxxx
> > > <mailto:stambaughw@xxxxxxxxx>> wrote:
> > >
> > > Fabrizio,
> > >
> > > You are still using header instead of sentence capitalization of text
> > > box labels. For example, in pcbnew/dialogs/dialog_plot_base.cpp you
> > > changed:
> > >
> > > "Plot format:"
> > >
> > > to
> > >
> > > "Plot Format:"
> > >
> > > The original capitalization is correct. I see a lot of this in this
> > > patch.
> > >
> > > Please correct the text box label capitalization and resubmit your
> > > patch. Also, please give your patches a unique name so I don't have
> > to.
> > > Even easier, just use the name generated by `git format-patch`.
> > >
> > > Wayne
> > >
> > > On 7/19/2017 6:55 AM, Fabrizio Tappero wrote:
> > > > Hi Wayne,
> > > > really tedious work to spot these kind of mistakes... I hope I
> > fixed all
> > > > mistakes.
> > > >
> > > > In attachment the new patch.
> > > >
> > > > cheers
> > > > Fabrizio
> > > >
> > > >
> > > > On Wed, Jul 19, 2017 at 12:19 PM, Fabrizio Tappero
> > > > <fabrizio.tappero@xxxxxxxxx <mailto:fabrizio.tappero@xxxxxxxxx>
> > > <mailto:fabrizio.tappero@xxxxxxxxx
> > > <mailto:fabrizio.tappero@xxxxxxxxx>>> wrote:
> > > >
> > > > Hi Wayne,
> > > > apologies for that. I sometimes think that a unique patch is
> > more
> > > > friendly than several ones.
> > > >
> > > > Will fix and resubmit.
> > > >
> > > > cheers
> > > > Fabrizio
> > > >
> > > >
> > > > On Tue, Jul 18, 2017 at 3:35 PM, Wayne Stambaugh
> > > > <stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>
> > > <mailto:stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>>> wrote:
> > > >
> > > > Fabrizio,
> > > >
> > > > This patch has far more changes than the plot dialogs
> > > mentioned
> > > > in your
> > > > commit message. In the future, please either make your
> > commit
> > > > message
> > > > describe all of the changes related to the patch. I would
> > > > prefer that
> > > > you only make one wxFormbuilder dialog change per patch.
> > > > wxFormbuilder
> > > > changes tend to be noisy so it takes longer to review them.
> > > >
> > > > You are using the incorrect capitalization in text box
> > labels.
> > > > Text box
> > > > labels use sentence capitalization not header
> > > capitalization per
> > > > the UI
> > > > guidelines[1].
> > > >
> > > > Please fix your patch accordingly so I can commit it.
> > > >
> > > > Cheers,
> > > >
> > > > Wayne
> > > >
> > > >
> > > [1]:http://docs.kicad-pcb.org/doxygen/md_Documentation_
> > development_ui-policy.html
> > > <http://docs.kicad-pcb.org/doxygen/md_Documentation_
> > development_ui-policy.html>
> > > >
> > > <http://docs.kicad-pcb.org/doxygen/md_Documentation_
> > development_ui-policy.html
> > > <http://docs.kicad-pcb.org/doxygen/md_Documentation_
> > development_ui-policy.html>>
> > > >
> > > > On 7/3/2017 4:45 AM, Fabrizio Tappero wrote:
> > > > > Hi JP,
> > > > > thanks for it. I updated my wxformbuilder to the latest
> > > (3.6v)
> > > > > version. And I fixed the (mm) issue you mentioned.
> > > > >
> > > > > In attachment the patch.
> > > > >
> > > > > thank you
> > > > > Fabrizio
> > > > >
> > > > >
> > > > > On Sat, Jul 1, 2017 at 8:40 PM, jp charras
> > > > <jp.charras@xxxxxxxxxx <mailto:jp.charras@xxxxxxxxxx>
> > > <mailto:jp.charras@xxxxxxxxxx <mailto:jp.charras@xxxxxxxxxx>>>
> > wrote:
> > > > >> Le 30/06/2017 à 15:34, Fabrizio Tappero a écrit :
> > > > >>> Hello,
> > > > >>> in attachment the patch reviewed after Wayne comments.
> > > > >>>
> > > > >>> Best regards
> > > > >>> Fabrizio
> > > > >>>
> > > > >>
> > > > >> Hi Fabrizio,
> > > > >>
> > > > >> I had a look at your last patch, and found a few issues:
> > > > >> for instance the change:
> > > > >>
> > > > >> - m_defaultLineWidthTitle = new wxStaticText(
> > > > sbSizerPlotFormat->GetStaticBox(), wxID_ANY,
> > > > >> _("Default line thickness"), wxDefaultPosition,
> > > > wxDefaultSize, 0 );
> > > > >> + m_defaultLineWidthTitle = new wxStaticText(
> > > > sbSizerPlotFormat->GetStaticBox(), wxID_ANY, _("Line
> > > > >> Thickness (mm):"), wxDefaultPosition, wxDefaultSize, 0
> > );
> > > > >>
> > > > >>
> > > > >> is not good for 2 reasons:
> > > > >> - "Default line thickness" is the right title (see a
> > > previous
> > > > Wayne's answer)
> > > > >> - "Line Thickness (mm):" is incorrect:
> > > > >> the units symbol " (mm):" is added a run time, because
> > > units
> > > > can be mm or inches.
> > > > >> so never change titles like "Value" to "Value (mm):".
> > > > >> It creates a wrong title.
> > > > >>
> > > > >> Besides your wxFormBuilder version is rather old and
> > > creates
> > > > incorrect C++ sources: please update it.
> > > > >>
> > > > >> Thanks.
> > > > >>
> > > > >> --
> > > > >> Jean-Pierre CHARRAS
> > > > >>
> > > > >>
> > > > >> _______________________________________________
> > > > >> Mailing list: https://launchpad.net/~kicad-developers
> > > <https://launchpad.net/~kicad-developers>
> > > > <https://launchpad.net/~kicad-developers
> > > <https://launchpad.net/~kicad-developers>>
> > > > >> Post to : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> > > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx
> > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
> > > > >> Unsubscribe : https://launchpad.net/~kicad-developers
> > > <https://launchpad.net/~kicad-developers>
> > > > <https://launchpad.net/~kicad-developers
> > > <https://launchpad.net/~kicad-developers>>
> > > > >> More help : https://help.launchpad.net/ListHelp
> > > <https://help.launchpad.net/ListHelp>
> > > > <https://help.launchpad.net/ListHelp
> > > <https://help.launchpad.net/ListHelp>>
> > > >
> > > > _______________________________________________
> > > > Mailing list: https://launchpad.net/~kicad-developers
> > > <https://launchpad.net/~kicad-developers>
> > > > <https://launchpad.net/~kicad-developers
> > > <https://launchpad.net/~kicad-developers>>
> > > > Post to : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> > > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx
> > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
> > > > Unsubscribe : https://launchpad.net/~kicad-developers
> > > <https://launchpad.net/~kicad-developers>
> > > > <https://launchpad.net/~kicad-developers
> > > <https://launchpad.net/~kicad-developers>>
> > > > More help : https://help.launchpad.net/ListHelp
> > > <https://help.launchpad.net/ListHelp>
> > > > <https://help.launchpad.net/ListHelp
> > > <https://help.launchpad.net/ListHelp>>
> > > >
> > > >
> > > >
> > >
> > >
> >
> >
Follow ups
References
-
Re: [PATCH] correct text inside two importantplot windows
From: Fabrizio Tappero, 2017-06-30
-
Re: [PATCH] correct text inside two importantplot windows
From: jp charras, 2017-07-01
-
Re: [PATCH] correct text inside two importantplot windows
From: Fabrizio Tappero, 2017-07-03
-
Re: [PATCH] correct text inside two importantplot windows
From: Wayne Stambaugh, 2017-07-18
-
Re: [PATCH] correct text inside two importantplot windows
From: Fabrizio Tappero, 2017-07-19
-
Re: [PATCH] correct text inside two importantplot windows
From: Fabrizio Tappero, 2017-07-19
-
Re: [PATCH] correct text inside two importantplot windows
From: Wayne Stambaugh, 2017-07-19
-
Re: [PATCH] correct text inside two importantplot windows
From: Fabrizio Tappero, 2017-07-20
-
Re: [PATCH] correct text inside two importantplot windows
From: Wayne Stambaugh, 2017-07-21
-
Re: [PATCH] correct text inside two importantplot windows
From: Fabrizio Tappero, 2017-07-21