Tom and Orson, You are far more familiar with this code that I am. I'm going to defer to you for input and approval on this. It's presently looking more involved than I am comfortable with during a feature freeze. Please let me know what you think when you get a chance to review and test it.
Thanks. Wayne On 1/9/2018 3:29 PM, Jeff Young wrote: > 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 <[email protected] >> <mailto:[email protected]>> 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 <[email protected] >> <mailto:[email protected]>>: >> >> 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 <[email protected] >>> <mailto:[email protected]>> 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 <[email protected] >>> <mailto:[email protected]>>: >>> >>> Did you do a CMake? There’s a change in the CMake files…. >>> >>> > On 9 Jan 2018, at 18:59, kristoffer Ödmark >>> <[email protected] >>> <mailto:[email protected]>> 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 >>> >>>>> <[email protected] >>> <mailto:[email protected]>> 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 : [email protected] >>> <mailto:[email protected]> >>> >>>>>>> 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 : [email protected] >>> <mailto:[email protected]> >>> >>>>>> 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 : [email protected] >>> <mailto:[email protected]> >>> >>>>> 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 : [email protected] >>> <mailto:[email protected]> >>> >>> 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 : [email protected] >>> <mailto:[email protected]> >>> >> 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 : [email protected] >>> <mailto:[email protected]> >>> > 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 : [email protected] >>> <mailto:[email protected]> >>> 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 : [email protected] > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
