Re: Revised GUI patch

[Adrian Robert]

On Oct 16, 2004, at 12:06 PM, Alexander Malmberg wrote:

> Adrian Robert wrote:
>
> 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?

Yes, this works.  However, as you know, -_showOnActivateApp: is a 
private method, so a compiler warning gets generated.


> > 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).

Yes, I got the description wrong.. I've redone the patch for this file 
as per this and the other suggestions and it is attached.


> > 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:?

I determined that OS X DOES automatically redisplay when the last tab 
is removed, whereas GNUstep was not doing so, but it seemed like the 
update WAS happening in other cases, so I opted for a minimal change.  
There is one more test I need to do on GNUstep however (remove a tab 
that is not currently selected).  I'll try this and change the patch if 
the display does not update.


> [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?

I cannot definitely say that I notice an objective difference in a 
head-to-head comparison.  However I did observe that the code paths 
that get called on menu update can be quite lengthy.  And I think we 
should always be keeping an eye out for inefficiency in as central a 
place as the main event processing loop.  How about at least excluding 
mouse drags (is the user ever going to actuate a menu while dragging)?


> [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.)

Yes, this fixes the issue completely and the event loop switching for 
Emacs now works exactly as on OS X!  Consider this a strong vote to 
commit from me at least.

nsColorList_textFile.patch
Description: Binary data