Hey, I'm not too familiar with the code, but I think NSMenu[Item] are model objects - it seems wrong to be copying them to set up different views (normal openstep, top of window, top of screen, etc.).
I just wanted to repeat a comment from March from Wolfgang and Fred: >> So, in the long run I'd prefer NSMenu were changed to handle multiple >> menu views rather than having this menu duplication cruft. But it is >> certainly too late for the next release ... > > This surely is the way to go. NSMenu should loose its two windows and > the direct reference to the NSMenuView. All these connections should be > the other way around. Hopefully we make some progress in that direction > after the next release. This is still the goal, right? Eric On 2011-06-04, at 4:10 AM, Fred Kiefer wrote: > I see now that your change is again just a workaround. Would you mind to add > a comment, next time you add code that is most likely wrong? > > It surely would help to track down the real problem if you could share and > application showing the issue or at least a back trace of the problem. > > I had a short look at NSMenuItem and we seem to use the ivar _target for two > separate purposes. For the normal target of the menu item and as well for the > super menu, when the item holds a submenu. Could you please try to find out, > which of these two cases is the problematic one? You could do the retain just > in one of the cases (target being an NSMenu or not) and see whether you get > the problem. If it isn't the NSMenu target, then I would expect that problem > to be in a completely different place in your application. Here a static code > analysis or a memory debugger could help. > > I really would like to see this problem resolved this time. The change you > made looks wrong and somebody may remove it again at some point, thereby > breaking your application. > > Fred > > On 03.06.2011 10:19, Gregory Casamento wrote: >> Fred, >> >> Yes, at this point I do realize it was wrong. I'm not entirely >> convinced that the fix I did this time around was correct since it >> involves retaining the _target ivar of the item. However, the fix >> that is currently in place seems "more correct" than using the >> archiver as I was doing before. >> >> I think there is an extraneous release someplace, but, thus far, I'm >> unable to find it. >> >> Later, GC >> >> On Fri, Jun 3, 2011 at 3:41 AM, Fred Kiefer<[email protected]> wrote: >>> Thank you that you finally took the time to inspect the real issue and fix >>> it. In the meantime you surely noticed that the analysis you provided below >>> was wrong. The NSMenu did copy the NSMenuItems. The bug was in the copy >>> method of the item. >>> >>> It would have been a lot better if you had looked into the issue when it >>> first showed up, but everything should be fine now. >>> >>> Fred >>> >>> >>> Am 03.06.2011 um 00:24 schrieb Gregory Casamento<[email protected]>: >>> >>>> Fred, >>>> >>>> It's actually pretty simple to see what's broken in the copyWithZone >>>> method in NSMenu.m. That's what I was trying to explain in the >>>> "Revert". I don't need to write a test program since it's plain to >>>> see on inspection that it's incorrect. >>>> >>>> The method does a recursive copy of the menu and simply inserts the >>>> menu items from the menu being copied into the copy. As I explained >>>> in my comment, this causes all of the menus to share all of their >>>> NSMenuItems, which is not correct. >>>> >>>> It wasn't my intention to do a "silent revert" or anything of that >>>> nature. I'm trying right now to fix some issues I'm seeing with the >>>> Gnome theme and had forgotten about the debate in March. I'll try to >>>> fix this problem properly sometime this evening. >>>> >>>> Thanks, GC >>>> >>>> On Thu, Jun 2, 2011 at 5:47 PM, Fred Kiefer<[email protected]> wrote: >>>>> On 02.06.2011 22:22, Gregory Casamento wrote: >>>>>> >>>>>> Author: gcasa >>>>>> Date: Thu Jun 2 22:22:39 2011 >>>>>> New Revision: 33234 >>>>>> >>>>>> URL: http://svn.gna.org/viewcvs/gnustep?rev=33234&view=rev >>>>>> Log: >>>>>> Use the NSArchiver to copy the menu since the copy method does not do a >>>>>> deep enough copy. >>>>>> >>>>>> Modified: >>>>>> libs/gui/trunk/ChangeLog >>>>>> libs/gui/trunk/Source/GSThemeMenu.m >>>>> >>>>> Haven't we been through that before? >>>>> >>>>> There was a long mail exchange in March why this code was incorrect and >>>>> causing trouble to German. At that point I suggested that you report back >>>>> with a short example program showing what is broken with the [NSMenu >>>>> -copyWithZone:] method. Instead you changed the code to use copy and now >>>>> you >>>>> changed it back and added this comment: >>>>> >>>>> /* >>>>> * NOTE: The reason the copy or copyWithZone method is not used here is >>>>> because >>>>> * it doesn't make a deep copy of the menu. A deep copy is needed in order >>>>> to >>>>> * allow the individual menu items to be used in multiple menus at one time >>>>> without >>>>> * interfering with one another's state. >>>>> */ >>>>> >>>>> This doesn't explain anything, nor will it ever help to resolve the issue, >>>>> if there is any. It just breaks German's code again. >>>>> >>>>> Don't you think that reopening the debate with what ever new evidence you >>>>> came up with would be a lot more productive then doing a silent revert? > > > _______________________________________________ > Gnustep-dev mailing list > [email protected] > https://lists.gnu.org/mailman/listinfo/gnustep-dev _______________________________________________ Gnustep-dev mailing list [email protected] https://lists.gnu.org/mailman/listinfo/gnustep-dev
