Re: NSMenuView patch

[Christopher Armstrong]

Hi Fred

I agree this method is very fragile and has a ridiculously high cyclomatic 
complexity, but I'm struggling to work out how to simplify it.

If you split it up so that we have an implementation for each menu style, we 
won't get much code reduction. Each menu style uses most of the code in that 
method, so that there will be little difference between them. We'd also get 
alot of duplicated code which will be even more complicated to maintain (three 
divergent implementations of the same method).

We could also consider splitting each of the if..then sections into separate 
methods, but I think that might end up hiding some of the complexity of the 
method, especially where it re-enters itself through other menu views. There is 
also alot of variables to pass along for the calculations.

I'd be tempted to apply the patch as it stands because our current menu 
handling is frustrating for any user and can cause too much weird behaviour 
(e.g. menus still open if you release the mouse outside the GNUstep windows).

Cheers
Chris

On 20/02/2011, at 01:27 AM, Fred Kiefer wrote:

> First off, we are talking about a method here that is already 350 lines
> long. This surely is wrong and needs to be changed. The patch only
> slightly shortens the method, but makes a few things clearer, which
> surly is a good thing.
> Christopher also took create care to release the captured mouse on all
> possible paths. This is very important, as otherwise the application
> could stop responding. But now consider that somebody else works on this
> method in the future. The person might easily not know about that
> necessity and accidentally break stuff. Even in this patch the mouse
> sometimes gets release after a tracking in a submenu. I don't have a
> clue how this is supposed to work. We always have to release the mouse
> before handling over control to a different window. This of course is
> easy to fix for now. But who will remember this for future changes?
> 
> As much as I like the idea of this patch, I think it is too fragile code
> to be added. We should think hard on how to restructure the code here to
> make it less fragile and then we might be able to safely add a mouse
> capture to it.
> 
> Cheers
> Fred
> 
> Am 19.02.2011 07:08, schrieb Germán Arias:
>> OK, I tested this patch and works fine. I don't see problems, and
>> definitively is a better implementation for menu in window. Are there
>> objections to apply it? Fred? Greg?
>> 
>> 
>> On sáb, 2011-02-19 at 13:09 +1100, Christopher Armstrong wrote:
>>> Hi Germán
>>> 
>>> I am still investigating both problems. I think the first might be a bit 
>>> more subtle, as I was able to get something similar to happen (the menu 
>>> closes if you slowing move the cursor upwards over the top-level menu item 
>>> and then try and click the minimise button). For this, I've attached a new 
>>> patch you can give a try.
>>> 
>>> I'm still looking into replicating the second issue, but the patch might 
>>> address that too.
>>> 
>>> Cheers
>>> Chris
>>> 
>>> On 19/02/2011, at 12:08 PM, Germán Arias wrote:
>>> 
>>>> On sáb, 2011-02-19 at 10:34 +1100, Christopher Armstrong wrote:
>>>>> Hi German
>>>>> I'm unable to replicate the issues you are talking about. I'm using the 
>>>>> latest SVN code and tried it under GNOME (Metacity) and WindowMaker with 
>>>>> Gorm. 
>>>>> 
>>>>> Could you give a more detailed description of your environment including:
>>>>> * application you're testing with
>>>>> * defaults output relevant to the programme your running (effectively 
>>>>> 'defaults read NSGlobalDomain && defaults read <my app name>')
>>>>> * window manager
>>>>> * GNUstep version
>>>>> 
>>>>> All are quite important and could make a difference to the environment 
>>>>> that affects this sort of patch.
>>>>> 
>>>>> Thanks
>>>> 
>>>> I have the latest SVN code, but even I face these problems. I'm trying
>>>> this with Ink but I get the same problems with other apps. Here the
>>>> output of "defaults read NSGlobalDomain"
>>>> 
>>>> NSGlobalDomain 'Local Time Zone' America/Guatemala
>>>> NSGlobalDomain NSFontSize 11.0
>>>> NSGlobalDomain GSFirstControlKey Super_L
>>>> NSGlobalDomain NSLabelFont 'Bitstream Charter'
>>>> NSGlobalDomain GSTheme Silver
>>>> NSGlobalDomain GSSecondAlternateKey ISO_Level3_Shift
>>>> NSGlobalDomain GSUseIconManager 1
>>>> NSGlobalDomain NSLanguages '(
>>>>   Spanish
>>>> )'
>>>> NSGlobalDomain GSSecondCommandKey Control_R
>>>> NSGlobalDomain NSLabelFontSize 11.0
>>>> NSGlobalDomain GSWorkspaceApplication ''
>>>> NSGlobalDomain GSSecondControlKey Super_R
>>>> NSGlobalDomain NSPreviewApp evince
>>>> NSGlobalDomain NSMenuFontSize 10.0
>>>> NSGlobalDomain NSMenuFont 'Bitstream Charter'
>>>> NSGlobalDomain NSMenuInterfaceStyle NSWindows95InterfaceStyle
>>>> NSGlobalDomain GSFirstAlternateKey Alt_L
>>>> NSGlobalDomain NSFont 'Bitstream Charter'
>>>> NSGlobalDomain GSRemovableMediaPaths '(
>>>>   "/mnt/floppy",
>>>>   "/mnt/cdrom"
>>>> )'
>>>> NSGlobalDomain GSReservedMountNames '(
>>>>   proc,
>>>>   devpts,
>>>>   shm,
>>>>   usbdevfs,
>>>>   devpts,
>>>>   sysfs,
>>>>   tmpfs
>>>> )'
>>>> NSGlobalDomain GSFirstCommandKey Control_L
>>>> 
>>>> 
>>>> 
>>>> The output of "defaults read Ink"
>>>> 
>>>> Ink NSDefaultOpenDirectory /home/german/Escritorio
>>>> Ink NSMenuLocations '{
>>>>   "\033" = "-1 540 77 202 0 0  1024 768 ";
>>>> }'
>>>> Ink NSRecentDocuments '(
>>>> 
>>>> "file://localhost/home/german/Instalados/FisicaLab/English.lproj/mod_cinema.rtfd/",
>>>> 
>>>> "file://localhost/home/german/Instalados/FisicaLab/English.lproj/bienvenida.rtfd/",
>>>> 
>>>> "file://localhost/home/german/Instalados/FisicaLab/Spanish.lproj/bienvenida.rtfd/",
>>>>   "file://localhost/home/german/Escritorio/pruenamn.txt",
>>>> 
>>>> "file://localhost/home/german/Instalados/Gemas-0.1/HighlighterKit/Documentation/SyntaxFileFormat.rtf"
>>>> )'
>>>> Ink GSScrollerButtonsOffset 0
>>>> Ink 'NSWindow Frame NSFindPanel' '0 546 462 222 0 0  1024 768 '
>>>> 
>>>> 
>>>> I'm trying this on GNOME 2.22.1 (metacity). Here a better explanation of
>>>> the problems:
>>>> 
>>>> 1) Launch Ink, open a submenu, try to minimize the window with a clic on
>>>> title bar. The first click doesn't nothing, the second click close the
>>>> submenu, and the third miniaturize the window.
>>>> 
>>>> 2) Launch Ink, open a submenu, click at main menu in a place where there
>>>> aren't items (for example at right of item "Windows") and wait, after
>>>> some seconds the app crash.
> 

--------
Christopher Armstrong
address@hidden