github / twui

A framework for developing interfaces on the Mac.
Other
400 stars 79 forks source link

Fixed weakSelf issues #133

Closed CodaFi closed 11 years ago

CodaFi commented 11 years ago

Cleaned up a couple of places where TUIPopover and TUIProgressBar had declared a weakSelf pointer, but didn't actually use it inside the block (:facepunch:), thus leading to retain cycles.

avaidyam commented 11 years ago

Of course, I don't have the reigns to this, but :shipit:! :D

EDIT: What's with the binary files added? Those shouldn't be here...

CodaFi commented 11 years ago

Yes, perhaps that would be cleaner.

avaidyam commented 11 years ago

:+1:

jspahrsummers commented 11 years ago

The hidden files (backups?) should be removed.

jspahrsummers commented 11 years ago

:mailbox_with_mail:

CodaFi commented 11 years ago

I don't see any hidden files that shouldn't be there. Perhaps this is a side-effect of Quarantine on OS X ML screwing with my drive. Also, now that I look into it, it seems to be a Git issue. The branch-creation mechanism must have done a little more than it was supposed to.

jspahrsummers commented 11 years ago

See the diff on github.com. There are definitely file additions.

CodaFi commented 11 years ago

Yes, I know, but for all intents and purposes, they don't exist physically. The finder can't find them, searches turn up empty, and even the terminal thinks they don't exist.

avaidyam commented 11 years ago

Use the GitHub for Mac app, rollback the commit, and when you try to commit the same exact thing, uncheck the _lib files from the commit, and push those alone. That helps.

CodaFi commented 11 years ago

Ah! Got it (with a sudo rm -rf, @jwilling LOL). Will merge in a couple of minutes, any objections?

jwilling commented 11 years ago

@CodaFi Don't merge your own pull request. Let others handle it. :+1:

CodaFi commented 11 years ago

Objection noted.

avaidyam commented 11 years ago

How are you allowed to merge to TwUI though? I see no such option on any PR. And also, how are you assigned to a PR?

jspahrsummers commented 11 years ago

:postbox:

jwilling commented 11 years ago

If you can, please use the two suggestions I've given above and apply them to the rest of the file. However, I should note that if the block has a return type there should be a space between it and their arguments.

avaidyam commented 11 years ago

Is this not optional? Simply using a proper return statement should be fine.

jwilling commented 11 years ago

@galaxas0 Any new contributions should adhere to the style guide as much as possible. The conventions specify that there should be a space between the arguments and the return type if the return type exists. Since it is defined in the style guide, we should conform to it.

avaidyam commented 11 years ago

Ah! Thanks!

CodaFi commented 11 years ago

I believe the source of the weird backup files was because my Github/TwUI repo was being kept on a USB stick (which I reformatted), but apparently either one of the SanDisk crapware processes or Spotlight has been trying to backup and index those files. Not good

CodaFi commented 11 years ago

@jspahrsummers Any idea why changing the pointer type inside of a typdefd block would throw build warnings?

jspahrsummers commented 11 years ago

It makes sense, actually. The block expected is one that accepts any kind of TUIView (although in practice it will only receive a single one), so passing in a block that expects a more specific type should logically result in an error.

So, flip-flopping: because we need a cast, I think we should go back to (id), which is less fragile than a specific type.

avaidyam commented 11 years ago

@jspahrsummers That's odd, why not simply cast the block as a (TUIViewDrawRect) with whatever TUIView superclass you'd like?

jspahrsummers commented 11 years ago

Casting a block destroys a lot more type information than casting an argument to a more specific type. If the block type changes in the future, the call point won't break, and crazy shit will result.

avaidyam commented 11 years ago

@jspahrsummers I didn't think of that! You're right, no one wants crazy shit to happen. (suddenly, I remember shitIsBad in the coding conventions....)

avaidyam commented 11 years ago

Is there a reason this hasn't been merged yet? :D

jwilling commented 11 years ago

Patience, young grasshopper.

avaidyam commented 11 years ago

@jwilling LOL, alright, then i'll go back to hopping.... :dash:

avaidyam commented 11 years ago

@CodaFi You could squash the commits to remove the few commits that went back and forth changing the style.

CodaFi commented 11 years ago

@galaxas0 Yes, I understand that, it was the first thing I tried, but GHfM spawned a weird git cherry-pick process, and threw errors every time I tried to revert the commit.

avaidyam commented 11 years ago

@CodaFi Ah, that's not good. Alright, doesn't make an exactly earth shattering change, so it's all good.

jspahrsummers commented 11 years ago

:shipit: