Re: Revised GUI patch

[Alexander Malmberg]

Adrian Robert wrote:
> Hi,
>
> I'm attaching a patch that contains the non-controversial, completed
> parts of my earlier patch to GUI to get Emacs working, in cleaned up
> form. Please consider this for commit.  Also, I listed some pending
> issues (from excluded portions of the original patch or otherwise)
> below.

Thanks for the patch!

I've rearranged the mail and tried to group patches and descriptions 
together when commenting on (some of) them. Unless there are other 
objections, feel free to commit the parts that are OK (with suitable 
ChangeLog entries, etc. :).


> Source/Functions.m
> NSBestDepth() - check outarg exactMatch for non-nil before setting.

OK.


> Headers/AppKit/NSApplication.h
> added instance variable '_app_is_launched'
>
> Source/NSApplication.m
> -run - only call -finishLaunching first time

Seems OK in principle, but this breaks binary compatibility, so it'll 
have to wait. I'll get back to you on this (soon, I hope).


> Source/NSApplication.m
> -setMainMenu: - make new menu visible after hiding old one
>
> Index: Source/NSApplication.m
> ===================================================================
> @@ -2225,6 +2233,8 @@
>      {
>        [_main_menu close];
>        [[_main_menu window] setLevel: NSSubmenuWindowLevel];
> +      //FIXME: should only do if determine that prev menu was visible..
> +      [aMenu display];
>      }
>
>    ASSIGN(_main_menu, aMenu);

The main menu is visible iff the app is active. A patch I've had locally 
for a while is:

> @@ -2234,6 +2251,9 @@
>   [[_main_menu window] setTitle: [[NSProcessInfo processInfo] 
processName]];
>    [[_main_menu window] setLevel: NSMainMenuWindowLevel];
>    [_main_menu setGeometry];
> +
> +  if ([self isActive])
> +    [_main_menu _showOnActivateApp: nil];
>  }
>
>  - (void) rightMouseDown: (NSEvent*)theEvent

and this seems like a more correct way of handling this (display only if 
app is active, display _after_ setting menu window attributes, and use 
the "standard" method for displaying the main menu). Could you check if 
this fixes whatever issue you were having before?


> Source/NSColorList.m
> -initWithName:fromFile: - 'path' argument should not include
>               filename; change code to take account of
>               this but support inclusion (with
>               deprecation warning) for now;

Isn't the description the wrong way around? Anyway, OpenStep seems 
fairly clear that the path should include the file name, so this is OK 
if you document -initWithName:fromFile: (to avoid future confusion), and 
with some changes (below).

> also,
>               support text color list file format, only
>               RGBA for now

> Index: Source/NSColorList.m
> ===================================================================
> @@ -104,9 +106,12 @@
>    {
>      if ([[file pathExtension] isEqualToString: @"clr"])
>        {
> -        file = [file stringByDeletingPathExtension];
> -        newList = [[NSColorList alloc] initWithName: file
> -                                       fromFile: dir];
> +        NSString *name = [file stringByDeletingPathExtension];
> +              ///PENDING, judging by behavior in OS X fromFile: 
should have the
> +              // path including the filename, though the docs are 
not worded
> +              // clearly...

Drop this comment. Better to document -initWithName:fromName: (and, if 
necessary, add a comment about the uncertainty there).

> @@ -190,16 +195,38 @@
>
>    if (path != nil)
>      {
> -      ASSIGN (_fullFileName, [[path stringByAppendingPathComponent: 
name]
> -  stringByAppendingPathExtension: @"clr"]);
> +      BOOL isDir = NO;
> +      // previously impl wrongly expected directory containing color 
file
> +      // rather than color file; we support this for apps that rely 
on it
> +      if (([[NSFileManager defaultManager] fileExistsAtPath: path
> +                                                isDirectory: 
&isDir]==NO)

Coding conventions say spaces around ==. (They also have a thing or two 
to say about comment style, but that part isn't adhered to all that much 
here anyway.)

> +          || (isDir == YES))
> +        {
> +          NSLog(@"NSColorList -initWithName:fromFile: warning: 
including "
> +@"filename as path of path (%@) is deprecated.", path);

Not sure if conventions say anything about this, but I think it's better 
to line up the @" on the second line with the @" on the first line. :)

> @@ -213,6 +240,77 @@
>      ASSIGN(_orderedColorKeys, [NSMutableArray
>        arrayWithArray: cl->_orderedColorKeys]);
>    }
> +      else if ([[NSFileManager defaultManager] fileExistsAtPath: path])
> +        {
> +          _colorDictionary = [[NSMutableDictionary alloc] init];
> +          _orderedColorKeys = [[NSMutableArray alloc] init];
> +    _is_editable = YES;
> +          /*
> +           * Try reading the following text format accepted by OS X:
[snip]

Since this is a large block of code with lots of local variables, I 
think it'd be better to move it to a new private function or method in 
this file.

[snip]
> +              for (i=0; i<nColors; i++)

Whitespace.

[snip]
> +              if (i == nColors)
> +                {
> +                  could_load = YES;
> +                  _is_editable = [[NSFileManager defaultManager]
> +                                   isWritableFileAtPath: _fullFileName];
> +                }

The could_load==NO handler below allocates clean versions of 
_colorDictionary and _orderedColorKeys, so I think you need to 
deallocate them here (if i!=nColors) to avoid leaking if there's an error.


> Source/NSFont.m
> getNSFont() - use defaultSize for 'fontSize' args of <0 as well as ==0

OK.


> Headers/AppKit/NSScroller.h
> added instance variable '_buttonsWidth'
>
> Source/NSScroller.m
[snip]

Also breaks binary compatibility; more soon...

> Source/NSTabView.m
> - removeTabViewItem: - remove item from display if it is selected;
>                        set display to update if last tab removed
> -drawRect: - don't select first tab if there are no tabs
[snip]
> @@ -98,6 +99,9 @@
>      {
>        [_delegate tabViewDidChangeNumberOfTabViewItems: self];
>      }
> +
> +  if ([_items count] == 0)
> +      [self setNeedsDisplay: YES];

Why not always call -setNeedsDisplay:?


[snip]
> STILL PENDING:
>
> Source/NSApplication.m
> -run - [_listener updateServicesMenu] seems to be called on too many
>        events; a FIXME comment was put in, feedback is needed

I think the current code is pretty much correct. Menu entries or 
services can be enabled/disabled by pretty much anything, and definitely 
by key events (consider the effect of selecting/deselecting text in a 
text view using the keyboard on all menu entries/services that depend on 
a text selection). Is this causing you real performance problems?


[snip]
> NSApplication and/or NSRunLoop
> There seems to be a general problem in the event loop where poll()
> in NSRunLoop is being called with long timeouts, and
> NSApplicationDefined events that come in don't get picked up until
> some keyboard or mouse event occurs.

This reminds me of some similar issues I had a while back. The following 
patch fixed those problems:

http://w1.423.telia.com/~u42308495/alex/NSRunLoop_timers_1.patch

(Uncommitted; I believe I sent an RFC for it but didn't hear anything.)

- Alexander Malmberg