Merge into product : enhanced-highcontrast : Code : KiCad

Status:	Merged
Merge reported by:	Dick Hollenbeck
Merged at revision:	not available
Proposed branch:	lp:~mangelajo/kicad/enhanced-highcontrast
Merge into:	lp:kicad/product
Diff against target:	160 lines (+36/-5) 6 files modified include/gr_basic.h (+1/-0) pcbnew/class_drawsegment.cpp (+12/-0) pcbnew/class_edge_mod.cpp (+14/-0) pcbnew/class_pad_draw_functions.cpp (+7/-3) pcbnew/class_track.cpp (+1/-1) pcbnew/tracepcb.cpp (+1/-1)
To merge this branch:	bzr merge lp:~mangelajo/kicad/enhanced-highcontrast
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
jean-pierre charras	2012-02-25	Pending
Dick Hollenbeck	2012-02-24	Pending
Review via email: mp+94593@code.launchpad.net

Description of the change

Now the high contrast mode will gray out the module edges, and other elements to
make visualization much nicer in high contrast.

Also we added a new draw mode bit, allowing us to pass down when we wanted to allow
the high contrast mode: for example in the module browser, we don't pass such bit, so
the the modules are drawn in full detail.

Revision history for this message

Miguel Angel Ajo (mangelajo) wrote on 2012-02-25:

#

Please review the implementation, not sure if adding the extra bit in gr_basic.h for draw_type is the best solution. At least I thinked of many options, and it was less code-intrusive implementation I found to know at draw time if the high contrast drawing could be done in a certain window.

The idea is not to do high contrast operations on the library viewer, which was odd.

Revision history for this message

Miguel Angel Ajo (mangelajo) wrote on 2012-02-25:

#

May be it makes sense to take the chance to refactor the code, and instead of passing down a draw_mode, passing down a structure with the "drawing context", which could contain

1) a pointer to the DisplayOptions,
2) current display layer ,
3) if we want high contrast mode in this mode...

We would get rid of a lot of dependencies in the bottom..

Cheers,

Revision history for this message

Dick Hollenbeck (dickelbeck) wrote on 2012-02-27:

#

On 02/25/2012 06:34 AM, Miguel Angel Ajo wrote:
> May be it makes sense to take the chance to refactor the code, and instead of passing down a draw_mode, passing down a structure with the "drawing context", which could contain
>
> 1) a pointer to the DisplayOptions,
> 2) current display layer ,
> 3) if we want high contrast mode in this mode...
>
> We would get rid of a lot of dependencies in the bottom..
>
> Cheers,

Old news, google the mailing list for "class PAINTER".

Revision history for this message

Miguel Angel Ajo (mangelajo) wrote on 2012-02-27:

#

A) for "PAINTER class":
I like design you're talking about. I has many benefits, and it may have
really little impact in the code. So +1 for it.

Do you keep using the IRC?, any channel for devel, or just #kicad/freenode?

B) About the high contrast change, what my code does is this: (see the
pictures).

https://lists.launchpad.net/kicad-developers/msg07541.html

It addresses a couple of things:

1) The module edges, and other objects didn't gray out when you where
routing in the HC mode, which could be very anoying
if you had a lot.

2) Some objects where wrayed out (pads, etc, -now mod edges too-) when you
went into library editor or library viewer. Which looks undesired (at least
to me), since the HC mode it's useful for routing, and you loose access to
HC on/off buttons or hotkeys from that part of the interface.
I fixed that with the extra bit of draw mode, but that impacts the top
code, which I didn't really like, but it looked as the cleanest path.

Other details:

3) Now that I read it again, I'm not sure if the draw_segment class may be
included on this or not. What's a segment object exactly?,

4) About my politeness: May be I should have asked in the list before
going to change this. It's ok if finally it isn't considered for merging as
it's being helpful to me to get more familiar with KiCad's code design and
the project workflows. Anyway, I find this way much easier to route, and
may be if there is no agreement on what people like, may be just some
settings on the "display options" would do for everybody :-)

5) Do you prefer the launchpad branch/merge workflow, or just sending a
patch to the list?, not that I recall the kicad's coding style guide I see
that a patch may be preferred, but It could have been written before going
into launchpad/bzr

Cheers, and thanks for your patience ;)

2012/2/27 Dick Hollenbeck <email address hidden>

> On 02/25/2012 06:34 AM, Miguel Angel Ajo wrote:
> > May be it makes sense to take the chance to refactor the code, and
> instead of passing down a draw_mode, passing down a structure with the
> "drawing context", which could contain
> >
> > 1) a pointer to the DisplayOptions,
> > 2) current display layer ,
> > 3) if we want high contrast mode in this mode...
> >
> > We would get rid of a lot of dependencies in the bottom..
> >
> > Cheers,
>
> Old news, google the mailing list for "class PAINTER".
>
>
>
>
> --
>
> https://code.launchpad.net/~miguelangel-r/kicad/enhanced-highcontrast/+merge/94593
> You are the owner of lp:~miguelangel-r/kicad/enhanced-highcontrast.
>

--

Miguel Angel Ajo Pelayo
http://www.nbee.es
+34 636 52 25 69
skype: ajoajoajo

A) for "PAINTER class":
I like design you're talking about. I has many benefits, and it may have
really little impact in the code. So +1 for it.

Do you keep using the IRC?, any channel for devel, or just #kicad/freenode?

B) About the high contrast change, what my code does is this: (see the
pictures).

https://lists.launchpad.net/kicad-developers/msg07541.html

It addresses a couple of things:

1)  The module edges, and other objects didn't gray out when you where
routing in the HC mode, which could be very anoying
if you had a lot.

2) Some objects where wrayed out (pads, etc, -now mod edges too-) when you
went into library editor or library viewer. Which looks undesired (at least
to me), since the HC mode it's useful for routing, and you loose access to
HC on/off buttons or hotkeys from that part of the interface.
     I fixed that with the extra bit of draw mode, but that impacts the top
code, which I didn't really like, but it looked as the cleanest path.

Other details:

3) Now that I read it again, I'm not sure if the draw_segment class may be
included on this or not. What's a segment object exactly?,

4) About my politeness: May be I should have asked in the list before
going to change this. It's ok if finally it isn't considered for merging as
it's being helpful to me to get more familiar with KiCad's code design and
the project workflows. Anyway, I find this way much easier to route, and
may be if there is no agreement on what people like, may be just some
settings on the "display options" would do for everybody :-)

5) Do you prefer the launchpad branch/merge workflow, or just sending a
patch to the list?, not that I recall the kicad's coding style guide I see
that a patch may be preferred, but It could have been written before going
into launchpad/bzr

Cheers, and thanks for your patience ;)

2012/2/27 Dick Hollenbeck <dick@softplc.com>

> On 02/25/2012 06:34 AM, Miguel Angel Ajo wrote:
> > May be it makes sense to take the chance to refactor the code, and
> instead of passing down a draw_mode, passing down a structure with the
> "drawing context", which could contain
> >
> > 1) a pointer to the DisplayOptions,
> > 2) current display layer ,
> > 3) if we want high contrast mode in this mode...
> >
> > We would get rid of a lot of dependencies in the bottom..
> >
> > Cheers,
>
> Old news, google the mailing list for "class PAINTER".
>
>
>
>
> --
>
> https://code.launchpad.net/~miguelangel-r/kicad/enhanced-highcontrast/+merge/94593
> You are the owner of lp:~miguelangel-r/kicad/enhanced-highcontrast.
>

--

Miguel Angel Ajo Pelayo
http://www.nbee.es
+34 636 52 25 69
skype: ajoajoajo

Revision history for this message

Dick Hollenbeck (dickelbeck) wrote on 2012-02-27:

#

Download full text (4.4 KiB)

On 02/27/2012 01:28 AM, Miguel Angel Ajo wrote:
> A) for "PAINTER class":
> I like design you're talking about. I has many benefits, and it may have
> really little impact in the code. So +1 for it.

It dramatically impacts the code. I measure impact by what is *replicated* or propagated
throughout the body of the source.
class PAINTER would be a dramatic change, but one that can be done in parallel with
working existing Draw() functions until a final switch over is possible, after which you
go back and kill off numerous graphical_object::Draw() functions.

Remember PAINTER is a replaceable *compile time* interface. Meaning there is no reason
for polymorphic call routing.

You implement one or more PAINTERs, one for KiCad GAL, probably one for wxDC, and choose
at compile time which is used. Polymorphism does not enter into it. Which of the
graphical interfaces are in use should be hidden, not propagated throughout the code.

For the wxDC implementation (if any), you would end up copying the BOARD_ITEM::Draw()
functions down into the PAINTER::Draw() function, thus hiding them behind a much simpler
interface.

After they are working, you could then go back and delete BOARD_ITEM::Draw() functions.
This is a smooth non-disruptive migration path, very much like what I am in process of
doing with KICAD_PLUGIN::{Load,Save}.

> Do you keep using the IRC?, any channel for devel, or just #kicad/freenode?

Tom of CERN thinks we should use Jabber, and have specific meeting times. To set this up,
we need to create a Jabber home, and agree on an agenda and a meeting time.
I cannot see myself trolling on an IRC channel day and night, so to that extend, I trust
Tom's Jabber suggestion to be workable.

Tom recommends Pidgin as a Jabber client. Somebody has to:

a) create a Jabber home for us.
b) establish an agenda for our first meeting
c) publish a meeting time.

>
> B) About the high contrast change, what my code does is this: (see the
> pictures).

I will try and look at it this week. I use freerouter for routing, so the weaknesses of
PCBNEW in terms of routing are no longer even visible to me, since I don't use it for
routing. PCBNEW failed me on a 6 layer board and I wrote the SPECCTRA round tripping
bridge and never looked back.

This is a problem if we want to bring it PCBNEW up to snuff, so this "routing improvement"
work is necessary, and perhaps bloody. But it cannot be considered complete, perhaps
ever, but especially not until there is push and shove routing in there. Obviously the
current best solution for free routing using KiCad is freerouting. But somebody has to
pay the price of making it be PCBNEW, so thanks for your work.

I would suggest you use the merge request for now, sometimes folks just get busy, and
looking at patches can be time consuming. You may need to be patient.

> https://lists.launchpad.net/kicad-developers/msg07541.html
>
> It addresses a couple of things:
>
> 1) The module edges, and other objects didn't gray out when you where
> routing in the HC mode, which could be very anoying
> if you had a lot.
>
> 2) Some objects where wrayed out (pads, etc, -now mod edges too-) when you
> went into library edi...

On 02/27/2012 01:28 AM, Miguel Angel Ajo wrote:
> A) for "PAINTER class":
> I like design you're talking about. I has many benefits, and it may have
> really little impact in the code. So +1 for it.

It dramatically impacts the code.  I measure impact by what is *replicated* or propagated
throughout the body of the source.
class PAINTER would be a dramatic change, but one that can be done in parallel with
working existing Draw() functions until a final switch over is possible, after which you
go back and kill off numerous graphical_object::Draw() functions.

Remember PAINTER is a replaceable *compile time* interface.  Meaning there is no reason
for polymorphic call routing.

You implement one or more PAINTERs, one for KiCad GAL, probably one for wxDC, and choose
at compile time which is used.  Polymorphism does not enter into it.  Which of the
graphical interfaces are in use should be hidden, not propagated throughout the code.

For the wxDC implementation (if any), you would end up copying the BOARD_ITEM::Draw()
functions down into the PAINTER::Draw() function, thus hiding them behind a much simpler
interface.

After they are working, you could then go back and delete BOARD_ITEM::Draw() functions. 
This is a smooth non-disruptive migration path, very much like what I am in process of
doing with KICAD_PLUGIN::{Load,Save}.

> Do you keep using the IRC?, any channel for devel, or just #kicad/freenode?

Tom of CERN thinks we should use Jabber, and have specific meeting times.  To set this up,
we need to create a Jabber home, and agree on an agenda and a meeting time.
I cannot see myself trolling on an IRC channel day and night, so to that extend, I trust
Tom's Jabber suggestion to be workable.

Tom recommends Pidgin as a Jabber client.  Somebody has to:

a) create a Jabber home for us.
b) establish an agenda for our first meeting
c) publish a meeting time.

>
> B) About the high contrast change, what my code does is this: (see the
> pictures).

I will try and look at it this week.  I use freerouter for routing, so the weaknesses of
PCBNEW in terms of routing are no longer even visible to me, since I don't use it for
routing.  PCBNEW failed me on a 6 layer board and I wrote the SPECCTRA round tripping
bridge and never looked back.

This is a problem if we want to bring it PCBNEW up to snuff, so this "routing improvement"
work is necessary, and perhaps bloody.  But it cannot be considered complete, perhaps
ever, but especially not until there is push and shove routing in there.  Obviously the
current best solution for free routing using KiCad is freerouting.  But somebody has to
pay the price of making it be PCBNEW, so thanks for your work.

I would suggest you use the merge request for now, sometimes folks just get busy, and
looking at patches can be time consuming.  You may need to be patient.

> https://lists.launchpad.net/kicad-developers/msg07541.html
>
> It addresses a couple of things:
>
> 1)  The module edges, and other objects didn't gray out when you where
> routing in the HC mode, which could be very anoying
> if you had a lot.
>
> 2) Some objects where wrayed out (pads, etc, -now mod edges too-) when you
> went into library editor or library viewer. Which looks undesired (at least
> to me), since the HC mode it's useful for routing, and you loose access to
> HC on/off buttons or hotkeys from that part of the interface.
>      I fixed that with the extra bit of draw mode, but that impacts the top
> code, which I didn't really like, but it looked as the cleanest path.
>
>
> Other details:
>
>  3) Now that I read it again, I'm not sure if the draw_segment class may be
> included on this or not. What's a segment object exactly?,

Use Doxygen and answer your own question.

>  4) About my politeness: May be I should have asked in the list before
> going to change this. It's ok if finally it isn't considered for merging as
> it's being helpful to me to get more familiar with KiCad's code design and
> the project workflows. Anyway, I find this way much easier to route, and
> may be if there is no agreement on what people like, may be just some
> settings on the "display options" would do for everybody :-)

Are you talking now about SWIG or high contrast?

>  5) Do you prefer the launchpad branch/merge workflow, or just sending a
> patch to the list?, not that I recall the kicad's coding style guide I see
> that a patch may be preferred, but It could have been written before going
> into launchpad/bzr

1	=== modified file 'include/gr_basic.h'
2	--- include/gr_basic.h 2012-01-23 04:33:36 +0000
3	+++ include/gr_basic.h 2012-02-24 17:59:24 +0000
4	@@ -41,6 +41,7 @@
5	#define GR_AND 0x04000000
6	#define GR_NXOR 0x08000000
7	#define GR_INVERT 0x10000000
8	+#define GR_ALLOW_HIGHCONTRAST 0x20000000
9
10	#define GR_HIGHLIGHT 0x80000000
11
12
13	=== modified file 'pcbnew/class_drawsegment.cpp'
14	--- pcbnew/class_drawsegment.cpp 2012-01-23 04:33:36 +0000
15	+++ pcbnew/class_drawsegment.cpp 2012-02-24 17:59:24 +0000
16	@@ -34,6 +34,7 @@
17	#include <gr_basic.h>
18	#include <bezier_curves.h>
19	#include <class_drawpanel.h>
20	+#include <class_pcb_screen.h>
21	#include <kicad_string.h>
22	#include <colors_selection.h>
23	#include <trigo.h>
24	@@ -189,6 +190,7 @@
25	int l_trace;
26	int color, mode;
27	int radius;
28	+ int curr_layer = ( (PCB_SCREEN*) panel->GetScreen() )->m_Active_Layer;
29
30	BOARD * brd = GetBoard( );
31
32	@@ -197,6 +199,16 @@
33
34	color = brd->GetLayerColor( GetLayer() );
35
36	+ if( ( draw_mode & GR_ALLOW_HIGHCONTRAST ) && DisplayOpt.ContrastModeDisplay )
37	+ {
38	+ if( !IsOnLayer( curr_layer ) )
39	+ {
40	+ color &= ~MASKCOLOR;
41	+ color \|= DARKDARKGRAY;
42	+ }
43	+ }
44	+
45	+
46	GRSetDrawMode( DC, draw_mode );
47	l_trace = m_Width >> 1; /* half trace width */
48
49
50	=== modified file 'pcbnew/class_edge_mod.cpp'
51	--- pcbnew/class_edge_mod.cpp 2012-01-23 04:33:36 +0000
52	+++ pcbnew/class_edge_mod.cpp 2012-02-24 17:59:24 +0000
53	@@ -33,6 +33,7 @@
54	#include <wxstruct.h>
55	#include <trigo.h>
56	#include <class_drawpanel.h>
57	+#include <class_pcb_screen.h>
58	#include <confirm.h>
59	#include <kicad_string.h>
60	#include <colors_selection.h>
61	@@ -45,6 +46,7 @@
62	#include <class_module.h>
63	#include <class_edge_mod.h>
64
65	+#include <stdio.h>
66
67	EDGE_MODULE::EDGE_MODULE( MODULE* parent, STROKE_T aShape ) :
68	DRAWSEGMENT( parent, PCB_MODULE_EDGE_T )
69	@@ -97,12 +99,14 @@
70	int ux0, uy0, dx, dy, radius, StAngle, EndAngle;
71	int color, type_trace;
72	int typeaff;
73	+ int curr_layer = ( (PCB_SCREEN*) panel->GetScreen() )->m_Active_Layer;
74	PCB_BASE_FRAME* frame;
75	MODULE* module = (MODULE*) m_Parent;
76
77	if( module == NULL )
78	return;
79
80	+
81	BOARD * brd = GetBoard( );
82
83	if( brd->IsLayerVisible( m_Layer ) == false )
84	@@ -110,6 +114,16 @@
85
86	color = brd->GetLayerColor( m_Layer );
87
88	+ if(( draw_mode & GR_ALLOW_HIGHCONTRAST ) && DisplayOpt.ContrastModeDisplay )
89	+ {
90	+ if( !IsOnLayer( curr_layer ) )
91	+ {
92	+ color &= ~MASKCOLOR;
93	+ color \|= DARKDARKGRAY;
94	+ }
95	+ }
96	+
97	+
98	frame = (PCB_BASE_FRAME*) panel->GetParent();
99
100	type_trace = m_Shape;
101
102	=== modified file 'pcbnew/class_pad_draw_functions.cpp'
103	--- pcbnew/class_pad_draw_functions.cpp 2012-02-19 04:02:19 +0000
104	+++ pcbnew/class_pad_draw_functions.cpp 2012-02-24 17:59:24 +0000
105	@@ -234,7 +234,9 @@
106	}
107
108	// if PAD_SMD pad and high contrast mode
109	- if( ( GetAttribute() == PAD_SMD \|\| GetAttribute() == PAD_CONN ) && DisplayOpt.ContrastModeDisplay )
110	+ if( ( aDraw_mode & GR_ALLOW_HIGHCONTRAST ) &&
111	+ ( GetAttribute() == PAD_SMD \|\| GetAttribute() == PAD_CONN ) &&
112	+ DisplayOpt.ContrastModeDisplay )
113	{
114	// when routing tracks
115	if( frame && frame->GetToolId() == ID_TRACK_BUTT )
116	@@ -301,7 +303,8 @@
117	// if Contrast mode is ON and a technical layer active, show pads on this
118	// layer so we can see pads on paste or solder layer and the size of the
119	// mask
120	- if( DisplayOpt.ContrastModeDisplay && screen->m_Active_Layer > LAST_COPPER_LAYER )
121	+ if( ( aDraw_mode & GR_ALLOW_HIGHCONTRAST ) &&
122	+ DisplayOpt.ContrastModeDisplay && screen->m_Active_Layer > LAST_COPPER_LAYER )
123	{
124	if( IsOnLayer( screen->m_Active_Layer ) )
125	{
126	@@ -373,7 +376,8 @@
127
128	// Display net names is restricted to pads that are on the active layer
129	// in hight contrast mode display
130	- if( !IsOnLayer( screen->m_Active_Layer ) && DisplayOpt.ContrastModeDisplay )
131	+ if( ( aDraw_mode & GR_ALLOW_HIGHCONTRAST ) &&
132	+ !IsOnLayer( screen->m_Active_Layer ) && DisplayOpt.ContrastModeDisplay )
133	drawInfo.m_Display_netname = false;
134
135	DrawShape( aPanel->GetClipBox(), aDC, drawInfo );
136
137	=== modified file 'pcbnew/class_track.cpp'
138	--- pcbnew/class_track.cpp 2012-02-10 22:26:42 +0000
139	+++ pcbnew/class_track.cpp 2012-02-24 17:59:24 +0000
140	@@ -604,7 +604,7 @@
141	return;
142	#endif
143
144	- if( DisplayOpt.ContrastModeDisplay )
145	+ if( ( draw_mode & GR_ALLOW_HIGHCONTRAST ) && DisplayOpt.ContrastModeDisplay )
146	{
147	if( !IsOnLayer( curr_layer ) )
148	{
149
150	=== modified file 'pcbnew/tracepcb.cpp'
151	--- pcbnew/tracepcb.cpp 2012-02-19 04:02:19 +0000
152	+++ pcbnew/tracepcb.cpp 2012-02-24 17:59:24 +0000
153	@@ -109,7 +109,7 @@
154
155	TraceWorkSheet( DC, GetScreen(), g_DrawDefaultLineThickness );
156
157	- GetBoard()->Draw( m_canvas, DC, GR_OR );
158	+ GetBoard()->Draw( m_canvas, DC, GR_OR \| GR_ALLOW_HIGHCONTRAST);
159
160	DrawGeneralRatsnest( DC );
161

KiCad

Merge lp:~mangelajo/kicad/enhanced-highcontrast into lp:kicad/product

Commit message

Description of the change

Preview Diff

Subscribers