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

Migrate Three20's TTMessageController #12

Closed
jverkoey opened this issue Jun 15, 2011 · 43 comments
Closed

Migrate Three20's TTMessageController #12

jverkoey opened this issue Jun 15, 2011 · 43 comments

Comments

@jverkoey
Copy link
Owner

Three20's message controller is fairly useful, even in the presence of the native mail interface because it allows you to customize every aspect of the controller.

Some points of note in transitioning this controller:

  • We are no longer going to have any base view controller bullshit going on like Three20 has. TTMessageController should inherit from UIViewController, plain and simple.
  • The controller is already fairly well documented, but the docs need to be updated with the new doxygen style in order for the docs to be consistent with the rest of Nimbus. This means adding a brief sentence describing each method and property, grouping similar methods, and writing concise, explanatory documentation about when to use the controller and how it should be used.
  • I'm not entirely convinced that all of the controller's public methods need to be public, so some auditing here would be helpful.
  • The TTMessageField objects are scantily documented. I'm not even entirely sure how these objects work together myself. It would be great to document how each of these fields are used together and how to build custom fields. That being said, it's possible that there is a simpler solution for this as well.

How difficult will this be?

This should be a fairly straightforward port from Three20. Most of the work will be in cleaning up the documentation and providing examples and sample applications.

@bartvandendriessche
Copy link
Contributor

I'll try to have a go at this if that's ok.

Spent some time setting up a simple skeleton + example app, ready to start porting functionality now.

@jverkoey
Copy link
Owner Author

jverkoey commented Oct 3, 2011

Sweet, I've touched up the outline above a bit to clarify some thoughts.

@NukemHill
Copy link

@bartvandendriessche - have you made much progress here? I actually rewrote TTMessageController for my own limited needs. It as a pain, and some of the original code was rather non-intuitive to me. Partly due to the inheritance structure that @jverkoey dismisses above. Part of what I did was to integrate it with CoreDataLibrary (I actually integrated a lot of CDL with Three20; don't know if anyone else has an interest in what I've done). It all got to be rather messy, but I might have some perspective to lend.

If you want another pair of eyeballs on this, let me know.

@bartvandendriessche
Copy link
Contributor

@NukemHill - Unfortunately no.

A straight port may be possible, but feels kinda dirty after reading through and grokking some of the code. The major annoyances to me are:

  • the TTMessageField inheritance structure feels wrong. There is a lot of type checking code which to me feels like a code smell, but I haven't really come up with a better alternative yet.
  • We probably need to port TTTextPickerField first to have a nice solution for picking recipients.

It may be a good idea to have a Nimbus implementation of TTTextPickerField first, and then start with a fairly static version of MessageController.

This MessageController would just have a to: a subject: and a message field.

After that it may become a bit clearer on how to cleanly go forward with making the MessageController a bit more flexible.

Your ideas and comments are very much welcome.

@bartvandendriessche
Copy link
Contributor

I'm going to take a step back from this issue.

I can't really spend enough time to implement a good version of this at the moment.

@rogchap
Copy link
Contributor

rogchap commented Oct 19, 2011

No worries @bartvandendriessche; if you have any code at all that might help someone taking this on please link to it here :)

@NukemHill
Copy link

I've been neck-deep in wrestling with Xcode 4.2 (I don't know if it's me or Xcode, but I've managed to bring it to its knees several times since the GM came out), so I've not had a chance to dig into this at all. I'm hoping I'll have some time later this week or over the weekend to play with nimbus and come up with some approaches.

I will let you know when I have had a chance to explore.

-g

On Oct 18, 2011, at 4:30 PM, Bart Vandendriessche wrote:

I'm going to take a step back from this issue.

I can't really spend enough time to implement a good version of this at the moment.

Reply to this email directly or view it on GitHub:
#12 (comment)

@jverkoey
Copy link
Owner Author

Awesome @NukemHill :) Looking forward to seeing you around!

@joshholat
Copy link

Has anybody made any progress on this?

@NukemHill
Copy link

No. Sorry about that. I've been completely bogged down with prototypes for multiple projects. I'm hoping to get back to Nimbus stuff in a week or so.

-g

On Jan 19, 2012, at 10:28 AM, Josh Holat wrote:

Has anybody made any progress on this?


Reply to this email directly or view it on GitHub:
#12 (comment)

@taknology
Copy link

I am going to give this a go. I have created my own version of this for another project using Nimbus. I am getting ready to again modify that version for another project and will be attempting to make it more generic so I don't have to rewrite it again :). Is there a preference as to where this should live? I'm just wondering if it should be on it's own, like webcontroller, or be part of the core?

@jverkoey
Copy link
Owner Author

It should be a new feature called messagecontroller/NimbusMessageController.

@taknology
Copy link

FYI. This is coming along nicely. I am building a sample app as I go. Makes it easier to migrate that way.

Is there going to be an app showcase like three20 has? I have an app in the app store that I used Nimbus to create.

@taknology
Copy link

I have pushed a branch to my fork here.

https://github.com/taknology/nimbus/tree/messagecontroller

This includes the work I have completed to date on the message controller. It still needs work on the documentation and a bunch of testing, but it is complete and includes an NIPickerTextField in the mix. I have done a fair share of unit testing and it seems to be working well. I would like it if others could have a look and let me know what you think, give suggestions for areas of improvement and let me know what level of documentation is expected (that's the part I really hate doing).

@taknology
Copy link

Is anyone still watching this task?

@jverkoey
Copy link
Owner Author

Yup! Been busy; will hopefully catch up on my Nimbus backlog in the next few days.

@taknology
Copy link

Ok, just checking. It's been eerily quiet! :)

@NukemHill
Copy link

I'll take a look at it. I've been wrestling with #107, and am having trouble with Xcode handling branches properly. So this might be a good time to change directions and look at something new.

@NukemHill
Copy link

@taknology - I've been digging around your fork, and can't find NimbusMessageController. I'm seeing references to it, but no actual code. Has it been committed?

@taknology
Copy link

@NukemHill - Sorry about that. You're right the source files had not been added to my previous commits. It's all there now.

@NukemHill
Copy link

@taknology - I'm having trouble pulling down the changes you've made. Everything I try ends up pulling down the original fork from @jverkoey. None of your changes are included. Short of copying each individual file, there doesn't seem to be a way to grab your stuff.

Any idea what I'm doing wrong?

Yes, Github continues to baffle and befuddle me. I feel like a noob idiot sometimes....

@taknology
Copy link

@NukemHill - What are you using to try to get the repository?

@NukemHill
Copy link

I change the branch to messagecontroller, and then have tried to Clone in Mac and download the zip file. Either way, I only get the original fork from @jverkoey. None of your changes show up.

@taknology
Copy link

@NukemHill - I'm not sure what to say. I just now tried the web page for my fork and clicked the Zip button while switched to the message controller branch and it came down to me in a new zip file.

try this in terminal

git clone https://taknology@github.com/taknology/nimbus.git
git checkout messagecontroller

@NukemHill
Copy link

@taknology - Weird. Now it's working for me.

I also ran the commands in terminal and it worked.

I was able to run the example. I'll try to dig into it over the weekend.

Thanks for the help. Not really sure what I was doing wrong before.

@NukemHill
Copy link

@taknology - Just tried "Clone" again and it still doesn't pull down your stuff. But the zip/command line options are good enough.

I'm running into another interesting problem with Git, which I've documented over at #107. If you have a second, think you might be able to take a look at see if you have any ideas what's going wrong? Don't want to suck up your time, but I'm kinda baffled with this issue.

Just read the last comment in the thread. Don't worry about the rest of it.

I'm not really strong with the command-line fu with Git, but I'm thinking I may have to buckle down and manage things that way....

@taknology
Copy link

I have updated my fork to include a conversion of NIPostController as well. I still need to go back through and do the documentation and include an example usage of NIPostController in the sample project.

@NukemHill have you had a chance to work with the fork at all?

@taknology
Copy link

@jverkoey - Do you still work for Facebook?

@jverkoey
Copy link
Owner Author

jverkoey commented Apr 9, 2012

I don't; at Google now.

@NukemHill
Copy link

I've taken a brief look at it (enough to prove to myself that I pulled it down right and that it works). I want to dig in deeper, though. I think it might be sufficient enough to replace the monstrosity I've been using from Three20. I seriously modified the Three20 code to handle CoreData, and in retrospect, it was a horrible kludge. Using your migration as a fresh start, I think I might be able to build a much cleaner extension to it.

It'll probably be a few weeks before I get to it, though. I'm trying to close a couple of contracts. When I do, this will most likely end up being a part of the systems I build. So the work I do then will (at least partly) be incorporated into my fork of your code. I'll be sure to keep you in the loop.

-greg

On Apr 9, 2012, at 7:14 PM, Taknology wrote:

I have updated my fork to include a conversion of NIPostController as well. I still need to go back through and do the documentation and include an example usage of NIPostController in the sample project.

@NukemHill have you had a chance to work with the fork at all?


Reply to this email directly or view it on GitHub:
#12 (comment)

@vgrichina
Copy link

@taknology great thanks for a port! I've rebased it on latest master (for cleaner history) and fixed compilation errors: https://github.com/vgrichina/nimbus/commits/messagecontroller/

I'm also curious, why have you updated NIWebController? Looks like you only reformatted it, correct?

@jverkoey
Copy link
Owner Author

How's development on this going?

@vgrichina
Copy link

@jverkoey I am using code by @taknology right now in production. So I think to merge it only some work on documentation is needed.

@jverkoey
Copy link
Owner Author

Awesome! I'll look into this then :)

@taknology
Copy link

I pulled down the latest nimbus version from github with arc support and it caused a few minor issues with this port. I will try to look at those today and push back up to my repository.

@jverkoey
Copy link
Owner Author

Cool, thanks much!

@taknology
Copy link

I have just pushed up changes that migrate the example project and the messagecontroller branch to support ARC. This did require a few core changes so you will want to review closely.

@jverkoey
Copy link
Owner Author

Sweet, thanks @taknology. Can you open a pull request and reference #12?

@jverkoey
Copy link
Owner Author

I'll start going through the change and pointing out high level things that we'll need to fix before we can merge it into Nimbus. From a quick glance it looks like there are some bad practices from Three20 that made their way over and I want to make sure we keep Nimbus' code healthy :)

@taknology
Copy link

Agreed. My goal was to get a working port and then address any outstanding issues. I'm going to try to submit a pull request right now.

@taknology
Copy link

Ok. Please review my pull request and make sure I have done that properly. I know I have not done it right in the past when contributing to the Three20 project.

@jverkoey
Copy link
Owner Author

Looks good! Gonna close this guy while we review that pull request.

@jverkoey
Copy link
Owner Author

Link to #216.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants