UIButton.when breaks in reusable cells #132

Closed
codezomb opened this Issue Aug 30, 2012 · 9 comments

Comments

Projects
None yet
3 participants

I could be completely using this wrong, but when trying to use this syntax inside a call for a reusable cell, it throws an exception after scrolling. The app dies either silently, or with an unknown selector error. Also, on another note - it would be nice if it would automatically pass sender.

This is from using GMGridView, but it's the same idea.

def GMGridView(aGridView, cellForItemAtIndex:index)
  cell = aGridView.dequeueReusableCell
  cell = GridViewCell.new unless cell

  cell.button.tap do |button|
    button.tag = index
    button.selected = selected?(index)
    button.when(UIControlEventTouchUpInside) do
      select_photo(cell.button)
    end
  end

  cell
end

Here's the method it should be calling, which works fine until you scroll the view:

def select_photo(sender)
  if sender.selected?
    @selected_photos.delete(sender.tag)
  else
    @selected_photos << sender.tag
  end
  sender.selected = !sender.selected?
end
Member

dmarkow commented Aug 30, 2012

Is UIControlEvenTouchUpInside a typo in your issue, or does that typo actually exist in your code? (it should be Event, not Even)

Member

dmarkow commented Aug 30, 2012

A couple questions:

  1. If you completely remove the button.when block, you can scroll perfectly fine?
  2. What happens if you replace the select_photo(cell.button) with something like puts "test".
  3. Do you have a sample repository or something we could test this on? I'm not familiar with the GMGridView library, but when I tried making a custom UITableViewCell with a button on my end, my button.when block worked fine and scrolled fine.

I've put up a demo repository here: https://github.com/gitt/MyGridTest
Note: If the xib files don't compile, just move them into the base resources path. I've patched the motion script to include nibs from subdirectories, but the my pull request hasn't been accepted yet.

This is the error I get after scrolling... Before scrolling button presses work as expected. This leads me to believe the issue is in the reuse.

2012-08-30 23:18:39.188 MyGridTest[21023:c07] -[UIControlTargetAction call]: unrecognized selector sent to instance 0xe58a980
2012-08-30 23:18:39.189 MyGridTest[21023:c07] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[UIControlTargetAction call]: unrecognized selector sent to instance 0xe58a980'
*** First throw call stack:
(0x9ca022 0x35acd6 0x9cbcbd 0x930ed0 0x930cb2 0x9cbe99 0x124814e 0x12480e6 0x12eeade 0x12eefa7 0x12ee266 0x126d3c0 0x126d5e6 0x1253dc4 0x1247634 0x2b53ef5 0x99e195 0x902ff2 0x9018da 0x900d84 0x900c9b 0x2b527d8 0x2b5288a 0x1245626 0x10f10 0x10aa5)
((null))> rake aborted!ng an exception
Command failed with status (1): [DYLD_FRAMEWORK_PATH="/Applications/Xcode.a...]
Is UIControlEvenTouchUpInside a typo in your issue, or does that typo actually exist in your code? (it should be Event, not Even)

This was an error when I cleaned up the paste for posting. Sorry about that, it's fixed now.

Member

dmarkow commented Aug 31, 2012

Thanks, that's very helpful.

It looks like when you scroll a cell away, the block you provided gets garbage collected, but the cell's button still keeps it as a target. When you scroll the cell back into the view, your code adds the block/target again, so the button now thinks it has two actions to perform when it's clicked: the initial block (which no longer exists), and the second block from when the cell was scrolled back into the view.

So when you click the button, it tries to call the first block, which doesn't exist, which is why you get random errors (either a crash with no error whatsoever, or something like [UIControlTargetAction call]: unrecognized selector sent to instance 0xe58a980 (though it won't always be UIControlTargetAction)).

Manually retaining the block in the when method (block.retain) works, but then when you scroll away and scroll back up, clicking the button performs the selector multiple times.

Instead, removing all existing targets within the when method BubbleWrap adds seems to "fix" the issue, but I don't think we necessarily want to do that, as some users may want to link multiple targets to a single button.

class UIControl
  def when(events, &block)
    # Remove any existing targets so the new one we add below is the only one.
    if allTargets.count > 0
      removeTarget(@callback[events], action:'call', forControlEvents: events)
    end
    @callback ||= {}
    @callback[events] = block
    addTarget(@callback[events], action:'call', forControlEvents: events)
  end
end

So, you may be able to add the code above to your project and get it working for now (as long as you don't have multiple when blocks for a single control), but I'll continue investigating -- it may be an upstream bug in RubyMotion.

Member

dmarkow commented Aug 31, 2012

More details: The reason your manual button.addTarget call doesn't cause the problem is because that call is pointing to a specific target/selector (in this case, self and "select_photo:"). So even if you call button.addTarget again on the same button when the cell re-appears, it doesn't add a second/duplicate action because the target and selector are identical to the existing action.

However, the way BubbleWrap works, it's setting a block as the target, and call as the selector (to perform block.call, thus executing your block code). But when you scroll away and back, the block is defined all over again, so the addTarget call thinks it's being given a different block, not the "same" one, and it adds it a second action (and a third, and a fourth, etc. after a bunch of scrolling back and forth). But at this point, all but the most recently added block has been garbage collected already.

Unfortunately, I don't think it's possible with Ruby to reliably determine if a whole block/proc is identical to another one. (In some circumstances, you can do something like proc1 == proc2 and get true, but it varies a lot based on what code the proc contains).

So that leaves two options, neither of them perfect.

  1. In the when method, change @callback[events] to an array and allow multiple blocks for a specific event. However, in this case, after some scrolling, select_photo(button) would be called multiple times with a single click.
  2. When adding the target in the when method, remove any existing targets for the same event. However, this would eliminate the ability to have multiple button.when(...) do blocks on a single button.

@clayallsopp Since you're the one who originally added the when method, do you have any input?

Member

dmarkow commented Aug 31, 2012

As just discussed in #124, a similar bug happens by putting a when block within a viewWillAppear method. The second time the view is displayed, it creates the same condition of an invalid first target.

My preference is option 2, as I believe this will cause less unexpected surprises to our users. Maybe we can add an "append" option to when, defaulting it to false, to allow the user to specify additional actions instead of replaceing the actions when necessary?

Member

clayallsopp commented Sep 3, 2012

I like the append: compromise. More often than not you'll only have one action for a given control event mask, so seems like a reasonable default.

dmarkow closed this in 5aa044c Sep 5, 2012

Member

dmarkow commented Sep 5, 2012

Fixed. There is now an append option if you truly want to execute multiple blocks:

my_button.when(UIControlEventTouchUpInside, append: true) do
  ...
end

However, it will default to replacing, not appending, which avoids the issue of multiple blocks added whenever a UITableViewCell, UIView, etc. re-appears.

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