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

Copy the text editor string (when creating new objects) don't reference it #1564

Merged
merged 1 commit into from Aug 12, 2013

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 12, 2013

For an example of the bug see #1532 (comment)

This is the weirdest bug I've ever worked on, and it intrigues me how:

a) nobody has ever picked up this bug before (it's been around since for ever I think)
b) it hasn't affected QS that much in the past

But anyway, if you see the teeny tiny disclaimer in the docs you'll see that 'for performance reasons' the same NSString object is returned every time. We want to store it (in our new QSObject), so we have to make a copy of it. Turns out it seems the NSNotification that I've changed does the same thing

Based against release branch

Oh - and the other change is just a performance thing I saw when debugging. All the drawing code in QSObjectCell was being called all the flippin' time even when views were off screen. I thought that was silly

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 12, 2013

nobody has ever picked up this bug before (it's been around since for ever I think)

I think it's been noticed. Wouldn't this explain/fix #1424, #1504, and #1525?

Good catch. Seeing autorelease makes me sad now, but I suppose you had to do it.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 12, 2013

Wouldn't this explain/fix #1424, #1504, and #1525?

I've just checked those 3, and I fixed them all.
I think I've just had a good bug fix day. Your turn to fix 4+ before the day's up ;-)

Good catch. Seeing autorelease makes me sad now, but I suppose you had to do it.

Yep. nothing much else I could do here as we have to copy it.

On 12 Awst 2013, at 22:08, Rob McBroom notifications@github.com wrote:

nobody has ever picked up this bug before (it's been around since for ever I think)

I think it's been noticed. Wouldn't this explain/fix #1424, #1504, and #1525?

Good catch. Seeing autorelease makes me sad now, but I suppose you had to do it.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 12, 2013

Yep. You fixed all of those. 🎈 🎉 🎈

I don't suppose you'd want to cherry-pick that second commit into resultsview and force push to remove it from here?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 12, 2013

Fixes #1424
Fixes #1504
Fixes #1525

...just had to do it 💃

cherry-picked as well

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Aug 12, 2013

P.S. IRC is actually working in China! Online now :)

On 12 Awst 2013, at 22:18, Rob McBroom notifications@github.com wrote:

Yep. You fixed all of those.

I don't suppose you'd want to cherry-pick that second commit into resultsview and force push to remove it from here?


Reply to this email directly or view it on GitHub.

skurfer added a commit that referenced this issue Aug 12, 2013
Copy the text editor string (when creating new objects) don't reference it
@skurfer skurfer merged commit 348e046 into quicksilver:release Aug 12, 2013
skurfer added a commit that referenced this issue Aug 12, 2013
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

2 participants