kicad-developers team mailing list archive

[Henner Zeller <h.zeller@xxxxxxx>]

> 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 ?); 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).

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

-h

>
> --
> Jean-Pierre CHARRAS