prevent duplicate QSObjects

[skurfer]

RTFM FTW!

This should fix the problems we’ve seen with duplicates in the third pane (#2097) and God knows how many other places.

This is also, I’m sure, the true fix for #1459. I remember you were trying to clean up the way identifiers were managed at the time @pjrobertson and we had to revert the changes, but could never figure out why. We can probably re-implement those changes now (once I figure out what they were).

[pjrobertson]

what the... how is that even possible?! I knew Cocoa uses hashes (much like Python) to compare objects, but can't believe it was never set. Crazy! [NSObject isEqual:] is the thing that uses the hash.

One thing:

• What happens when an object doesn't have an identifier

And... what the hell: can of worms! Why is there a protocol called QSObject which has methods like identifier defined, and then there's also a class called QSObject which is actually where the identifier iVar is defined. QSObject is a subclass of QSBasicObject which has QSObject (the protocol) as a protocol. Get your head round that!

[skurfer]

What happens when an object doesn't have an identifier

Good point. I’ve changed it to use stringValue as a fallback. I’d prefer displayName since it’s less expensive and not found in a separate category, but I’m afraid there might be a way for that to be nil, too. I’m open to suggestions here.

what the hell: can of worms!

That all makes sense to me. QSBasicObject conforms to the QSObject protocol, which means its subclasses (like QSObject) also conform to the protocol.

But I must admit I’ve never understood the purpose of QSBasicObject in the first place. It seems like an extra layer of complexity that adds no value.

[skurfer]

But I must admit I’ve never understood the purpose of QSBasicObject in the first place. It seems like an extra layer of complexity that adds no value.

The answer to that question will of course pre-date version control. Do you have a minute to weigh in @alcor?

[alcor]

I think it may have been to enable things like QSNullObject as placeholders in arrays for sorting and searching, it was also to allow other non-QSObject things to exist should they need to. Since it doesn't appear to be widely used, there may be more effective ways of doing the same things.

[skurfer]

I remember you were trying to clean up the way identifiers were managed at the time

Now that I’m pretty sure we’ve actually fixed issues with duplicates, we can remove that addToObjectDictionary boolean work-around. We never figured out why it worked anyway (until now).

[pjrobertson]

Looks good, although I'm getting two test failures running this locally:
testEquality and testFileNaming

.../Quicksilver/Tests/Tests-QSCore/QSFileObjectTests.m:43: error: -[QSFileObjectTests testFileNaming] : (([object label]) equal to (@"Safari")) failed: ("(null)") is not equal to ("Safari")

[skurfer]

Yeah, I thought I would fix testEquality with this. Instead, I made it fail consistently instead of just on the first run. I guess that’s an improvement?

The test is probably set up wrong. I don’t think it’s possible to have two different objects with the same identifier (especially after that last commit). So maybe isEqual: can be greatly simplified and the test altered to match.

No idea what’s up with file naming. I’ll look into it.

[skurfer]

Rebased this to make sure tests are happy.

[skurfer]

This was included in the 1.5.0 dev preview and no issues have been reported. Merging.