Skip to content

Nimbus Photos Library #107

Closed
NukemHill opened this Issue Nov 4, 2011 · 27 comments

6 participants

@NukemHill

Jeff,

I've been hacking at the Photos code, and trying to understand the architecture. I've been swimming in NIToolbarPhotoViewController, and the sample code in NetworkPhotoAlbumViewController and DribbblePhotoAlbumViewController.

My purpose is to build an AlbumViewController that loads image paths from an NSManagedObject, and then pulls the image up from the file system. I might, eventually, make it so that it does either local or network, but I'm not worried about that at the moment.

What I'm finding is that several code chunks in NetworkPhotoAlbumViewController should actually be refactored upwards, either into NIToolbarPhotoViewController, or into a sub-class of that. Aaaaand, actually, I'm beginning to think that a general purpose NIPhotoScrollViewController should be the parent of NIToolbarPhotoViewController, and it should manage all of the work not specific to the toolbar/scrubber work done by NIToolbarPVC.

Some of the work that I think should be in NIPhotoScrollViewController:
1) manage _highQualityImageCache and _thumbnailImageCache;
2) - (NSString )cacheKeyForPhotoIndex:(NSInteger)photoIndex;
3) NIPhotoAlbumScrollView
_photoAlbumView;
4) @property (nonatomic, readonly, retain) NIPhotoAlbumScrollView* photoAlbumView;
5) - (void)loadThumbnails; (currently in DribbblePAVC);
6) - (void)loadAlbumInformation; (which would actually be in the data source identified by this new NIPhotoSVC);

Before I get too deep in the weeds here, I'd like your perspective on what I'm proposing. Especially if I'm missing something fundamental. The main reason I'm suggesting all of this is that I'm finding I need some of these pieces, but they're either buried in the example code sited above, or seemingly inappropriately (again, in my opinion!) in NIToolbarPVC. This refactoring, I think, will allow for easier implementation of sub-classes of the photo viewers in the future, as some of the "heavy lifting" would already be handled.

Thoughts?

@kyvu
kyvu commented Nov 4, 2011

I think this is a great idea. I'm looking to do the same thing as you NukemHill but it would make sense that some of those code be in the framework already.

@NukemHill

@kyvu - thanks. I sat on it last night and I think I'm going to go ahead and do it. I'll submit it here once it's done for you guys to look at. It's not a horrible amount of work. I'm just worried I'm missing something fundamental that makes it a wasted exercise.

@kyvu
kyvu commented Nov 6, 2011

@NukemHill
Awesome. Looking forward to it.

@rogchap
rogchap commented Nov 7, 2011

Hey, I'm not an expert on the photo stuff, but I know that @jverkoey is pulling out the paging scroll views: see #97

@jverkoey
Owner
jverkoey commented Nov 7, 2011

Work's being done here: https://github.com/jverkoey/nimbus-workbench/commits/

I'll hopefully get to merging this into master sometime this week and then we'll make a 0.9.1 cut.

@NukemHill

@jverkoey - I've done much of the refactoring that I talk about above. How should I get the code to you?

@jverkoey
Owner
jverkoey commented Nov 7, 2011

Open a pull request on jverkoey/nimbus master. Ignore my workbench because that's just meant for my personal use when I need to move files around.

@NukemHill

K. I've been digging around, but I'm not quite sure how to do this. I downloaded the source a few weeks ago. And it's been managed by the Github Mac application. Do I commit the changes I made that way, or is their something else I need to do?

Definitely not experienced in this arena. I just want to make sure I don't stomp on anything.

@NukemHill

Also, there aren't any unit tests for photos. Aren't a whole lot of unit tests for anything. Is that something that will come in the future? I've not done a whole lot with unit tests, and would like to learn. If you've got a template for how you want them done, I'd love to see it. I think this will be a great platform to learn on.

@jverkoey
Owner
jverkoey commented Nov 8, 2011

If you cloned the repo using the Github Mac app then open the repository in the app. Click the Branches tab and then click the "+" button in the top right corner of the master branch. Create a new branch that's appropriately named. Save your changes in this new branch and commit the changes to the branch, then publish the branch to your github clone of nimbus. You then issue a pull request via the pull request guide rogchap just linked to.

@jverkoey
Owner
jverkoey commented Nov 8, 2011

There aren't any unit tests for the UI features sadly because I haven't experimented too much with UI testing yet. I imagine the Automation framework would be a good place to start looking into writing tests for the UI features, though.

@NukemHill
@exalted
exalted commented Feb 22, 2012

Hey everyone. I created this image album view controller PTImageAlbumViewController here which will wrap up essentially what's going on in NetworkPhotoAlbum example project in Nimbus.

I believe having coded it much like an UITableViewController, so instead of figuring out all data source or caching stuff, just inherit from it and implement PTImageAlbumViewDataSource protocol.

An example demo project is also included. Bare with me until I write some README on how to install and showcase all that with a couple of screenshots.

Please let me know what do you think...

@NukemHill

I'm back, I think. :-/ Life's been stupid, but hopefully I'm going to be able to get back into this code.

@exalted I'll take a look at what you've done and see if this is what I was talking about a few months ago. If not, then I'll check my code back in and let @jverkoey take a look at it.

I forked the latest version of nimbus fresh. Hopefully, I'll figure out how to manage everything without accidentally overwriting my changes. Did that once, 'though fortunately I had a backup.

@NukemHill

@jverkoey By the way--I seem to recall the topic of automated testing UI stuff through Instruments came up at WWDC last year. It might be worth digging through the video library to see of there's something meaty enough there to give us a leg up on unit testing this code.

I didn't go to the session, but I'm pretty sure I either saw it on the schedule, or heard mention of it at some point.

In case you can't tell, I'm not in any way caught up on conversation threads here. If this has already been resolved, then ... never mind! :-)

@NukemHill

I've got the core code integrated in, but now I need to figure out what modifications I made to the sample code (NetworkPhotoAlbumViewController & DribbblePhotoAlbumViewController). I'm getting foggy, as it's been a long day. But I'm pretty close. I'll wrap it up tomorrow and then push my fork up.

@jverkoey Can you integrate the fork into the master codebase, or is there something else that needs to be done? I don't know the inner workings of how forking works in source control, so I don't know if you'll have access to the code I check in.

Let me know if there's anything you need me to do.

By the way--I went with a fork because I could never figure out how to get Github Mac to do what you outlined above. It was being rather stubborn in refusing to look anything like what you were describing. I'm sure it was a total failure on my part. But I gave up in frustration after multiple attempts. Alas.

@NukemHill

@jverkoey Okay. I think I'm running into a philosophical issue re: the overall design of the photo code.

In digging through NetworkPhotoAlbumViewController and DribbblePhotoAlbumViewController, I'm seeing some implementations that are very generic, and could be moved up into NIToolbarPhotoViewController, or my new NIPhotoAlbumViewController. But you indicate in NITPVC that:

This controller does not implement the photo album data source, it simply implements
some of the most common UI elements that are associated with a photo viewer.

For an example of implementing the data source, see the photos examples in the
examples directory.

This seems to indicate that maybe you do not want generic methods like - (void)loadThumbnails, - (NSInteger)numberOfPagesInPagingScrollView:(NIPhotoAlbumScrollView *)photoScrollView, etc. moved up to NITPVC or NIPAVC. Am I reading this right? These methods are plain vanilla, and it would seem to be appropriate to have them in the super-classes.

I'm also trying to sort through the appropriate definitions of "toolbar". I appears your view is that the scrubber is part of the overall chrome implementation, and is managed by the toolbar code. Does that mean that all of the thumbnail management code should be in TPVC, rather than the new parent PAVC? Or do you view thumbnails as an intrinsic part of the overall photo album "experience" and that management code belongs in PAVC? My belief is that later, but I don't want to stomp on some some overall design philosophy that I'm not clear on.

I'm going to proceed based on my how I think it should work, and submit it. But if you have any strong objections, I'd like to hear them. I don't mind refactoring based on any insights and clarifications you can give me.

@NukemHill

@jverkoey Okay, another question:

In DribbblePhotoAlbumViewController (as well as the Facebook VC), an array of dictionaries is being built, with all of the photo details, off of the downloaded data. There are keys, like @"dimension", @"originalSource", @"thumbnailSource", etc., that are being used to build this dictionary. And then later, these keys are being used to access the data.

It seems to me that a large part of this code can, again, be abstracted up a couple of levels to the appropriate parent class. Basically, there is a common structure to most of the photo album data.

Does this make sense to you? I'm going to do this abstraction, and then push the code changes up to my fork. I'd appreciate any feedback you're willing to give. I think that abstracting as much as we can will make the codebase more flexible in the long run. But I want to make sure that I'm not missing anything.

Edit: Hmm. Okay, starting to see a few complications. You're building the array of dictionaries inside of - (void)operationWillFinish:(NINetworkRequestOperation *)operation. This is a network-level task, so some of it could be abstracted to NetworkPhotoAlbumViewController, which is currently not a part of the core library. I think it's worth exploring adding it, but I'll leave that for later.

So, some of the code really does belong at this level, as it really is specific to network-based data management. But some of it still can be abstracted, so I'll manage that for now, and then examine the network-specific work and see what can be done there.

@NukemHill

I've made the changes and pushed them to my fork. I'm taking a stab at refactoring it using decoupled data sources (#151). I'm having a bit of a time getting my brain wrapped around it, but it's probably 'cause I'm tired. I'll take another look at it tomorrow.

If anyone has any insights on how to get my fork over here, I'm all ears.

@NukemHill

Alright. I think I've run into a glitch in Xcode 4.3. At least, it's the only thing I can think of.

I've completed the refactoring that decouples the data source from the view controller. It works, and seems to work well.

The problem is, I branched this code off of the branch I made above to refactor the view controllers. And it appears that Xcode, GitHub for Mac, and github.com aren't talking to each other very well now.

I've executed a commit inside of Xcode. However, the branch this code is on does not show up in either GitHub for Mac or github.com. The branch doesn't even exist, as far as they seem to be concerned.

Is this because I branched a branch, instead of merging the first changes back into master, and then branching off of that again? That's pretty screwed, if that's the case.

Any ideas here? I'd really like to push the code up and have you guys take a look at it. I've got some questions about my approach and would like some feedback.

@jverkoey @rogchap - If you guys are still paying attention, I'd appreciate any assistance you can bring to this.

@bartvandendriessche

Hey NukemHill,

There shouldn't be anything wrong with branching of a branch instead of master.

If you open up console, cd into the root of your project directory, and run

git branch -a

What is the output you get ? Also take not of which branch has a little asterisk next to it, since that is your current branch.

If you want to push your branch_of_branch, you should be able to:

git push origin <name_of_branch_of_branch>
@NukemHill
@NukemHill

@bartvandendriessche - Well, I thought I had things handled, but now everything's completely FUBAR'd.

I did what you suggested, and it worked well, although the secondary branch still doesn't show up in GitHub for Mac. But the branch shows up in Github.com. So that's fine.

However, I tried to do some merges, to get everything down into master, so that I could start fresh, and I seem to have really screwed the pooch, at this point. I am now dealing with a problem where I try to commit code, but project.pbxproj (under Nimbus) has a little "C" next to it, and nothing I do will fix it. I've restored from backups, pulled down the commit I pushed up yesterday.... Nothing.

I don't know what the hell to do at this point. I'm so frustrated with this whole process. Stuff that should seemingly be straight-forward is hopelessly complex and filled with land mines.

There is a comparison panel that shows the current version of the .pbxproj file and the latest "archived" version, but the differences seem completely cosmetic to me, and I can't suss out what the heck the issue really is. What should I be looking for? How do I clean up the file so that I can commit everything and proceed with my merge?

I know it's not your job to hold my hand here, so I apologize if I'm coming across as demanding. But I'm really at my wits end. I spend so much time wrestling with source control, when I've got so many more important tasks that need to be managed. Any hints you (or anyone else) can throw at me would be greatly appreciated.

I'm going to post this on the Apple Forums, too. If I get any responses there, I'll cross-post them here.

My ultimate goal, by the way, is to get the mods I've been talking about in this thread posted up for you guys to look at. I'd really like some feedback on my approach and whether or not it's worth integrating back into the main codebase.

@NukemHill

I did a little digging, and the issue with the pbxproj file is happening for others, as well. It has something to do with "staging", but the proposed fixes are not working in my case. I don't really understand the underlying issue, which certainly doesn't help matters any.

What, exactly, is staging, in git? How do I get from the current situation, to the point where the code is actually committed and pushed up to github, and Xcode is happy?

@NukemHill

Okay. Re-googled, and found a tutorial on git that showed me how to do it all through the command-line. "git add" and "git commit" did it. However, now Xcode is hanging in the push dialog. I've tried twice, and have had to force-quit both times.

I've reached the point where I think I'm just going to have to buckle down and learn how to manage git through the command-line. I'd really rather run it all through IDEs, but that seems problematic at this point. Of course, if Apple had their act together with Xcode, this wouldn't be nearly the problem it is now.

I'm going to work on the merge process now. I'll let you know how it goes....

@bartvandendriessche

@NukemHill good to hear you're making some progress.

Git can be a bit of a pain to learn initially, and being able to do stuff through the commandline will definitely help you.

Personally I almost never use the git tools baked into XCode and always open up a terminal for committing.

Reading through http://progit.org/book/ will give you a good head start if you haven't checked it out already.

@jverkoey jverkoey closed this Jun 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.