Re: NSMatrix, NSCell, NSBrowser,NSBrowserCell proposed patch

[Fred Kiefer]

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.

>> 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. Currently it is also not clear to me, which cells in the browser
get the focus ring drawn and which don't.

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

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

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