Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Wayne Stambaugh]

John,

On 5/6/2019 8:11 AM, John Beard wrote:
> On 03/05/2019 19:28, Wayne Stambaugh wrote:
>>
>> I'm guessing John used a later version of clang-format when he wrote the
>> commit hooks.  John, any ideas how to fix this or do we force devs to
>> use clang-format > 3.8?
> 
> I'm on Arch, so I have quite recent clang-format (8.0.0). This
> particular option has always been in the _clang-format, and seems the
> config was introduced in clang-format 3.9.
> 
> I don't really have any great idea to fix this in general. I think the
> options are:
> 
> * Tell people to use 3.9 or later (actually I don't know what options we
> have need what versions). Most distros will allow people to install
> newer toolchains (sometimes need to enable the updates/backports repos).
> I think clang 4 is available in Mint 18/Xenial, and 5 and 6 are in
> Xenial-updates.
> 
> * Provide multiple style files, suitable for older clangs. Then the git
> hook can feed the right one to clang-format. In this case, you might
> find formatting differences if people use older clangs.
> 
> * Remove any options that don't suit clang 3.8 (our de facto minimum
> version) and deal with the misformattings:
> 
> In this case: formatting clang-format <= 3.8 (BreakStringLiteral not
> available, so "default", which is "true"):
> 
>     std::string var =
>     "Lorem ipsum dolor sit amet, consectetur adipiscing elit, "
>     "sed do eiusmod tempor incididunt ut labore et dolore magna "
>     "aliqua";
> 
> Current _clang-format (BreakStringLiteral=false) with clang-format >= 3.9:
> 
>     std::string var =
>     "Lorem ipsum dolor sit amet, consectetur adipiscing elit,
> sed do eiusmod tempor incididunt ut labore et dolore magna aliqua";
> 
> I suspect this happens little enough that we can deal with it in any
> case. People always need to be aware that you cannot blindly apply the
> formatter anyway. `git add -p` is the way to selectively apply
> formatting changes.
> 
> @Reese, could you give it a go with clang-4 and see if there are any
> more broken options?
> 
> @Wayne, any preference for how we deal with it?

I'm fine with asking users to update clang-format.  Even if they are not
interested updating, breaking long strings is fairly uncommon and we can
just ask patch submitters to break the string as needed.

Wayne

> 
> Cheers,
> 
> John
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[John Beard]

On 06/05/2019 22:12, Wayne Stambaugh wrote:

On 5/6/2019 5:11 PM, John Beard wrote:

I still suggest to change it to false be default and allow developers to
manually align when they want (and then override the formatter). Then at
least the default behaviour is a valid formatting choice.


That works for me.


Here's a quick patch to make the _clang-format change and make it clear 
in the style guide that the column aligned method is still permitted 
when better for readability. The current switch formatting example is 
already formatted in multi-line style, so that's covered implicitly.


Cheers,

John
>From 7f2ae3c674030c1b22ecbadb9b70149d0e3e64ec Mon Sep 17 00:00:00 2001
From: John Beard 
Date: Mon, 6 May 2019 22:23:51 +0100
Subject: [PATCH] Format: Default to switch cases on separate lines by default

Currently, the format enforces single lines when possible, but does
not enforce readable column-based alignment (and, moreover, *removes*
such manually added alignment:

switch( m_orientation )
{
case PIN_RIGHT: m_orientation = PIN_UP; break;
case PIN_UP: m_orientation = PIN_LEFT; break;
}

Change this to multi-line by default:

switch( m_orientation )
{
case PIN_RIGHT:
m_orientation = PIN_UP;
break;
case PIN_UP:
m_orientation = PIN_LEFT;
break;
}

If the developer wishes for column-aligned single-line cases, this
is permitted, but much be done manually:

switch( m_orientation )
{
case PIN_RIGHT: m_orientation = PIN_DOWN;  break;
case PIN_UP:m_orientation = PIN_RIGHT; break;
}

CHANGE: the _clang-format file to reflect this, and add note about
manual override in the dev docs.
---
 Documentation/development/coding-style-policy.md | 14 ++
 _clang-format|  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/development/coding-style-policy.md b/Documentation/development/coding-style-policy.md
index 8836f6a27..1be6fd6f1 100644
--- a/Documentation/development/coding-style-policy.md
+++ b/Documentation/development/coding-style-policy.md
@@ -467,6 +467,20 @@ The case statement is to be indented to the same level as the switch.
 }
 ~
 
+It is permitted to place all cases on a single line each, if that makes the
+code more readable. This is often done for look-ups or translation functions. In
+this case, you will have to manually align for readability as appropriate and
+reject clang-format's suggested changes, if you use it:
+
+~{.cpp}
+switch( m_orientation )
+{
+case PIN_RIGHT: m_orientation = PIN_UP;break;
+case PIN_UP:m_orientation = PIN_LEFT;  break;
+case PIN_LEFT:  m_orientation = PIN_DOWN;  break;
+case PIN_DOWN:  m_orientation = PIN_RIGHT; break;
+}
+~
 
 # 5. License Statement # {#license_statement}
 There is a the file copyright.h which you can copy into the top of
diff --git a/_clang-format b/_clang-format
index 1a4179264..413c8b571 100644
--- a/_clang-format
+++ b/_clang-format
@@ -7,7 +7,7 @@ AlignOperands: true
 AlignTrailingComments: true
 AllowAllParametersOfDeclarationOnNextLine: true
 AllowShortBlocksOnASingleLine: false
-AllowShortCaseLabelsOnASingleLine: true
+AllowShortCaseLabelsOnASingleLine: false
 AllowShortFunctionsOnASingleLine: false
 AllowShortIfStatementsOnASingleLine: false
 AllowShortLoopsOnASingleLine: false
-- 
2.21.0

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Wayne Stambaugh]

Looks good John, feel free to push the change.

Thanks,

Wayne

On 5/6/2019 5:35 PM, John Beard wrote:
> On 06/05/2019 22:12, Wayne Stambaugh wrote:
>> On 5/6/2019 5:11 PM, John Beard wrote:
>>> I still suggest to change it to false be default and allow developers to
>>> manually align when they want (and then override the formatter). Then at
>>> least the default behaviour is a valid formatting choice.
>>
>> That works for me.
> 
> Here's a quick patch to make the _clang-format change and make it clear
> in the style guide that the column aligned method is still permitted
> when better for readability. The current switch formatting example is
> already formatted in multi-line style, so that's covered implicitly.
> 
> Cheers,
> 
> John

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Wayne Stambaugh]

John,

On 5/6/2019 5:11 PM, John Beard wrote:
> 
> 
> On 06/05/2019 21:46, Wayne Stambaugh wrote:
>> John,
>>
>> On 5/6/19 4:09 PM, John Beard wrote:
>>> On 06/05/2019 17:51, Reece R. Pollack wrote:
 John,

 I've already jumped to clang-format 6.0, which is one of the optional
 installs for Mint 18. That works, once you get all the symlinks fixed,
>>>
>>> Good to know, thanks for the update.
>>>
 except it keeps wanting to reformat my switch statements like this,
 which is contrary to the KiCad coding standards:

 @@ -148,15 +130,9 @@ int PCB_ORIGIN_TRANSFORM_Y_ABS::FromDisplay( int
 aValue ) const
    case ORIGIN_REFERENCE_PAGE:
    // No-op
    break;
 -    case ORIGIN_REFERENCE_AUX:
 -    origin = m_PcbBaseFrame->GetAuxOrigin().y;
 -    break;
 -    case ORIGIN_REFERENCE_GRID:
 -    origin = m_PcbBaseFrame->GetGridOrigin().y;
 -    break;
 -    default:
 -    wxASSERT(false);
 -    break;
 +    case ORIGIN_REFERENCE_AUX: origin =
 m_PcbBaseFrame->GetAuxOrigin().y; break;
 +    case ORIGIN_REFERENCE_GRID: origin =
 m_PcbBaseFrame->GetGridOrigin().y; break;
 +    default: wxASSERT( false ); break;
    }

    // Invert the direction if needed
>>>
>>> This is a weird style that I don't personally like, and IMO goes against
>>> the style implied by our "spacious" bracing and newline-before-if
>>> policies. But there are quite some uses of it. It's enforced by this
>>> line:
>>>
>>>  AllowShortCaseLabelsOnASingleLine: true
>>>
>>> It's the only line-condensing option we have set:
>>>
>>>  AllowShortBlocksOnASingleLine: false
>>>  AllowShortCaseLabelsOnASingleLine: true # the only true here
>>>  AllowShortFunctionsOnASingleLine: false
>>>  AllowShortIfStatementsOnASingleLine: false
>>>  AllowShortLoopsOnASingleLine: false
>>>
>>> If the formatter is proposing a change that goes against the existing
>>> style in the code area in question, I do not think it's controversial to
>>> ignore its suggestion.
>>>
>>> @Wayne, what do you think: is this enforcement representative of the
>>> right style? Should we change AllowShortCaseLabelsOnASingleLine to
>>> false? (+1 from me).
>>
>> I would rather not have clang-format force this change.  I am OK if the
>> developer prefers this for short case labels but if they prefer a
>> separate line that's fine.  I prefer everything on more than one line
>> but if I'm changing code that is already formatted on a single line, I
>> don't change it.  We didn't specify this in the coding policy so it's a
>> don't care but I would ask you to change it if you your case labels were
>> complex and you had them on a single line.
> 
> Fair enough - gratuitous formatting changes in the same commit as
> functional changes are a sin anyway!
> 
> Remember, (git-)clang-format will only change what you change, it
> shouldn't forcibly change formatting in general unless you tell it to.
> Users still need to ensure it looks sane and only accept good changes.
> 
> Having AllowShortCaseLabelsOnASingleLine = true means that clang-format
> will *enforce* the all-on-a-line policy when it can, but it will not
> enforce column alignment (and will actually collapse manual column
> alignment). So basically, the formatter is unlikely to ever give a good
> result for a short-case switch as it stands.
> 
> I still suggest to change it to false be default and allow developers to
> manually align when they want (and then override the formatter). Then at
> least the default behaviour is a valid formatting choice.
> 
> Cheers,
> 
> John

That works for me.

Thanks,

Wayne

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[John Beard]

On 06/05/2019 21:46, Wayne Stambaugh wrote:

John,

On 5/6/19 4:09 PM, John Beard wrote:

On 06/05/2019 17:51, Reece R. Pollack wrote:

John,

I've already jumped to clang-format 6.0, which is one of the optional
installs for Mint 18. That works, once you get all the symlinks fixed,


Good to know, thanks for the update.


except it keeps wanting to reformat my switch statements like this,
which is contrary to the KiCad coding standards:

@@ -148,15 +130,9 @@ int PCB_ORIGIN_TRANSFORM_Y_ABS::FromDisplay( int
aValue ) const
   case ORIGIN_REFERENCE_PAGE:
   // No-op
   break;
-    case ORIGIN_REFERENCE_AUX:
-    origin = m_PcbBaseFrame->GetAuxOrigin().y;
-    break;
-    case ORIGIN_REFERENCE_GRID:
-    origin = m_PcbBaseFrame->GetGridOrigin().y;
-    break;
-    default:
-    wxASSERT(false);
-    break;
+    case ORIGIN_REFERENCE_AUX: origin =
m_PcbBaseFrame->GetAuxOrigin().y; break;
+    case ORIGIN_REFERENCE_GRID: origin =
m_PcbBaseFrame->GetGridOrigin().y; break;
+    default: wxASSERT( false ); break;
   }

   // Invert the direction if needed


This is a weird style that I don't personally like, and IMO goes against
the style implied by our "spacious" bracing and newline-before-if
policies. But there are quite some uses of it. It's enforced by this line:

     AllowShortCaseLabelsOnASingleLine: true

It's the only line-condensing option we have set:

     AllowShortBlocksOnASingleLine: false
     AllowShortCaseLabelsOnASingleLine: true # the only true here
     AllowShortFunctionsOnASingleLine: false
     AllowShortIfStatementsOnASingleLine: false
     AllowShortLoopsOnASingleLine: false

If the formatter is proposing a change that goes against the existing
style in the code area in question, I do not think it's controversial to
ignore its suggestion.

@Wayne, what do you think: is this enforcement representative of the
right style? Should we change AllowShortCaseLabelsOnASingleLine to
false? (+1 from me).


I would rather not have clang-format force this change.  I am OK if the
developer prefers this for short case labels but if they prefer a
separate line that's fine.  I prefer everything on more than one line
but if I'm changing code that is already formatted on a single line, I
don't change it.  We didn't specify this in the coding policy so it's a
don't care but I would ask you to change it if you your case labels were
complex and you had them on a single line.


Fair enough - gratuitous formatting changes in the same commit as 
functional changes are a sin anyway!


Remember, (git-)clang-format will only change what you change, it 
shouldn't forcibly change formatting in general unless you tell it to. 
Users still need to ensure it looks sane and only accept good changes.


Having AllowShortCaseLabelsOnASingleLine = true means that clang-format 
will *enforce* the all-on-a-line policy when it can, but it will not 
enforce column alignment (and will actually collapse manual column 
alignment). So basically, the formatter is unlikely to ever give a good 
result for a short-case switch as it stands.


I still suggest to change it to false be default and allow developers to 
manually align when they want (and then override the formatter). Then at 
least the default behaviour is a valid formatting choice.


Cheers,

John

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Jeff Young]

We do use (and encourage) single-line cases when it’s more readable that way.  
This particularly comes up when mapping or converting something, such as:
switch( aFaceId )
{
case FACE_SCH:  name = KIFACE_PREFIX "eeschema";break;
case FACE_PCB:  name = KIFACE_PREFIX "pcbnew";  break;
case FACE_CVPCB:name = KIFACE_PREFIX "cvpcb";   break;
case FACE_GERBVIEW: name = KIFACE_PREFIX "gerbview";break;
case FACE_PL_EDITOR:name = KIFACE_PREFIX "pl_editor";   break;
case FACE_PCB_CALCULATOR:   name = KIFACE_PREFIX "pcb_calculator";  break;
case FACE_BMP2CMP:  name = KIFACE_PREFIX "bitmap2component";break;

default:
wxASSERT_MSG( 0, wxT( "caller has a bug, passed a bad aFaceId" ) );
return wxEmptyString;
}

or:

switch( aType )
{
case SCH_LABEL_T:newtext = new SCH_LABEL( position, txt );break;
case SCH_GLOBAL_LABEL_T: newtext = new SCH_GLOBALLABEL( position, txt );  break;
case SCH_HIER_LABEL_T:   newtext = new SCH_HIERLABEL( position, txt );break;
case SCH_TEXT_T: newtext = new SCH_TEXT( position, txt ); break;

default:
wxASSERT_MSG( false, wxString::Format( "Invalid text type: %d.", aType ) );
return;
}

or:

if( aRotateCCW )
{
switch( m_orientation )
{
case PIN_RIGHT: m_orientation = PIN_UP;break;
case PIN_UP:m_orientation = PIN_LEFT;  break;
case PIN_LEFT:  m_orientation = PIN_DOWN;  break;
case PIN_DOWN:  m_orientation = PIN_RIGHT; break;
}
}
else
{
switch( m_orientation )
{
case PIN_RIGHT: m_orientation = PIN_DOWN;  break;
case PIN_UP:m_orientation = PIN_RIGHT; break;
case PIN_LEFT:  m_orientation = PIN_UP;break;
case PIN_DOWN:  m_orientation = PIN_LEFT;  break;
}
}

Cheers,
Jeff.


> On 6 May 2019, at 21:09, John Beard  wrote:
> 
> On 06/05/2019 17:51, Reece R. Pollack wrote:
>> John,
>> I've already jumped to clang-format 6.0, which is one of the optional 
>> installs for Mint 18. That works, once you get all the symlinks fixed, 
> 
> Good to know, thanks for the update.
> 
>> except it keeps wanting to reformat my switch statements like this, which is 
>> contrary to the KiCad coding standards:
>> @@ -148,15 +130,9 @@ int PCB_ORIGIN_TRANSFORM_Y_ABS::FromDisplay( int aValue 
>> ) const
>>  case ORIGIN_REFERENCE_PAGE:
>>  // No-op
>>  break;
>> -case ORIGIN_REFERENCE_AUX:
>> -origin = m_PcbBaseFrame->GetAuxOrigin().y;
>> -break;
>> -case ORIGIN_REFERENCE_GRID:
>> -origin = m_PcbBaseFrame->GetGridOrigin().y;
>> -break;
>> -default:
>> -wxASSERT(false);
>> -break;
>> +case ORIGIN_REFERENCE_AUX: origin = m_PcbBaseFrame->GetAuxOrigin().y; 
>> break;
>> +case ORIGIN_REFERENCE_GRID: origin = m_PcbBaseFrame->GetGridOrigin().y; 
>> break;
>> +default: wxASSERT( false ); break;
>>  }
>>  // Invert the direction if needed
> 
> This is a weird style that I don't personally like, and IMO goes against the 
> style implied by our "spacious" bracing and newline-before-if policies. But 
> there are quite some uses of it. It's enforced by this line:
> 
>AllowShortCaseLabelsOnASingleLine: true
> 
> It's the only line-condensing option we have set:
> 
>AllowShortBlocksOnASingleLine: false
>AllowShortCaseLabelsOnASingleLine: true # the only true here
>AllowShortFunctionsOnASingleLine: false
>AllowShortIfStatementsOnASingleLine: false
>AllowShortLoopsOnASingleLine: false
> 
> If the formatter is proposing a change that goes against the existing style 
> in the code area in question, I do not think it's controversial to ignore its 
> suggestion.
> 
> @Wayne, what do you think: is this enforcement representative of the right 
> style? Should we change AllowShortCaseLabelsOnASingleLine to false? (+1 from 
> me).
> 
> Cheers,
> 
> John
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Wayne Stambaugh]

John,

On 5/6/19 4:09 PM, John Beard wrote:
> On 06/05/2019 17:51, Reece R. Pollack wrote:
>> John,
>>
>> I've already jumped to clang-format 6.0, which is one of the optional
>> installs for Mint 18. That works, once you get all the symlinks fixed, 
> 
> Good to know, thanks for the update.
> 
>> except it keeps wanting to reformat my switch statements like this,
>> which is contrary to the KiCad coding standards:
>>
>> @@ -148,15 +130,9 @@ int PCB_ORIGIN_TRANSFORM_Y_ABS::FromDisplay( int
>> aValue ) const
>>   case ORIGIN_REFERENCE_PAGE:
>>   // No-op
>>   break;
>> -    case ORIGIN_REFERENCE_AUX:
>> -    origin = m_PcbBaseFrame->GetAuxOrigin().y;
>> -    break;
>> -    case ORIGIN_REFERENCE_GRID:
>> -    origin = m_PcbBaseFrame->GetGridOrigin().y;
>> -    break;
>> -    default:
>> -    wxASSERT(false);
>> -    break;
>> +    case ORIGIN_REFERENCE_AUX: origin =
>> m_PcbBaseFrame->GetAuxOrigin().y; break;
>> +    case ORIGIN_REFERENCE_GRID: origin =
>> m_PcbBaseFrame->GetGridOrigin().y; break;
>> +    default: wxASSERT( false ); break;
>>   }
>>
>>   // Invert the direction if needed
> 
> This is a weird style that I don't personally like, and IMO goes against
> the style implied by our "spacious" bracing and newline-before-if
> policies. But there are quite some uses of it. It's enforced by this line:
> 
>     AllowShortCaseLabelsOnASingleLine: true
> 
> It's the only line-condensing option we have set:
> 
>     AllowShortBlocksOnASingleLine: false
>     AllowShortCaseLabelsOnASingleLine: true # the only true here
>     AllowShortFunctionsOnASingleLine: false
>     AllowShortIfStatementsOnASingleLine: false
>     AllowShortLoopsOnASingleLine: false
> 
> If the formatter is proposing a change that goes against the existing
> style in the code area in question, I do not think it's controversial to
> ignore its suggestion.
> 
> @Wayne, what do you think: is this enforcement representative of the
> right style? Should we change AllowShortCaseLabelsOnASingleLine to
> false? (+1 from me).

I would rather not have clang-format force this change.  I am OK if the
developer prefers this for short case labels but if they prefer a
separate line that's fine.  I prefer everything on more than one line
but if I'm changing code that is already formatted on a single line, I
don't change it.  We didn't specify this in the coding policy so it's a
don't care but I would ask you to change it if you your case labels were
complex and you had them on a single line.

Wayne

> 
> Cheers,
> 
> John
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[John Beard]

On 06/05/2019 17:51, Reece R. Pollack wrote:

John,

I've already jumped to clang-format 6.0, which is one of the optional 
installs for Mint 18. That works, once you get all the symlinks fixed, 


Good to know, thanks for the update.

except it keeps wanting to reformat my switch statements like this, 
which is contrary to the KiCad coding standards:


@@ -148,15 +130,9 @@ int PCB_ORIGIN_TRANSFORM_Y_ABS::FromDisplay( int 
aValue ) const

  case ORIGIN_REFERENCE_PAGE:
  // No-op
  break;
-case ORIGIN_REFERENCE_AUX:
-origin = m_PcbBaseFrame->GetAuxOrigin().y;
-break;
-case ORIGIN_REFERENCE_GRID:
-origin = m_PcbBaseFrame->GetGridOrigin().y;
-break;
-default:
-wxASSERT(false);
-break;
+case ORIGIN_REFERENCE_AUX: origin = 
m_PcbBaseFrame->GetAuxOrigin().y; break;
+case ORIGIN_REFERENCE_GRID: origin = 
m_PcbBaseFrame->GetGridOrigin().y; break;

+default: wxASSERT( false ); break;
  }

  // Invert the direction if needed


This is a weird style that I don't personally like, and IMO goes against 
the style implied by our "spacious" bracing and newline-before-if 
policies. But there are quite some uses of it. It's enforced by this line:


AllowShortCaseLabelsOnASingleLine: true

It's the only line-condensing option we have set:

AllowShortBlocksOnASingleLine: false
AllowShortCaseLabelsOnASingleLine: true # the only true here
AllowShortFunctionsOnASingleLine: false
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false

If the formatter is proposing a change that goes against the existing 
style in the code area in question, I do not think it's controversial to 
ignore its suggestion.


@Wayne, what do you think: is this enforcement representative of the 
right style? Should we change AllowShortCaseLabelsOnASingleLine to 
false? (+1 from me).


Cheers,

John

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Reece R. Pollack]

John,

I've already jumped to clang-format 6.0, which is one of the optional installs 
for Mint 18. That works, once you get all the symlinks fixed, except it keeps 
wanting to reformat my switch statements like this, which is contrary to the 
KiCad coding standards:

@@ -148,15 +130,9 @@ int PCB_ORIGIN_TRANSFORM_Y_ABS::FromDisplay( int aValue ) 
const
 case ORIGIN_REFERENCE_PAGE:
 // No-op
 break;
-    case ORIGIN_REFERENCE_AUX:
-    origin = m_PcbBaseFrame->GetAuxOrigin().y;
-    break;
-    case ORIGIN_REFERENCE_GRID:
-    origin = m_PcbBaseFrame->GetGridOrigin().y;
-    break;
-    default:
-    wxASSERT(false);
-    break;
+    case ORIGIN_REFERENCE_AUX: origin = m_PcbBaseFrame->GetAuxOrigin().y; 
break;
+    case ORIGIN_REFERENCE_GRID: origin = m_PcbBaseFrame->GetGridOrigin().y; 
break;
+    default: wxASSERT( false ); break;
 }

 // Invert the direction if needed



On 5/6/19 8:11 AM, John Beard wrote:

On 03/05/2019 19:28, Wayne Stambaugh wrote:


I'm guessing John used a later version of clang-format when he wrote the
commit hooks.  John, any ideas how to fix this or do we force devs to
use clang-format > 3.8?


I'm on Arch, so I have quite recent clang-format (8.0.0). This particular 
option has always been in the _clang-format, and seems the config was 
introduced in clang-format 3.9.

I don't really have any great idea to fix this in general. I think the options 
are:

* Tell people to use 3.9 or later (actually I don't know what options we have 
need what versions). Most distros will allow people to install newer toolchains 
(sometimes need to enable the updates/backports repos). I think clang 4 is 
available in Mint 18/Xenial, and 5 and 6 are in Xenial-updates.

* Provide multiple style files, suitable for older clangs. Then the git hook 
can feed the right one to clang-format. In this case, you might find formatting 
differences if people use older clangs.

* Remove any options that don't suit clang 3.8 (our de facto minimum version) 
and deal with the misformattings:

In this case: formatting clang-format <= 3.8 (BreakStringLiteral not available, so 
"default", which is "true"):

    std::string var =
    "Lorem ipsum dolor sit amet, consectetur adipiscing elit, "
    "sed do eiusmod tempor incididunt ut labore et dolore magna "
    "aliqua";

Current _clang-format (BreakStringLiteral=false) with clang-format >= 3.9:

    std::string var =
    "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod 
tempor incididunt ut labore et dolore magna aliqua";

I suspect this happens little enough that we can deal with it in any case. 
People always need to be aware that you cannot blindly apply the formatter 
anyway. `git add -p` is the way to selectively apply formatting changes.

@Reese, could you give it a go with clang-4 and see if there are any more 
broken options?

@Wayne, any preference for how we deal with it?

Cheers,

John

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp



___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Kevin Cozens]

On 2019-05-06 8:11 a.m., John Beard wrote:
* Tell people to use 3.9 or later (actually I don't know what options we 
have need what versions).
I recently found an online clang configuration tool as I'm thinking of 
adding in git hooks to force a developer to follow a coding style we've 
talked about.


The configuation tool I found lets you select which version of clang you 
want to use and then only shows you the valid options for that version.


Have a look at https://zed0.co.uk/clang-format-configurator/

--
Cheers!

Kevin.

http://www.ve3syb.ca/   | "Nerds make the shiny things that
https://www.patreon.com/KevinCozens | distract the mouth-breathers, and
| that's why we're powerful"
Owner of Elecraft K2 #2172  |
#include  | --Chris Hardwick

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[John Beard]

On 03/05/2019 19:28, Wayne Stambaugh wrote:


I'm guessing John used a later version of clang-format when he wrote the
commit hooks.  John, any ideas how to fix this or do we force devs to
use clang-format > 3.8?


I'm on Arch, so I have quite recent clang-format (8.0.0). This 
particular option has always been in the _clang-format, and seems the 
config was introduced in clang-format 3.9.


I don't really have any great idea to fix this in general. I think the 
options are:


* Tell people to use 3.9 or later (actually I don't know what options we 
have need what versions). Most distros will allow people to install 
newer toolchains (sometimes need to enable the updates/backports repos). 
I think clang 4 is available in Mint 18/Xenial, and 5 and 6 are in 
Xenial-updates.


* Provide multiple style files, suitable for older clangs. Then the git 
hook can feed the right one to clang-format. In this case, you might 
find formatting differences if people use older clangs.


* Remove any options that don't suit clang 3.8 (our de facto minimum 
version) and deal with the misformattings:


In this case: formatting clang-format <= 3.8 (BreakStringLiteral not 
available, so "default", which is "true"):


std::string var =
"Lorem ipsum dolor sit amet, consectetur adipiscing elit, "
"sed do eiusmod tempor incididunt ut labore et dolore magna "
"aliqua";

Current _clang-format (BreakStringLiteral=false) with clang-format >= 3.9:

std::string var =
"Lorem ipsum dolor sit amet, consectetur adipiscing elit, 
sed do eiusmod tempor incididunt ut labore et dolore magna aliqua";


I suspect this happens little enough that we can deal with it in any 
case. People always need to be aware that you cannot blindly apply the 
formatter anyway. `git add -p` is the way to selectively apply 
formatting changes.


@Reese, could you give it a go with clang-4 and see if there are any 
more broken options?


@Wayne, any preference for how we deal with it?

Cheers,

John

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Wayne Stambaugh]

Reece,

On 5/3/2019 2:09 PM, Reece R. Pollack wrote:
> Hi Wayne,
> 
> No worries on formatting. As a senior professional software engineer I'm
> used to dealing with formal coding conventions and I'm keeping the web
> page open while I'm writing. I prefer the K style myself, but I'm not
> going to lose sleep over the differences.
> 
> I had a bit of trouble getting the clang-format commit checker to work.
> Mint 18 installs clang-format 3.8 by default, which doesn't support the
> 'BreakStringLiterals' key. It spits out the following message:
> 
> YAML:23:22: error: unknown key 'BreakStringLiterals'
> BreakStringLiterals: false
>  ^
> Error reading /home/reece/MyProjects/KiCad/kicad/_clang-format: Invalid
> argument
> 
> With the invocation of clang-format buried under the git extension
> mechanism this error message is invisible to the user. Worse,
> clang-format then blithely ignores the entire style file and reformats
> the file using its default settings (IndentWidth 2, etc.). The notes
> should probably be updated to note the minimum version of clang-format
> required to handle the supplied _clang-format file.

I'm guessing John used a later version of clang-format when he wrote the
commit hooks.  John, any ideas how to fix this or do we force devs to
use clang-format > 3.8?

> 
> -Reece
> 
> 
> On 5/2/19 7:56 AM, Wayne Stambaugh wrote:
>> Hi Reece,
>>
>> Just a few comments on top of Jeff's reply since this will be your first
>> patch submission:
>>
>> Please follow the coding style policy[1].  It saves a lot of back and
>> forth.  There is also a git commit hook[2] which you can use to verify
>> your coding style is correct.  You will have to have clang-format
>> installed on your system in order to use this feature.
>>
>> When you are ready to submit your patch(es), use `git format-patch` to
>> create the patch(es) and post them on the mailing list for comment.
>>
>> It was great talking with you at KiCon.  Thank you again for your
>> interest in contributing to KiCad.
>>
>> Cheers,
>>
>> Wayne
>>
>> [1]:
>> http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html
>>
>> [2]:
>> http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html#tools
>>
>>
>> On 4/30/19 9:50 PM, Reece R. Pollack wrote:
>>> Inspired by KiCon (and before the high wears off) I'm moving forward
>>> with my project to allow the user to specify the coordinate display
>>> origin in pcbnew. I have this working as patches to the v5.1.2 branch,
>>> but they're hard-coded to use the Aux origin. I'd like some guidance on
>>> how best to code this for eventual inclusion into the v6 development
>>> branch.
>>>
>>> My plan is:
>>>
>>>   1. Create a new class ORIGIN_TRANSFORM. This class maintains the
>>> user's
>>>  configuration and provides functions to perform the transforms
>>>  necessary to convert between the internal coordinate representation
>>>  and the equivalent coordinates relative to the user-selected
>>> origin.
>>>   2. Embed two ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS
>>>  class representing the origin transforms for the X and Y axes.
>>>   3. Create a new tab in the pcbnew "Preferences" to allow the user to
>>>  select the origin (Page [default], Aux, or Grid) and direction
>>> (Left
>>>  or Right [default], Up or Down [default]) for each axis.
>>>   4. Modify the board file format to save and load the user's
>>>  ORIGIN_TRANSFORM configuration. Saving this in the .kicad_pcb file
>>>  allows everyone editing the board to see coordinates reported
>>>  relative to the same origin.
>>>   5. Modify each of the dialog classes that displays object coordinates
>>>  to use the ORIGIN_TRANSFORM objects to display coordinates relative
>>>  to the user-selected origin. When the dialog is closed object
>>>  coordinates are converted back to internal coordinates.
>>>   6. Modify the PCB_BASE_FRAME class to display the absolute cursor
>>>  position relative to the user-selected origin.
>>>
>>> This is structured to allow the UNIT_BINDER class to manage origin
>>> transforms, much like it handles the mm/mil conversions. I would change
>>> the constructor to take a reference to an ORIGIN_TRANSFORM object as an
>>> additional argument; the default value would be a unity-transform whose
>>> conversion functions return the value to be converted unmodified. Those
>>> UNIT_BINDER objects that represent a coordinate value (commonly m_posX
>>> and m_posY) would be constructed with a reference to one of the two
>>> ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS class.
>>>
>>> The problem with this strategy is that KiCad reuses UNIT_BINDER objects
>>> for dissimilar purposes. For example,
>>> DIALOG_GRAPHIC_ITEM_PROPERTIES::m_endX can represent the end point of a
>>> graphic line, for which origin transform is needed. But it can also
>>> represent the radius of a circle, for which origin 

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Reece R. Pollack]

Hi Wayne,

No worries on formatting. As a senior professional software engineer I'm 
used to dealing with formal coding conventions and I'm keeping the web 
page open while I'm writing. I prefer the K style myself, but I'm not 
going to lose sleep over the differences.


I had a bit of trouble getting the clang-format commit checker to work. 
Mint 18 installs clang-format 3.8 by default, which doesn't support the 
'BreakStringLiterals' key. It spits out the following message:


YAML:23:22: error: unknown key 'BreakStringLiterals'
BreakStringLiterals: false
 ^
Error reading /home/reece/MyProjects/KiCad/kicad/_clang-format: Invalid 
argument


With the invocation of clang-format buried under the git extension 
mechanism this error message is invisible to the user. Worse, 
clang-format then blithely ignores the entire style file and reformats 
the file using its default settings (IndentWidth 2, etc.). The notes 
should probably be updated to note the minimum version of clang-format 
required to handle the supplied _clang-format file.


-Reece


On 5/2/19 7:56 AM, Wayne Stambaugh wrote:

Hi Reece,

Just a few comments on top of Jeff's reply since this will be your first
patch submission:

Please follow the coding style policy[1].  It saves a lot of back and
forth.  There is also a git commit hook[2] which you can use to verify
your coding style is correct.  You will have to have clang-format
installed on your system in order to use this feature.

When you are ready to submit your patch(es), use `git format-patch` to
create the patch(es) and post them on the mailing list for comment.

It was great talking with you at KiCon.  Thank you again for your
interest in contributing to KiCad.

Cheers,

Wayne

[1]:
http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html
[2]:
http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html#tools

On 4/30/19 9:50 PM, Reece R. Pollack wrote:

Inspired by KiCon (and before the high wears off) I'm moving forward
with my project to allow the user to specify the coordinate display
origin in pcbnew. I have this working as patches to the v5.1.2 branch,
but they're hard-coded to use the Aux origin. I'd like some guidance on
how best to code this for eventual inclusion into the v6 development branch.

My plan is:

  1. Create a new class ORIGIN_TRANSFORM. This class maintains the user's
 configuration and provides functions to perform the transforms
 necessary to convert between the internal coordinate representation
 and the equivalent coordinates relative to the user-selected origin.
  2. Embed two ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS
 class representing the origin transforms for the X and Y axes.
  3. Create a new tab in the pcbnew "Preferences" to allow the user to
 select the origin (Page [default], Aux, or Grid) and direction (Left
 or Right [default], Up or Down [default]) for each axis.
  4. Modify the board file format to save and load the user's
 ORIGIN_TRANSFORM configuration. Saving this in the .kicad_pcb file
 allows everyone editing the board to see coordinates reported
 relative to the same origin.
  5. Modify each of the dialog classes that displays object coordinates
 to use the ORIGIN_TRANSFORM objects to display coordinates relative
 to the user-selected origin. When the dialog is closed object
 coordinates are converted back to internal coordinates.
  6. Modify the PCB_BASE_FRAME class to display the absolute cursor
 position relative to the user-selected origin.

This is structured to allow the UNIT_BINDER class to manage origin
transforms, much like it handles the mm/mil conversions. I would change
the constructor to take a reference to an ORIGIN_TRANSFORM object as an
additional argument; the default value would be a unity-transform whose
conversion functions return the value to be converted unmodified. Those
UNIT_BINDER objects that represent a coordinate value (commonly m_posX
and m_posY) would be constructed with a reference to one of the two
ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS class.

The problem with this strategy is that KiCad reuses UNIT_BINDER objects
for dissimilar purposes. For example,
DIALOG_GRAPHIC_ITEM_PROPERTIES::m_endX can represent the end point of a
graphic line, for which origin transform is needed. But it can also
represent the radius of a circle, for which origin transform is
inappropriate. Thus UNIT_BINDER needs a method similar to the Show()
function to enable or disable the origin transform depending on how it
is being used. This may argue against pushing this functionality into
UNIT_BINDER.

If I don't modify the UNIT_BINDER class I'll probably code the
ORIGIN_TRANSFORM class to handle wxPoint objects rather than individual
coordinates. Thus there would be only one ORIGIN_TRANSFORM object added
to the BOARD_DESIGN_SETTINGS class. The transform in the dialog class
then 

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Wayne Stambaugh]

Hi Reece,

Just a few comments on top of Jeff's reply since this will be your first
patch submission:

Please follow the coding style policy[1].  It saves a lot of back and
forth.  There is also a git commit hook[2] which you can use to verify
your coding style is correct.  You will have to have clang-format
installed on your system in order to use this feature.

When you are ready to submit your patch(es), use `git format-patch` to
create the patch(es) and post them on the mailing list for comment.

It was great talking with you at KiCon.  Thank you again for your
interest in contributing to KiCad.

Cheers,

Wayne

[1]:
http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html
[2]:
http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html#tools

On 4/30/19 9:50 PM, Reece R. Pollack wrote:
> Inspired by KiCon (and before the high wears off) I'm moving forward
> with my project to allow the user to specify the coordinate display
> origin in pcbnew. I have this working as patches to the v5.1.2 branch,
> but they're hard-coded to use the Aux origin. I'd like some guidance on
> how best to code this for eventual inclusion into the v6 development branch.
> 
> My plan is:
> 
>  1. Create a new class ORIGIN_TRANSFORM. This class maintains the user's
> configuration and provides functions to perform the transforms
> necessary to convert between the internal coordinate representation
> and the equivalent coordinates relative to the user-selected origin.
>  2. Embed two ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS
> class representing the origin transforms for the X and Y axes.
>  3. Create a new tab in the pcbnew "Preferences" to allow the user to
> select the origin (Page [default], Aux, or Grid) and direction (Left
> or Right [default], Up or Down [default]) for each axis.
>  4. Modify the board file format to save and load the user's
> ORIGIN_TRANSFORM configuration. Saving this in the .kicad_pcb file
> allows everyone editing the board to see coordinates reported
> relative to the same origin.
>  5. Modify each of the dialog classes that displays object coordinates
> to use the ORIGIN_TRANSFORM objects to display coordinates relative
> to the user-selected origin. When the dialog is closed object
> coordinates are converted back to internal coordinates.
>  6. Modify the PCB_BASE_FRAME class to display the absolute cursor
> position relative to the user-selected origin.
> 
> This is structured to allow the UNIT_BINDER class to manage origin
> transforms, much like it handles the mm/mil conversions. I would change
> the constructor to take a reference to an ORIGIN_TRANSFORM object as an
> additional argument; the default value would be a unity-transform whose
> conversion functions return the value to be converted unmodified. Those
> UNIT_BINDER objects that represent a coordinate value (commonly m_posX
> and m_posY) would be constructed with a reference to one of the two
> ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS class.
> 
> The problem with this strategy is that KiCad reuses UNIT_BINDER objects
> for dissimilar purposes. For example,
> DIALOG_GRAPHIC_ITEM_PROPERTIES::m_endX can represent the end point of a
> graphic line, for which origin transform is needed. But it can also
> represent the radius of a circle, for which origin transform is
> inappropriate. Thus UNIT_BINDER needs a method similar to the Show()
> function to enable or disable the origin transform depending on how it
> is being used. This may argue against pushing this functionality into
> UNIT_BINDER.
> 
> If I don't modify the UNIT_BINDER class I'll probably code the
> ORIGIN_TRANSFORM class to handle wxPoint objects rather than individual
> coordinates. Thus there would be only one ORIGIN_TRANSFORM object added
> to the BOARD_DESIGN_SETTINGS class. The transform in the dialog class
> then becomes a single call to transform a coordinate pair rather than
> one each for X and Y.
> 
> Thoughts? Comments? Suggestions? Brickbats?
> 
> -Reece
> 
> 
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] Pcbnew display origin transforms for v6

[Jeff Young]

Hi Reece,

You should code internal stuff in VECTOR2I rather than wxPoint.  wxPoint should 
only be used when going to the UI.  (We’re trying to reduce our wxWidgets 
dependencies, although this is very much a work in progress.)

If you do use UNIT_BINDER keep in mind that it’s not yet used in WX_GRID 
(mainly because some work needs to be done so that cells know whether or not 
they hold a numeric value).  If you want to tackle that too, that would be 
awesome.

Cheers,
Jeff.


> On 1 May 2019, at 02:50, Reece R. Pollack  wrote:
> 
> Inspired by KiCon (and before the high wears off) I'm moving forward with my 
> project to allow the user to specify the coordinate display origin in pcbnew. 
> I have this working as patches to the v5.1.2 branch, but they're hard-coded 
> to use the Aux origin. I'd like some guidance on how best to code this for 
> eventual inclusion into the v6 development branch.
> 
> My plan is:
> Create a new class ORIGIN_TRANSFORM. This class maintains the user's 
> configuration and provides functions to perform the transforms necessary to 
> convert between the internal coordinate representation and the equivalent 
> coordinates relative to the user-selected origin.
> Embed two ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS class 
> representing the origin transforms for the X and Y axes.
> Create a new tab in the pcbnew "Preferences" to allow the user to select the 
> origin (Page [default], Aux, or Grid) and direction (Left or Right [default], 
> Up or Down [default]) for each axis. 
> Modify the board file format to save and load the user's ORIGIN_TRANSFORM 
> configuration. Saving this in the .kicad_pcb file allows everyone editing the 
> board to see coordinates reported relative to the same origin.
> Modify each of the dialog classes that displays object coordinates to use the 
> ORIGIN_TRANSFORM objects to display coordinates relative to the user-selected 
> origin. When the dialog is closed object coordinates are converted back to 
> internal coordinates.
> Modify the PCB_BASE_FRAME class to display the absolute cursor position 
> relative to the user-selected origin.
> This is structured to allow the UNIT_BINDER class to manage origin 
> transforms, much like it handles the mm/mil conversions. I would change the 
> constructor to take a reference to an ORIGIN_TRANSFORM object as an 
> additional argument; the default value would be a unity-transform whose 
> conversion functions return the value to be converted unmodified. Those 
> UNIT_BINDER objects that represent a coordinate value (commonly m_posX and 
> m_posY) would be constructed with a reference to one of the two 
> ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS class.
> 
> The problem with this strategy is that KiCad reuses UNIT_BINDER objects for 
> dissimilar purposes. For example, DIALOG_GRAPHIC_ITEM_PROPERTIES::m_endX can 
> represent the end point of a graphic line, for which origin transform is 
> needed. But it can also represent the radius of a circle, for which origin 
> transform is inappropriate. Thus UNIT_BINDER needs a method similar to the 
> Show() function to enable or disable the origin transform depending on how it 
> is being used. This may argue against pushing this functionality into 
> UNIT_BINDER.
> 
> If I don't modify the UNIT_BINDER class I'll probably code the 
> ORIGIN_TRANSFORM class to handle wxPoint objects rather than individual 
> coordinates. Thus there would be only one ORIGIN_TRANSFORM object added to 
> the BOARD_DESIGN_SETTINGS class. The transform in the dialog class then 
> becomes a single call to transform a coordinate pair rather than one each for 
> X and Y.
> Thoughts? Comments? Suggestions? Brickbats?
> -Reece
> 
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

[Kicad-developers] Pcbnew display origin transforms for v6

[Reece R. Pollack]

Inspired by KiCon (and before the high wears off) I'm moving forward 
with my project to allow the user to specify the coordinate display 
origin in pcbnew. I have this working as patches to the v5.1.2 branch, 
but they're hard-coded to use the Aux origin. I'd like some guidance on 
how best to code this for eventual inclusion into the v6 development branch.


My plan is:

1. Create a new class ORIGIN_TRANSFORM. This class maintains the user's
   configuration and provides functions to perform the transforms
   necessary to convert between the internal coordinate representation
   and the equivalent coordinates relative to the user-selected origin.
2. Embed two ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS
   class representing the origin transforms for the X and Y axes.
3. Create a new tab in the pcbnew "Preferences" to allow the user to
   select the origin (Page [default], Aux, or Grid) and direction (Left
   or Right [default], Up or Down [default]) for each axis.
4. Modify the board file format to save and load the user's
   ORIGIN_TRANSFORM configuration. Saving this in the .kicad_pcb file
   allows everyone editing the board to see coordinates reported
   relative to the same origin.
5. Modify each of the dialog classes that displays object coordinates
   to use the ORIGIN_TRANSFORM objects to display coordinates relative
   to the user-selected origin. When the dialog is closed object
   coordinates are converted back to internal coordinates.
6. Modify the PCB_BASE_FRAME class to display the absolute cursor
   position relative to the user-selected origin.

This is structured to allow the UNIT_BINDER class to manage origin 
transforms, much like it handles the mm/mil conversions. I would change 
the constructor to take a reference to an ORIGIN_TRANSFORM object as an 
additional argument; the default value would be a unity-transform whose 
conversion functions return the value to be converted unmodified. Those 
UNIT_BINDER objects that represent a coordinate value (commonly m_posX 
and m_posY) would be constructed with a reference to one of the two 
ORIGIN_TRANSFORM objects in the BOARD_DESIGN_SETTINGS class.


The problem with this strategy is that KiCad reuses UNIT_BINDER objects 
for dissimilar purposes. For example, 
DIALOG_GRAPHIC_ITEM_PROPERTIES::m_endX can represent the end point of a 
graphic line, for which origin transform is needed. But it can also 
represent the radius of a circle, for which origin transform is 
inappropriate. Thus UNIT_BINDER needs a method similar to the Show() 
function to enable or disable the origin transform depending on how it 
is being used. This may argue against pushing this functionality into 
UNIT_BINDER.


If I don't modify the UNIT_BINDER class I'll probably code the 
ORIGIN_TRANSFORM class to handle wxPoint objects rather than individual 
coordinates. Thus there would be only one ORIGIN_TRANSFORM object added 
to the BOARD_DESIGN_SETTINGS class. The transform in the dialog class 
then becomes a single call to transform a coordinate pair rather than 
one each for X and Y.


Thoughts? Comments? Suggestions? Brickbats?

-Reece


___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp