Re: NSMatrix, NSCell, NSBrowser,NSBrowserCell proposed patch

[Sergii Stoian]

Hi, Fred

On 3/22/07, Fred Kiefer <fredkiefer@gmx.de> wrote:
> I think the general idea of this patch is good, but I don't understand
> the details of it, which makes it hard for me to judge, if it is correct.

That's why I post email before commit to SVN tree.

> What is the relation of the change to NSBrowser to the rest of the
> patch? I don't like hard-code numbers in the code. Why is this one
> needed here?

There's no relation to other part of code. So please don't take it into account.
But in general we need minimum height of NSBrowser list item. Is it wrong?

> If I remember the specification of NSMatrix correctly there are both the
> concept of selected cells there as well as that of one dotted cell, the
> later being the one with the first responder indicator. There may be
> multiple selected cells, but only one dotted, that is key. cell. I am
> not sure. if this concept is reflected in your code. You test if the
> cell is highlighted and it's state is set. Does this match the above
> conditions?

Ok. Let me explain why I made this change to code. I want to have NSBrowser
wich don't have to show first responder state to it's selected row in
last column.
I've try to achieve it by calling setShowsFirstResponder:NO to newly
created rows
which are NSMatrix cells. This is doesn't work with current SVN code.
The reason is:
NSMatrix drawCellAtRow: method set attribute cell to on calls NSCell
drawWithFrame:inView:
then set attribute to off again. So in this situation NSCell (which is
a part of NSMatrix) doesn't
have defined attribute. NSMatrix changes it dynamically whithout
respect to my code.

Later I understand why this algorithm was implemented: NSCell's
drawWithFrame:inView:
method draws dotted frame without checking of _cell.is_highlighted and
_cell.state attributes.
So why I make change to NSCell.

> Why is there no change to NSMatrix selectAll:? The other place where the
> first responder displaying of the cells gets changed.

This is a good question. Main idea of this changes: do not show first responder
state if I set it in my code explicitly (setShowsFirstResponder:NO).
No matter what
selection were made multiple or single.
In general, I haven't check all code which have to adopt these changes.
I've posted these mail to check if I didn't missed something.

> The change to NSBrowserCell looks correct to me, but why not also remove
> this code from NSImageCell and NSPopupButtonCell? And you will also need
> to adopt the code in NSButtonCell to get buttons in a matrix working.

Good point. Thanks. I'll do it.

> I am not opposing your change, but would like to have these questions
> answered first.

No problem Fred! This is what I want: constructive critics and questions.

> Cheers,
> Fred
>
> Sergii Stoian wrote:
> > Hi,
> >
> > I want to propose changes to gui-trunk (patch attached). In general
> > changes are touches NSMattrix and NSCell and is about drawing first
> > reponder indicator (dotted frame). What is done:
> >
> > - NSCell is responsible for showing first responder state;
> > - Removed code from NSMatrix which sets attibute of cell to display
> > first responder state then draws cell and then set attibute to off;
> > - Removed code from NSBrowserCell which draws first responder indicator.
> >
> > Before this patch (current SVN) NSCell draws first responder indicator
> > when it's selected (highlighted) and when it's not slected. That's why
> > NSMatrix and the like should switch on and off NSCell's attribute.
> >
> > Please test the attached code. If there's no objections I'll commit
> > this code to the trunk.
> >
> >
> > ------------------------------------------------------------------------
> >
> > Index: Source/NSBrowser.m
> > ===================================================================
> > --- Source/NSBrowser.m     (revision 24896)
> > +++ Source/NSBrowser.m     (working copy)
> > @@ -1842,6 +1842,7 @@
> >    
> >      cs = [sc contentSize];
> >      ms = [matrix cellSize];
> > +    if (ms.height < 15) ms.height = 15;
> >      ms.width = cs.width;
> >      [matrix setCellSize: ms];
> >      [sc setDocumentView: matrix];
> > Index: Source/NSMatrix.m
> > ===================================================================
> > --- Source/NSMatrix.m      (revision 24896)
> > +++ Source/NSMatrix.m      (working copy)
> > @@ -2086,21 +2086,7 @@
> >      NSRectFill(cellFrame);
> >    }
> >
> > -      if (_dottedRow == row
> > -          && _dottedColumn == column
> > -    && [aCell acceptsFirstResponder]
> > -          && [_window isKeyWindow]
> > -    && [_window firstResponder] == self)
> > -  {
> > -    [aCell setShowsFirstResponder: YES];
> > -          [aCell drawWithFrame: cellFrame inView: self];
> > -          [aCell setShowsFirstResponder: NO];
> > -  }
> > -      else
> > -  {
> > -    [aCell setShowsFirstResponder: NO];
> > -          [aCell drawWithFrame: cellFrame inView: self];
> > -  }
> > +      [aCell drawWithFrame: cellFrame inView: self];
> >      }
> >  }
> >
> > @@ -4112,20 +4098,7 @@
> >      NSRectFill(cellFrame);
> >    }
> >
> > -      if (_dottedRow == row && _dottedColumn == column
> > -    && [aCell acceptsFirstResponder])
> > -  {
> > -    [aCell
> > -      setShowsFirstResponder: ([_window isKeyWindow]
> > -                               && [_window firstResponder] == self)];
> > -  }
> > -      else
> > -        {
> > -    [aCell setShowsFirstResponder: NO];
> > -  }
> > -
> >        [aCell drawWithFrame: cellFrame inView: self];
> > -      [aCell setShowsFirstResponder: NO];
> >      }
> >  }
> >
> > Index: Source/NSCell.m
> > ===================================================================
> > --- Source/NSCell.m        (revision 24896)
> > +++ Source/NSCell.m        (working copy)
> > @@ -2017,7 +2017,8 @@
> >
> >    // Draw first responder
> >    if (_cell.shows_first_responder
> > -    && [[controlView window] firstResponder] == controlView)
> > +    && [[controlView window] firstResponder] == controlView
> > +    && (_cell.is_highlighted || _cell.state))
> >      {
> >        // FIXME: Should depend on _cell.focus_ring_type
> >        [[GSTheme theme] drawFocusFrame: [self drawingRectForBounds:
> cellFrame]
> > Index: Source/NSBrowserCell.m
> > ===================================================================
> > --- Source/NSBrowserCell.m (revision 24896)
> > +++ Source/NSBrowserCell.m (working copy)
> > @@ -323,9 +323,6 @@
> >    // Draw the body of the cell
> >    [self _drawAttributedText: [self attributedStringValue]
> >    inFrame: title_rect];
> > -
> > -  if (_cell.shows_first_responder == YES)
> > -    NSDottedFrameRect(cellFrame);
> >  }
> >
> >  - (BOOL) isOpaque
> >
> >
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > Discuss-gnustep mailing list
> > Discuss-gnustep@gnu.org
> > http://lists.gnu.org/mailman/listinfo/discuss-gnustep

--
Sergii Stoian, ProjectCenter maintainer