Since we are asking users to run debug builds (something I'm not 100% comfortable with but it is what it is), I went ahead and committed your patch. Thanks.
On 9/14/2015 1:46 PM, Chris Pavlina wrote: > Well, personally, I don't like it - as developers looking for things to > refactor aren't the only people who use debug builds. But I also see your > point about not wanting it to get lost in the noise, and I'm not sure how > else to manage that. I really think we ought to have a proper place to triage > things like "refactor OnOrient", but that's a discussion for another time. > > So, I guess, do it however you like. > > On Mon, Sep 14, 2015 at 01:39:27PM -0400, Wayne Stambaugh wrote: >> How about leaving the assert rather than the fail in there as a reminder >> to developers in case it gets lost in the noise (which these things tend >> to do). It will compile away on release builds which is the desired >> behavior. >> >> On 9/14/2015 1:35 PM, Chris Pavlina wrote: >>> Oh, indeed. It's begging to be refactored. But as it stands, kicad crashes >>> (well, throws up a dialog asking the user if they would like it to crash ;) >>> if you press X or Y whilst holding a label. I hoped to get at least _that_ >>> fixed before the release, because that's quite broken. >>> >>> >>> On Mon, Sep 14, 2015 at 01:26:07PM -0400, Wayne Stambaugh wrote: >>>> On 9/14/2015 1:03 PM, Chris Pavlina wrote: >>>>> Well, you're welcome to try to find a non-hackish way to ensure that >>>>> OnOrient is not called for nonorientable objects even when the user has >>>>> selected one. I failed to do so. >>>>> >>>>> Why should that be a failure, anyway? It's not illegal to attempt to >>>>> orient a non-orientable object. I can still press Y and X over anything I >>>>> like. The user interface just silently does nothing when I do that. >>>>> >>>>> In "proper" object-oriented code, OnOrient wouldn't have the switch() in >>>>> there at all, it would simply call an Orient method of the SCH_ITEM and >>>>> then labels and other items that do not orient would just do nothing. >>>>> What is wrong with that behavior? >>>> >>>> Nothing. That would be my preference. It would require some surgery on >>>> all items derived from SCH_ITEM and SCH_ITEM itself because OnOrient is >>>> not defined as a virtual member of SCH_ITEM. Only SCH_COMPONENT has an >>>> OnOrient member. This code is screaming "refactor me". Do I have any >>>> takers for a "fun" task for after the stable release? ;) >>>> >>>>> >>>>> On Mon, Sep 14, 2015 at 01:00:40PM -0400, Wayne Stambaugh wrote: >>>>>> This should probably be a wxASSERT rather than a wxFAIL_MSG so in >>>>>> release builds it would not fail. The assertion was put there so that >>>>>> who ever wrote the code that allowed objects that cannot be oriented to >>>>>> be passed to OnOrient would get a reminder of there error. I would >>>>>> prefer that OnOrient not get called for objects that cannot be oriented >>>>>> rather than removing the assert. >>>>>> >>>>>> On 9/14/2015 10:26 AM, Chris Pavlina wrote: >>>>>>> SCH_EDIT_FRAME::OnOrient uses SCH_COLLECTOR to filter for only >>>>>>> orientable items. The problem is that it only does this for an >>>>>>> unselected item. If the item is returned by SCH_SCREEN::GetCurItem >>>>>>> it'll skip that part and go ahead trying to orient it. Then an >>>>>>> assertion failure "Schematic object type %s cannot be oriented." is >>>>>>> tripped. >>>>>>> >>>>>>> The assertion is totally unnecessary; if the object cannot be oriented >>>>>>> it should just silently not be oriented. This patch removes the >>>>>>> assertion. >>>>>>> >>>>>>> -- >>>>>>> Chris >>>>>>> >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> 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 >>>>> >>>>> _______________________________________________ >>>>> 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 >>> >>> _______________________________________________ >>> 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 > > _______________________________________________ > 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
