Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fixes #1114

Merged
merged 15 commits into from Oct 3, 2012
Merged

Bug fixes #1114

merged 15 commits into from Oct 3, 2012

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Sep 17, 2012

Late time issue-fixing-frenzy ;-).

See individual commits for details.

@@ -12,7 +12,7 @@ - (BOOL)getObjectValue:(id *)obj forString:(NSString *)string errorDescription:(
return YES;
}
- (NSString *)stringForObjectValue:(id)anObject {
return [anObject name];
return [(QSObject *)anObject name];
}
Copy link
Member

@skurfer skurfer Sep 17, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just tell this method to expect a QSObject instead of id?

Copy link
Member Author

@tiennou tiennou Sep 17, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was shoddy of changing the interface mainly ;-). But yeah, it doesn't really matter so I'll change the signature instead.

Copy link
Member Author

@tiennou tiennou Sep 18, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now I become doubtful ;-). It's only use is in a commented section in QSObjectCell. I think it might have been an attempt to modernize display of object by creating a real formatter. What do you think ?

Copy link
Member

@skurfer skurfer Sep 18, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you meant this method, but you're referring to the entire class. So should we just remove it?

@skurfer
Copy link
Member

skurfer commented Sep 17, 2012

I was doing something small this morning that probably isn't worth a separate pull request. Maybe it could be added to this one?

All I did was:

  • add [resource closeFile]; to line 56 of NSPasteboard_BLTRExtensions.m.
  • remove #import "NDResourceFork.h" from QSObject_FileHandling.m. It doesn't appear to be used there.

This was in an effort to silence the errors people have been complaining about on the mailing list.

NDAlias ERROR: you neglected to call closeFile: before disposing this NDResourceFork

@tiennou
Copy link
Member Author

tiennou commented Sep 17, 2012

That's fun, I think it's been more or less since that was updated that I get various stashes fixing that one in diverse ways ;-). I thought about putting it directly in NDResourceFork, like in -dealloc. I will investigate and add a commit.

@skurfer
Copy link
Member

skurfer commented Sep 17, 2012

Yeah, I have to wonder about that error. If you know closeFile: needs to be called, just call it and shut up. :-)

I didn't want to make changes to the “external” files, but I see you have no reservations about that. :-)

@skurfer
Copy link
Member

skurfer commented Sep 17, 2012

I'll test this out over the next couple of days. All looks good except my one comment on the plist.

Sorry about all the leaky strings. I try to stay away from CoreFoundation, and maybe I should. :-)

@skurfer
Copy link
Member

skurfer commented Sep 17, 2012

Oh, what about removing line 66 (an unused integer) from NSArray_BLTRExtensions.m?

@skurfer
Copy link
Member

skurfer commented Sep 18, 2012

Better. It's not limiting the third pane to folders, but that has nothing to do with the changes here.

@tiennou
Copy link
Member Author

tiennou commented Sep 18, 2012

Hmm, I can't see the integer in NSArray_BLTRExensions.m so I'll assume it was already fixed.

I think the -closeFile: issue is because you can't -closeFile: in -finalize because you aren't guaranteed that it will be called on the same thread that created the NDResourceFork, and not doing so would make the behavior under GC different that the normal, which is a bad idea. Now since GC is deprecated, I don't care about behavioral differences for something we will have to drop too because of deprecation anyway ;-).

@skurfer
Copy link
Member

skurfer commented Sep 18, 2012

It's not limiting the third pane to folders

I talked to @pjrobertson about this and the plist setting only affects the results of right-arrowing into something. To control the initial list, you still need to mention the action in validIndirectObjectsForAction:directObject:. I think in this case, it should act just like Move/Copy, so you could just add a check for QSCommandSaveAction to line 338.

When checking on this, I noticed that QSCommandSaveAction is tested in QSCommand.m. That doesn't seem right.

@skurfer
Copy link
Member

skurfer commented Sep 18, 2012

Hmm, I can't see the integer in NSArray_BLTRExensions.m so I'll assume it was already fixed.

Weird. It appears to still be there.

@tiennou
Copy link
Member Author

tiennou commented Sep 18, 2012

Oh yeah I got it, it appeared in da3dea7, and this branch is upstream from that so I can't fix it from there without merging in master in here... I'm still wondering at what happens if you rebase a branch from a pull request and push-force it here ;-).

@skurfer
Copy link
Member

skurfer commented Sep 18, 2012

Oh yeah I got it, it appeared in da3dea7, and this branch is upstream from that so I can't fix it from there without merging in master in here

OK, I'll just do it on one of my open pull requests. Thanks.

I'm still wondering at what happens if you rebase a branch from a pull request and push-force it here ;-).

Everything I've read implies that would be a bad idea, but if it worked, it sure would be a handy trick. :-) Maybe something to try on a bogus "test" pull request.

@skurfer skurfer merged commit 09c9243 into quicksilver:master Oct 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants