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

Drag and Drop (With Latest JNWCollectionView Code) #156

Closed
wants to merge 0 commits into from

Conversation

Deadpikle
Copy link
Contributor

Here's an implementation of Drag and Drop that runs off of the latest version of the JNWCollectionView code. It was based off of the work done by DarkDust at https://github.com/DarkDust/JNWCollectionView. The implementation is not the most beautiful thing in the world, and there could easily be some more work done on it, and I'll be the first one to claim that. I don't really have time to work on this at the moment. Of course, others can always pitch in and clean up!

Notable things that could be cleaned up:

  • Allow for putting the drag and drop marker inside of the layout itself (aka an actual table row, grid cell, etc.) so that you can show a full "blank" spot instead of a line.
  • Cleanup of delegate protocol (Pasteboard really shouldn't be required, I would imagine)
  • General refactoring and code clean up

For those of you wondering why I'm adding a 3rd pull request for drag and drop, I asked for permission first. Thanks, Jonathan!

@jwilling
Copy link
Owner

Closing other pull requests in favor of this one. I don't want to merge this in its current state, but I'll leave it open for anyone interested in implementing this functionality.

For further references on other implementations and designs, see the following pull requests:

This was referenced Dec 30, 2015
@jcampbell05
Copy link

Just a side note. Regarding the blank spot code, there is a library (I'll find it later and edit this comment) that implements drag and drop for iOS table view. It would be a great one to be inspired by for this functionality.

@Deadpikle
Copy link
Contributor Author

@jcampbell05 Like this one? https://github.com/lukescott/DraggableCollectionView I've used this for a project before and it has the sort of functionality for "blank spots" that I'd be looking for. I'm not sure how well the code would translate to JNWCollectionView, but really, I might be overthinking how complicated it should be. The pertinent code would probably be put inside the prepareLayout function, so it'd just take some logic rearranging to insert a "blank" item. I think another delegate method to grab the "empty" cell for the drag/drop view would be appropriate. It would function exactly like collectionView:cellForItemAtIndexPath:. ...I could be totally wrong, of course.

@jcampbell05
Copy link

@Deadpikle Yeah, also I think now 10.11 now finally has a collection view class like iOS with drag and drop support it is better to use that.

@jwilling
Copy link
Owner

Yes, I should really update the readme, as this is now soft-deprecated. I recommend using NSCollectionView now.

@fabioruxo
Copy link

The new NSCollectionView looks promising until you notice that just scrolling up and down will leak memory like crazy. Nice try Apple... But an epic fail memory wise :-(

@jwilling
Copy link
Owner

jwilling commented Mar 4, 2016

@objectivesheep I actually haven't noticed that behavior during my experiences with NSCollectionView. Have a sample app that shows it?

@fabioruxo
Copy link

Actually any tutorial project I downloaded can show this... Even Apple's
own image slides one (the one from the wwdc video). If you fill a
collection view with items and open activity monitor to see memory usage
you will notice that just scrolling up and down the collection several
times will make the allocated memory grow indefinitely(and a lot). Tried
the same with the UICollectionView and the footprint is both smaller and
doesn't grow a tad even if you scroll for hours. Too bad ... I really
needed the nee goodness 😟 Your collection view is awesome because the
footprint is small and memory is optimized but it's a little barebone and
needs some extra work on my side (which I already did... I'm using the jnw
collection in Pins and its performance skyrocketed).

Having said that if you find that I'm wrong and that there's a way to use
the new NSCollectonView without the memory issues then I'll be all ears...

That would be heaven for me :-)

Fabio Russo

@fabioruxo
Copy link

Just one more example. I downloaded this simple project (https://github.com/klaas/CollectionViewElCapitan) and it works great. When I fire it up with some added strings in the initial array it starts with 18MB of memory usage in Activity Monitor. I played around with just scrolling the list and resizing the window for about 5 minutes. Memory is now 74MB!
schermafbeelding 2016-03-05 om 08 56 01

@fabioruxo
Copy link

And just to be fair and to compare things. Here is the memory usage of your JNWCollectionView with 1800+ elements and scrolling/resizing. A stable 95MB. How awesome is that? Kudos man. You know your stuff.
schermafbeelding 2016-03-05 om 09 00 06

@fabioruxo
Copy link

Ok I have to take that back. Tried it some more and in more depth. The memory footprint can get a little large now and then but after a while it always goes down. I've especially noticed a big difference when using binding or not (no binding is usually empirically a smaller footprint). So ok for the new NSCollectionView. It works for me.

@jwilling
Copy link
Owner

jwilling commented Mar 6, 2016

Cool. That memory usage doesn't sound too high considering the number of views / layers visible at any given time. Making the window smaller generally puts more items into the cell reuse queue, which isn't always cleaned up. That means that even if you make your window small again the pool remains the same size, leading to no drop in memory usage.

@Deadpikle
Copy link
Contributor Author

Closed in order to clean up my fork's repo, which was starting to become a mess because I did drag+drop things on 'master' instead of on a branch. I deserve a slap on the wrist for that. :)

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

Successfully merging this pull request may close these issues.

4 participants