kicad-developers team mailing list archive

[jp charras <jp.charras@xxxxxxxxxx>]

Le 14/02/2014 17:14, Henner Zeller a écrit :
>> I committed your patch with 3 very minor changes:
> 
> Thanks for the review!
> 
>>
>> 1 - On wxWidgets 3.0.0, dialog_choose_component.cpp does not compile
>>     (tree.GetNext() function does not exist).
>>     fixedGetPrevVisible is compiled only if wxWidgets version is <
>> 3.0.0, and GetPrevVisible is used for version >= 3.0.0.
>>     Please, verify this point.
> 
> Looks good.
> In the meantime, I have changed this slightly: Turns out that choosing the
> next or previous _visible_ element sometimes is a problem if the item
> is partially
> obstructed; so sometimes the cursor stops scrolling through if at the edge.
> 
> See patch below. I think this should compile on all versions.
> 
>>
>> 2 - The dialog is now derived from DIALOG_SHIM, like all our dialogs.
>>     The advantage (among some others) is the position and size are
>> stored during the session.
> 
> Ah, cool. I was wondering how to do that :)
> 
>>
>> 3 - Comments in .h files are now tagged to be taken in account by
>> doxygen. (previously, They were fine for doxygen, but not tagged)
>>
>>
>>
>> I need a refinement:
>> When a component exists in more than one library, currently Eeschema
>> (like some other EDA tools) loads the first found (The order of
>> libraries is therefore important).
>>
>> There are both advantage and inconvenient to do that.
>>
>> Therefore, because the dialog allows the user to select a given lib,the
>> actual loaded component is (in rare cases) not the selected component
>> (if it comes from an other lib)
> 
> I think we can fix that, but actually returning the LIB_COMPONENT* or some
> fully qualified name (like lib:part). Internally, I already work with
> the LIB_COMPONENT,
> but then when returning, I just return the name to fit into the
> existing infrastructure.
> 
> If we do the LIB_COMPONENT thing, then the history would need to store
> these instead
> of strings (but not sure if the pointers to LIB_COMPONENTS can change
> over the life-time of
> the application ?);

They can easily change, if you modify/delete ... these components with
libedit or if you add/remove a lib from lib list.

 so maybe better with fully qualified names.
> I also noticed that the '-cache' libraries do have the same component
> names, but the
> entries are different; will have to take that into account.
> 
> That is on the 'return side'.
> On the 'display side', I could probably make sure that there are two
> components that
> look identical and show them next to each other so that the user can choose.
> 
> In general, I want to make the scoring of 'local libraries' slightly
> higher than 'global libraries',
> so that the user first sees their own things (which I guess would be
> more expected).

Currently one cannot choose the library. The first component found is
always loaded.

Changing this behavior is a very complicated thing.
Just add the library name like lib:component fixes this issue, but
creates a *lot* of other issues.

Just think about components with multiple units per package, and what
happen if each unit come from different libs (components with multiple
units per package are always a tricky case).
Or what happen if you move a component from a lib to an other lib (which
is a common case).

Currently, using a full qualified name will breaks Eeschema, because it
was not designed for that.

> 
> Anyway, will have a look over the weekend and try to come up with a
> good solution.
> 
>>
>> I am thinking it could be good to warn the user if the loaded component
>> comes from a different lib than the lib which is selected.
>>
>> (Note: this issue was already existing in the previous selection dialog)
>>
>> Thanks.
> 
> I do have a little followup-patch with some more improvments wrt. the
> styleguide and the
> tree navigation change mentioned above:
> 
> Commit message:
>     o Better naming for private struct: public fields uppercase.
>     o make some more fields 'const' that can.
>     o Instead of previous/next _visible_ element, Go through
>       previous and next element. Otherwise the cursor stops moving
>       if the item is only partially visible.
> 
> View:
>   https://github.com/hzeller/kicad/compare/master...followup-patch-component-choose
> 
> Download:
>   https://github.com/hzeller/kicad/compare/master...followup-patch-component-choose.diff

Committed. Thanks.

-- 
Jean-Pierre CHARRAS