Re: GWorkspace Segfault

[Fred Kiefer]

On 25.11.2013 16:40, Riccardo Mottola wrote:
> Fred Kiefer wrote:
>>
>> The method setFont: is an instance method, there is no guaranty that
>> it will always be the same for all cells.
> I made fontAttr re-calculated now. This should be safe.

That should be fine.

>>> Whiel trying to make infoFont and fontAttr non-static, I got puzzled
>>> and made GWorkspace crash again, so let's move gradually.
>>> The first thing: is infoFont actually needed? It is used aparently
>>> only for the dots, but the whole cell should use its own font (also
>>> since it implements setFont and re-sets fontAttr in there).
>>>
>>> I thus removed infoFont from everywhere and substituted it with just
>>> [self font] where needed, using NSCell's. This even saves memory :)
>>> What do you think? It works, I just commited and you sure have seen
>>> the patch flowing in.
>> I never commented on infoFont, as I don't understand it. I think it
>> has nothing to do with the dots handling and it is an italic font.
>> Maybe you better revert that change?
> I think it is some kind of left-over. It is only used in calculations
> where at the end, the standard text is used. I'm puzzled about the
> italic too. I tested it in all view types and I see no harm.

Actually no, at least not before your changes. It gets used as the font
of the infocell. What ever that is. The important method seems to be
-setNodeInfoShowType:, but I fail to understand what it does.

>>>> I also don't quite see why the cuttitle method gets cached. This just
>>>> obscures the code and doesn't speed up much.
>>> Well, the method is called for each cell in a browser thus it was
>>> thought to cache it.
>> Still not very likely to be noticable.
> I checked FSNTextCell and FSNBrowserCell, those methods get called
> continuosly while scrolling. Thus during initial drawing the speed-up is
> not worth it (with a couple of open views, I get less than 100 calls)
> however while scrolling you get many.
> 
> I thus left in the caching, i just cleaned it up by making the names
> more explicit and by initilializing them in -int and not +initialize.

Being called once for ever draw is no argument to cache a method
implementation. If you did so you could cache almost every method. You
should only do it if a method gets called in a tight loop. Even then it
is better to measure before using an IMP cache.
If you really want to optimize things in the
-drawInteriorWithFrame:inView: method, then have a look at the unneeded
[controlView lockFocus] call or think about the need to draw the windows
background colour. Or even better try to get the method more in line
with the NSCell implementation by moving more code into
-drawingRectForBounds:. Or leave the focus ring drawing to the super
implementation, or try to put your cutting code into the method
_drawAttributedString that you override from NSCell.

All of these attempts would be more worth while.


>> Somehow you didn't pick up any of the suggestions I made, but tried
>> some different a bit dubious changes. Not sure what to make of that. 
> Sorry, I did commit things in pieces. Except the text-alignment where I
> don't understand fully what you mean me to do, I think I implemented
> them all.

Just use the method - _nonAutoreleasedTypingAttributes where you
currently set fontAttr.
That should give you the same result now and will adopt to future
changes a lot better.

> For FSNTextCell fontAttr are needed also for the calculation of
> titlesize, so "caching" titlesize+fontAttr as ivar makes sense to me.

It looks that way, but to really comment on that I would need to
understand FSNIcon, and I clearly don't do so.