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 _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp

