Hi Seth, Yeah, I did try some of those other methods first. But they all let implementation details bleed into the wrong places (COLLECORS_GUIDE instead of SELECTION_TOOL, but the info still doesn’t belong outside of the router).
You are correct that some of the changes in selectPoint are just cleanup and are not required. I almost removed the Wait(TOOL_EVENT) thing entirely as selectPoint is never called with aOnDrag = true. But that seemed like perhaps too much cleanup. ;) However, it’s not true that Wait will get called under different circumstances as the heuristics part isn’t a filter per se, and won’t ever remove everything. Cheers, Jeff. > On 9 Jan 2018, at 20:09, Seth Hillbrand <seth.hillbr...@gmail.com> wrote: > > Jeff- > > I get your reasoning. However, it seems like you could accomplish the same > goal (preventing disambiguation) by using the existing > GENERAL_COLLECTOR::Modules[] for your footprint disambiguation and maybe > modifying the COLLECTORS_GUIDE or Inspect() to get the track disambiguation. > This would reduce the impact of your patch and not create duplicate > functionality. > > In other aspects, the changes to selectPoint do not seem required (correct me > if I'm wrong) and they introduce a subtle bug by triggering a > Wait(TOOL_EVENT) even if the heuristics have removed all items from the > collector. > > Overall, I like the idea. It makes sense to have a post-filter. But I would > worry that this is more invasive than is needed to resolve the issue during > feature-freeze. > > -S > > 2018-01-09 11:36 GMT-08:00 Jeff Young <j...@rokeby.ie > <mailto:j...@rokeby.ie>>: > Hi Seth, > > The test_window creates a SELECTION_TOOL without the rest of kicad behind > it. Since I added methods which SELECTION_TOOL calls, I had to mock them in > the test files. > > GENERAL_COLLECTOR::Modules[] can’t be used because the creator of the > collector (SELECTION_TOOL) shouldn’t know anything about the semantics of the > filter (only the specific action implementation knows that). In the > FootprintFilter case it’s simply enough that you’d nearly be willing to > overlook that, but the other filters are more complicated and required things > like ROUTER_TOOL knowledge, which definitely shouldn’t be leaking into > SELECTION_TOOL. > > Cheers, > Jeff. > > >> On 9 Jan 2018, at 19:26, Seth Hillbrand <seth.hillbr...@gmail.com >> <mailto:seth.hillbr...@gmail.com>> wrote: >> >> Same here. Same errors. Yes, cmaked. >> >> I don't understand why you are changing the test_window link libraries in >> this patch. >> >> Also, why did you decide to write a "FootprintFilter" routine instead of >> using the GENERAL_COLLECTOR::Modules[]? >> >> -S >> >> 2018-01-09 11:05 GMT-08:00 Jeff Young <j...@rokeby.ie >> <mailto:j...@rokeby.ie>>: >> Did you do a CMake? There’s a change in the CMake files…. >> >> > On 9 Jan 2018, at 18:59, kristoffer Ödmark <kristofferodmar...@gmail.com >> > <mailto:kristofferodmar...@gmail.com>> wrote: >> > >> > Hmm, cannot compile, get a lot of undefiner references. Even after nuking >> > the build dir. >> > >> > >> > ../../common/libpcbcommon.a(class_pad.cpp.o): In function >> > `D_PAD::HitTest(EDA_RECT const&, bool, int) const': >> > kicad/pcbnew/class_pad.cpp:982: undefined reference to >> > `TestPointInsidePolygon(wxPoint const*, int, wxPoint const&)' >> > ../../common/libpcbcommon.a(class_zone.cpp.o): In function >> > `ZONE_CONTAINER::Hatch()': >> > kicad/pcbnew/class_zone.cpp:1219: undefined reference to >> > `FindLineSegmentIntersection(double, double, int, int, int, int, double*, >> > double*, double*, double*, double*)' >> > ../../common/libcommon.a(shape_poly_set.cpp.o): In function >> > `SHAPE_POLY_SET::convertToClipper(SHAPE_LINE_CHAIN const&, bool)': >> > kicad/common/geometry/shape_poly_set.cpp:466: undefined reference to >> > `ClipperLib::Orientation(std::vector<ClipperLib::IntPoint, >> > std::allocator<ClipperLib::IntPoint> > const&)' >> > kicad/common/geometry/shape_poly_set.cpp:467: undefined reference to >> > `ClipperLib::ReversePath(std::vector<ClipperLib::IntPoint, >> > std::allocator<ClipperLib::IntPoint> >&)' >> > ../../common/libcommon.a(shape_poly_set.cpp.o): In function >> > `SHAPE_POLY_SET::booleanOp(ClipperLib::ClipType, SHAPE_POLY_SET const&, >> > SHAPE_POLY_SET::POLYGON_MODE)': >> > kicad/common/geometry/shape_poly_set.cpp:489: undefined reference to >> > `ClipperLib::Clipper::Clipper(int)' >> > >> > >> > On 2018-01-09 19:22, Wayne Stambaugh wrote: >> >> Does the patch resolve your issue? >> >> >> >> On 1/9/2018 1:21 PM, kristoffer Ödmark wrote: >> >>> Yes, I can reproduce this with very thick tracks! >> >>> >> >>> >> >>> On 2018-01-09 16:55, Jeff Young wrote: >> >>>> Hi Kristoffer, >> >>>> >> >>>> That’s odd. Did you try it with your mouse pointer directly over the >> >>>> corner? (You may need a reasonably thick track for this to happen. >> >>>> Try something on the order of 2mm.) >> >>>> >> >>>> Without my change the selection disambiguation is run *before* we know >> >>>> it’s a drag action on a simple corner, so the selection_tool thinks it >> >>>> needs to know which of the two segments is to be selected. >> >>>> >> >>>> Cheers, >> >>>> Jeff. >> >>>> >> >>>> >> >>>>> On 9 Jan 2018, at 15:37, Kristoffer Ödmark >> >>>>> <kristofferodmar...@gmail.com <mailto:kristofferodmar...@gmail.com>> >> >>>>> wrote: >> >>>>> >> >>>>> If i understand him correctly that would only be when on a corner, I >> >>>>> think it would be the desired behaviour. >> >>>>> >> >>>>> Although, when I try it on my build on my laptop, there is no clarify >> >>>>> selection menu popping up when trying do drag ( 'd' ) or such on >> >>>>> corners. Not in opengl anywas. So I dont know. >> >>>>> >> >>>>> Application: kicad >> >>>>> Version: (2017-12-30 revision b55eb0b5f)-master, release build >> >>>>> Libraries: >> >>>>> wxWidgets 3.0.3 >> >>>>> libcurl/7.57.0 OpenSSL/1.1.0g zlib/1.2.11 libidn2/2.0.4 >> >>>>> libpsl/0.19.1 (+libidn2/2.0.4) libssh2/1.8.0 nghttp2/1.29.0 >> >>>>> Platform: Linux 4.14.12-1-MANJARO x86_64, 64 bit, Little endian, wxGTK >> >>>>> Build Info: >> >>>>> wxWidgets: 3.0.3 (wchar_t,wx containers,compatible with 2.8) GTK+ >> >>>>> 2.24 >> >>>>> Boost: 1.65.1 >> >>>>> Curl: 7.57.0 >> >>>>> Compiler: GCC 7.2.1 with C++ ABI 1011 >> >>>>> >> >>>>> Build settings: >> >>>>> USE_WX_GRAPHICS_CONTEXT=OFF >> >>>>> USE_WX_OVERLAY=OFF >> >>>>> KICAD_SCRIPTING=ON >> >>>>> KICAD_SCRIPTING_MODULES=ON >> >>>>> KICAD_SCRIPTING_WXPYTHON=ON >> >>>>> KICAD_SCRIPTING_ACTION_MENU=ON >> >>>>> BUILD_GITHUB_PLUGIN=ON >> >>>>> KICAD_USE_OCE=ON >> >>>>> KICAD_SPICE=ON >> >>>>> >> >>>>> >> >>>>> -Kristoffer >> >>>>> >> >>>>> On 01/09/2018 04:27 PM, Wayne Stambaugh wrote: >> >>>>>> Jeff, >> >>>>>> Have actually confirmed that this is the desired behavior for this >> >>>>>> outside of you own objectives? I'm not saying that this is or isn't a >> >>>>>> good idea but I personally don't drag trace corners around so I'm not >> >>>>>> sure what the appropriate behavior should be. You should get comments >> >>>>>> from the dev list and users before you make a change like this. As >> >>>>>> far >> >>>>>> as pushing this to the dev repo, if it's not too invasive I will >> >>>>>> consider it. If it is a large change set, I would prefer that we hold >> >>>>>> off until after the stable release. >> >>>>>> Thanks, >> >>>>>> Wayne >> >>>>>> On 1/8/2018 5:49 AM, Jeff Young wrote: >> >>>>>>> Wayne, if I could get you to don that old project manager’s hat one >> >>>>>>> more time: >> >>>>>>> >> >>>>>>> If we’re still weeks out from declaring an RC, I wanted to make one >> >>>>>>> more plug for getting rid of the Clarify Selection dialog when >> >>>>>>> dragging corners or using ‘U’ or ‘I’ over a corner[1]. >> >>>>>>> >> >>>>>>> While it’s marked Wishlist, it seriously impacts productivity when >> >>>>>>> editing tracks, and I think most users would consider it a bug >> >>>>>>> (particularly in the corner case when dragging the corner is >> >>>>>>> clearly moving both the tracks listed in the Clarify Selection menu). >> >>>>>>> >> >>>>>>> I’ve been running the patch for about a week now with no issues. >> >>>>>>> >> >>>>>>> Cheers, >> >>>>>>> Jeff. >> >>>>>>> >> >>>>>>> [1] https://bugs.launchpad.net/kicad/+bug/1708869 >> >>>>>>> <https://bugs.launchpad.net/kicad/+bug/1708869> >> >>>>>>> _______________________________________________ >> >>>>>>> Mailing list: https://launchpad.net/~kicad-developers >> >>>>>>> <https://launchpad.net/~kicad-developers> >> >>>>>>> Post to : kicad-developers@lists.launchpad.net >> >>>>>>> <mailto:kicad-developers@lists.launchpad.net> >> >>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >> >>>>>>> <https://launchpad.net/~kicad-developers> >> >>>>>>> More help : https://help.launchpad.net/ListHelp >> >>>>>>> <https://help.launchpad.net/ListHelp> >> >>>>>>> >> >>>>>> _______________________________________________ >> >>>>>> Mailing list: https://launchpad.net/~kicad-developers >> >>>>>> <https://launchpad.net/~kicad-developers> >> >>>>>> Post to : kicad-developers@lists.launchpad.net >> >>>>>> <mailto:kicad-developers@lists.launchpad.net> >> >>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >> >>>>>> <https://launchpad.net/~kicad-developers> >> >>>>>> More help : https://help.launchpad.net/ListHelp >> >>>>>> <https://help.launchpad.net/ListHelp> >> >>>>> _______________________________________________ >> >>>>> Mailing list: https://launchpad.net/~kicad-developers >> >>>>> <https://launchpad.net/~kicad-developers> >> >>>>> Post to : kicad-developers@lists.launchpad.net >> >>>>> <mailto:kicad-developers@lists.launchpad.net> >> >>>>> Unsubscribe : https://launchpad.net/~kicad-developers >> >>>>> <https://launchpad.net/~kicad-developers> >> >>>>> More help : https://help.launchpad.net/ListHelp >> >>>>> <https://help.launchpad.net/ListHelp> >> >>> >> >>> _______________________________________________ >> >>> Mailing list: https://launchpad.net/~kicad-developers >> >>> <https://launchpad.net/~kicad-developers> >> >>> Post to : kicad-developers@lists.launchpad.net >> >>> <mailto:kicad-developers@lists.launchpad.net> >> >>> Unsubscribe : https://launchpad.net/~kicad-developers >> >>> <https://launchpad.net/~kicad-developers> >> >>> More help : https://help.launchpad.net/ListHelp >> >>> <https://help.launchpad.net/ListHelp> >> >> _______________________________________________ >> >> Mailing list: https://launchpad.net/~kicad-developers >> >> <https://launchpad.net/~kicad-developers> >> >> Post to : kicad-developers@lists.launchpad.net >> >> <mailto:kicad-developers@lists.launchpad.net> >> >> Unsubscribe : https://launchpad.net/~kicad-developers >> >> <https://launchpad.net/~kicad-developers> >> >> More help : https://help.launchpad.net/ListHelp >> >> <https://help.launchpad.net/ListHelp> >> > >> > >> > _______________________________________________ >> > Mailing list: https://launchpad.net/~kicad-developers >> > <https://launchpad.net/~kicad-developers> >> > Post to : kicad-developers@lists.launchpad.net >> > <mailto:kicad-developers@lists.launchpad.net> >> > Unsubscribe : https://launchpad.net/~kicad-developers >> > <https://launchpad.net/~kicad-developers> >> > More help : https://help.launchpad.net/ListHelp >> > <https://help.launchpad.net/ListHelp> >> >> >> _______________________________________________ >> Mailing list: https://launchpad.net/~kicad-developers >> <https://launchpad.net/~kicad-developers> >> Post to : kicad-developers@lists.launchpad.net >> <mailto:kicad-developers@lists.launchpad.net> >> Unsubscribe : https://launchpad.net/~kicad-developers >> <https://launchpad.net/~kicad-developers> >> More help : https://help.launchpad.net/ListHelp >> <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
