Re: NSMatrix, NSCell, NSBrowser,NSBrowserCell proposed patch

[Sergii Stoian]

On 3/22/07, Fred Kiefer <fredkiefer@gmx.de> wrote:
> Sergii Stoian wrote:
> > 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?
> >
>
> Not sure here. The minimal size should at least come from the selected
> font and not be hard coded. Perhaps using the defaultLineHeightForFont
> of the font. There are many places left in GNUstep, where we still use
> magic numbers, these should go away gradually.

Sure. I agree with you. 

> >> 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.
> >
>
> Do I understand this correctly, you want an NSBrowser where cells don't
> show their (selection or highlight) state by drawing a dotted frame?
> If this is what you want, why don't we implement the different
> NSFocusRingType values in NSCell and set the default type for
> NSBrowserCell to NSFocusRingTypeNone?
>
> I think the following code would do:
>
>   // Draw first responder
>   if (_cell.shows_first_responder
>     && [[controlView window] firstResponder] == controlView)
>     {
>       switch (_cell.focus_ring_type)
>     {
>       case NSFocusRingTypeDefault:
>               [[GSTheme theme] drawFocusFrame: [self drawingRectForBounds:
> cellFrame]
>                                  view: controlView];
>                 break;
>         case NSFocusRingTypeExterior:
>               [[GSTheme theme] drawFocusFrame: cellFrame
>                                  view: controlView];
>                 break;
>         case NSFocusRingTypeNone:
>         default:
>                 break;
>         }
>     }
>
> But neither this nor your patch would solve the problems I see when
> using an NSBrowser. Here highlighting of a cell may be switched of by
> selecting that cell once more. That way you cannot tell, which cell is
> active.

You're right. What you talking about (unhighliting of selected cell) is default  behaviour
of NSBrowser. By default NSBrowser shows responder ring of selected cell. So there's no
problem here.

> Currently it is also not clear to me, which cells in the browser
> get the focus ring drawn and which don't.

NSBrowser gets this information from NSMatrix (which is browser's column). And NSMatrix
decides whether to draw focus ring or not.

> My feeling is that your patch hides the problems on NSBrowser and breaks
> code in NSMatrix.

Yes, you're right. My change to NSMatrix  was incorrect. Forget it please.

> > 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.
> >
>
> OK, but why do you think that highlight and state should have anything
> to do with the focus ring?
>
> >> 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.
>
> How many cells with focus rings would you like to see in a matrix with
> multiple selected cells? None, one or all?

Current selecting is right.

> Just think about the fact that choice items are normally displayed in a
> matrix. I think that your change worked for you as you only did it on
> NSCell, but not on NSButtonCell.
>
> > 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.
>
> If nobody opposes, I will remove the focus ring drawing from these
> classes and update the code NSCell and NSButtonCell to take the focus
> ring style into account. Then you may experiment if setting the style to
>  NSFocusRingTypeNone solves your problem.
> And perhaps somebody looks into the general issue as well.

I've attached fpatches to NSCell and NSMatrix as a beginning. The changes are:
 NSCell:   
   [setShowsFirstResponder:]: update _cell.focus_ring_type wrt 'flag' parameter
   [drawWithFrame:inView:]: add proposed code by you.

NSMatrix: method _drawCellAtRow:column: removed. It duplicates drawCellAtRow:column:.
Some cleanups in drawCellAtRow:column.

What do you think?

-- 
Sergii Stoian, ProjectCenter maintainer