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

Allow drop onto empty area of table below last item to add to end of table #18

Closed
wants to merge 2 commits into from

Conversation

cvconover
Copy link

Drag from one table and dropping at the bottom of another table at the bottom below the last item of the drop table adds the dropped item to the end of the drop table.

Rearrange works in similar way. Dropping item below last item of table will swap the dragged item with the last item of the table.

SteveFortune and others added 2 commits March 10, 2014 22:19
handleDragStartedInSrcAtPoint now correctly calls determineIndexForConta...
…tem to end of table list; also allow rearrange in the same matter - dragged item is swapped with last item in table list
@cvconover
Copy link
Author

Just a heads up... looks like some extra user metadata files were added to this pull request.

// Now just construct the index path
indexPath = [NSIndexPath indexPathForRow:([theView numberOfRowsInSection:([theView numberOfSections]-1)]-1)
inSection:([theView numberOfSections]-1)];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if table is empty?
indexPath = <NSIndexPath: 0x918e930> {length = 2, path = 4294967295 - 4294967295}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably do with some additional test cases in the test app to cover scenarios like empty tables. Will put some together when I review/merge.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Steve,
I'll look into your first question about empty table. Should be easy to
address.

On another thought... I wish it were easier to specify a table DataSource
but looks like you have to declare protocol and implement more delegates.
So many cooler things could be done generically if that were the case
rather than hard coding the array with each table. Perhaps some sort of
metadata data structure in the helper could simplify it? Just thinking out
loud.
Craig

On Thu, Mar 13, 2014 at 6:10 AM, Steve Fortune notifications@github.comwrote:

In Classes/I3DragBetweenHelper.m:

@@ -264,6 +264,31 @@ -(NSIndexPath_) determineIndexForContainer:(UIView_) container
}

+-(NSIndexPath_) lastIndexPathForView:(UIView_) container{
+

  • NSIndexPath *indexPath;
  • if([container isKindOfClass:[UITableView class]]){
  •    UITableView _theView = (UITableView_)container;
    
  •    // Now just construct the index path
    
  •    indexPath = [NSIndexPath indexPathForRow:([theView numberOfRowsInSection:([theView numberOfSections]-1)]-1)
    
  •                                   inSection:([theView numberOfSections]-1)];
    

We could probably do with some additional test cases in the test app to
cover scenarios like empty tables. Will put some together when I
review/merge.

Reply to this email directly or view it on GitHubhttps://github.com//pull/18/files#r10563204
.

Lacrosse Stats Collection for the iPad - Get it on the App
Storehttp://itunes.apple.com/us/app/x-static-apps-lacrosse-stats/id516229459?ls=1&mt=8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thanks :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed your second point. Not quite sure what you mean, could you provide some sort of pseudo-example?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking out loud. I need to give it more thought. Once I flesh it
out a bit I'll provide more details or possibly a solution.

On Thu, Mar 13, 2014 at 1:56 PM, Steve Fortune notifications@github.comwrote:

In Classes/I3DragBetweenHelper.m:

@@ -264,6 +264,31 @@ -(NSIndexPath_) determineIndexForContainer:(UIView_) container
}

+-(NSIndexPath_) lastIndexPathForView:(UIView_) container{
+

  • NSIndexPath *indexPath;
  • if([container isKindOfClass:[UITableView class]]){
  •    UITableView _theView = (UITableView_)container;
    
  •    // Now just construct the index path
    
  •    indexPath = [NSIndexPath indexPathForRow:([theView numberOfRowsInSection:([theView numberOfSections]-1)]-1)
    
  •                                   inSection:([theView numberOfSections]-1)];
    

Sorry, missed your second point. Not quite sure what you mean, could you
provide some sort of pseudo-example?

Reply to this email directly or view it on GitHubhttps://github.com//pull/18/files#r10585519
.

Lacrosse Stats Collection for the iPad - Get it on the App
Storehttp://itunes.apple.com/us/app/x-static-apps-lacrosse-stats/id516229459?ls=1&mt=8

@SteveFortune
Copy link
Member

Hi Craig, thanks for the PR - looks cool! I'll merge locally when I get a chance and take a look. It'll probably be over the weekend now though.

I don't think we should be tracking userdata files (at least, I usually gitignore them on other proejcts), I'll add it to a .gitignore along with .DS_Store

@SteveFortune
Copy link
Member

Merge and review this on remote branch cvconover-merge as tracked userdata is interfering with my local repo.

SteveFortune added a commit that referenced this pull request Apr 5, 2014
… nil if the supplied container is not a UITableView or UICollectionView, or empty
@SteveFortune
Copy link
Member

There are some issues I've found with this PR by running through the test cases and having a look through the source...

General issues:

  • If I drag a cell from its container and drop it on an empty part of the container, the exchange animation is between the coordinates of the last cell in the container and the dragging cell, not the coordinates of the empty area on which I dragged the cell, which looks odd.
  • Does not play well with individual cells configured to be undraggable or unrearrangeable in the helper delegate [See 1 and 2 below].
  • If a helper passes a nil cell to the delegate, this PR makes the user assume that this means 'dragging or exchanging to the end'. I don't think that passing a nil cell is a very clear way to express this. We should have another delegate method specifically for dragging to the empty part of the table, e.g. droppedOnEmptyPartOfSrcFromDstAt: (or something similar) to make it more verbose.

Here are some more details on test-case specific issues:

  • (1) 'Unrearrangeble Table to Table' test case:
    • You can drag on the empty part of the rearrangeable table when the last cell is mean to be a placeholder cell fixed to the end of the table. The result is that you can drop cells before and after the cell that says 'Drag here to add...' which is a bit odd.
    • A suggested fix would be to make this feature configurable so that we can choose to turn it off if we want.
  • (2) 'Exchangeable Collections' test case:
    • Similar to the above, this test case is meant to demonstrate 2 'placeholder' cells in the collections (purple-y at the end of each collection).
    • When dragging a cell onto the empty part of the rearrangeable collection, it adds the cell after the purple 'placeholder' cell.
    • This is not only odd, but also causes the last cell of the collection (the one that the delegate marks as 'undraggable and unrearrangeable') becomes the placeholder, and the purple cell becomes draggable and rearrangeable.
  • (3) 'Paint Me' test case:
    • Dragging a UITableViewCell to the empty part of the UITableView causes the exchanging cell transition to the top left hand corner of the screen.
    • Crashes sometimes when dragging a UICollectionViewCell to the empty part of the UITableView. Here's my stack trace:

2014-04-05 18:42:53.063 Test App[75322:a0b] Drag Stopped 2014-04-05 18:42:53.064 Test App[75322:a0b] Dragging from source to destination. 2014-04-05 18:42:53.116 Test App[75322:a0b] *** Terminating app due to uncaught exception 'NSRangeException', reason: '*** -[__NSArrayM objectAtIndex:]: index 8 beyond bounds [0 .. 7]' *** First throw call stack: ( 0 CoreFoundation 0x017635e4 __exceptionPreprocess + 180 1 libobjc.A.dylib 0x014e68b6 objc_exception_throw + 44 2 CoreFoundation 0x017044e6 -[__NSArrayM objectAtIndex:] + 246 3 Test App 0x00006c41 -[I3CollectionToRearrangeableTableViewController droppedOnDstAtIndexPath:fromSrcIndexPath:] + 449 4 Test App 0x00013d00 -[I3DragBetweenHelper handleDragFromSrcStoppedInDstAtPoint:] + 768 5 Test App 0x000122fa -[I3DragBetweenHelper handleDragStopped:] + 1130 6 Test App 0x00011bc7 -[I3DragBetweenHelper handlePan:] + 279 7 UIKit 0x005cce8c _UIGestureRecognizerSendActions + 230 8 UIKit 0x005cbb00 -[UIGestureRecognizer _updateGestureWithEvent:buttonEvent:] + 383 9 UIKit 0x005cd56d -[UIGestureRecognizer _delayedUpdateGesture] + 60 10 UIKit 0x005d0acd ___UIGestureRecognizerUpdate_block_invoke + 57 11 UIKit 0x005d0a4e _UIGestureRecognizerRemoveObjectsFromArrayAndApplyBlocks + 317 12 UIKit 0x005c7148 _UIGestureRecognizerUpdate + 199 13 UIKit 0x0029319a -[UIWindow _sendGesturesForEvent:] + 1291 14 UIKit 0x002940ba -[UIWindow sendEvent:] + 1030 15 UIKit 0x00267e86 -[UIApplication sendEvent:] + 242 16 UIKit 0x0025218f _UIApplicationHandleEventQueue + 11421 17 CoreFoundation 0x016ec83f __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 15 18 CoreFoundation 0x016ec1cb __CFRunLoopDoSources0 + 235 19 CoreFoundation 0x0170929e __CFRunLoopRun + 910 20 CoreFoundation 0x01708ac3 CFRunLoopRunSpecific + 467 21 CoreFoundation 0x017088db CFRunLoopRunInMode + 123 22 GraphicsServices 0x036b89e2 GSEventRunModal + 192 23 GraphicsServices 0x036b8809 GSEventRun + 104 24 UIKit 0x00254d3b UIApplicationMain + 1225 25 Test App 0x0000411d main + 141 26 libdyld.dylib 0x01d9f725 start + 0 ) libc++abi.dylib: terminating with uncaught exception of type NSException (lldb)

I'm going to close this for now due to the issues associated with it, but I think it demonstrates how fragile this class is at the moment. Maybe we could come back to it after some refactoring.

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

3 participants