Re: GWorkspace Segfault

[Fred Kiefer]

On the road

Am 25.11.2013 um 02:10 schrieb Riccardo Mottola <riccardo.mottola@libero.it>:

>> On 11/24/13 12:41, Fred Kiefer wrote:
>> Oops, I wasn't aware of that. Yes of course, there is more to remove.
>> All that similar code in FSNTextCell that is never used. And in
>> FSNBrowserCell the fontAttr variable is a static although it is used by
>> different cells which might have different fonts. Obviously this isn't
>> going to work. I think it should just be a local variable in the
>> cuttitle method.
> Yes it is dubious. It is however dynamically set in setFont, at least that. 
> Perhaps it is supposed that all cells will have the same font?

The method setFont: is an instance method, there is no guaranty that it will 
always be the same for all cells.
> 
> 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 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.

> What I do wonder is the very strange way to cache it, I have never seen that 
> and I don't understand it.
> 
> Look at this:
> typedef NSString *(*cutIMP)(id, SEL, id, float);
> 
> that's the only typedef for cutIMP and I can hardly get what it means, the 
> IMP variable is then declared this way:
> static cutIMP cutTitle = NULL;
> 
> and used this way:
>   cutTitle = (cutIMP)[self instanceMethodForSelector: cutTitleSel];
> 
> and invoked this way:
> cuttitle = (*cutTitle)(self, cutTitleSel, uncuttedTitle, textlenght);
> 
> 
> I tried to rewrite it the way I have seen done in other places, by just using 
> "IMP". Please look at the attached patch.
> The main difference is
> 
> static IMP cutTitleImp = NULL;
> 
> (rewriting is just a first step to make it then outside initialize, but I am 
> tracking down the error and ask for help).
> 
> The attached patch breaks. The method gets called (!) but by an NSLog inside, 
> the last float parameter textlenght is always passed as 0.
> 
> Why ????

Either remove the caching completely or leave the old correct code. The typedef 
is the only way the compiler knows about the passed parameter and if you remove 
that it may corrupt you stack.

> 
>> This method also doesn't take into account the paragraph style (writing
>> direction and alignment). You would need something like NSCells
>> _nonAutoreleasedTypingAttributes method for this, but this isn't public
>> and doesn't exist on Cocoa.
> Are you referring to having file-names written right-to-left for example?
> Well, that's perhaps a task for the future. First let's remove crashes and 
> optimize stuff.

The actual code wouldn't be any more complicated, just correcter.

>> 
>> Don't get me started on code reviewing these classes. You could get back
>> a lot more input than you could handle :-)
> Not all a the same time. But there are several classes in FSNode that I want 
> to clean up and don't really know how :) Thus slowly... it will improve

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.