Issues with UITableView*

Adam_Stoller · September 12, 2016, 9:42pm

I’m trying to do something that I thought would be rather simple:
Display a list of information in rows of a table (in this case, each row would contain a ‘name’ and a ‘location’ label)
If clicked on once, the row should become selected (highlighted and checked)
If clicked on again (same row), the row should become un-selected (un-highlighted and un-checked)
Alternatively, after clicking and selecting one row, if you click on a different row, the first row should become un-selected and the second row should become selected (i.e.: single-selection list)

For my purposes I created two custom classes:
class ExistingLocationsViewController: UITableViewController
class ExistingLocationTableViewCell: UITableViewCell

I’ve gone through numerous trial-and-error situations based on some combination of:
What I’ve read in books
What I’ve read online in various forums
What I’ve thought made sense intuitively
What I could glean from lots of debugging print statements

The current version of my code seems to satisfy points #1-3, but crashes when attempting to deal with #4 - and I’m not sure that what I’ve done is “good” or “correct” in terms of satisfying the first three points.

I’ve attached the code and the (annotated) debugging log output - the latter gets to a point where I seem to solve #4, but then causes other complications

I’m looking for pointers, advice, suggestions, corrections, etc. While I’m not looking to have someone write my code for me, I’m certainly willing to accept such a contribution. Ideally, when done with this, I hope to have a reasonably clear understanding of what was wrong and what is right.

This is only the bare beginnings of the application I’m trying to write, and it’s rather frustrating to run into such a confusing issue on something that - to me - seems like it should be so simple to implement.

—fish
(Adam Stoller)

debugging_log.pdf (71.7 KB)

ExistingLocationTableViewCell.swift (1.15 KB)

ExistingLocationsViewController.swift (5.15 KB)

LocationObject.swift (566 Bytes)

ole · September 12, 2016, 10:08pm

The current version of my code seems to satisfy points #1-3, but crashes

> when attempting to deal with #4 - and I’m not sure that what I’ve done
> is “good” or “correct” in terms of satisfying the first three points.

I haven't looked at all of your code, but one thing that could definitely cause problems is that you're calling `dequeueReusableCellWithIdentifier` from `tableView(_:didDeselectRowAtIndexPath:)` and `tableView(_:didSelectRowAtIndexPath`). Don't do that.

`dequeueReusableCellWithIdentifier` creates a new cell (or fetches an unused one from the table view's reuse pool). You should only ever call it from inside `tableView(_:cellForRowAtIndexPath:)`.

In `didSelectRow...` and `didDeselectRow...`, you want to ask the table view for the _existing_ cell at the specified index path. Do this with something like:

     if let cell = tableView.cellForRowAtIndexPath(indexPath) {
         // cell found, do something with it
         ...
     } else {
         // No cell exists at this index path.
         // This probably doesn't happen in these callbacks,
         // but you never know.
     }

Note that while `tableView.cellForRowAtIndexPath(indexPath)` looks very much like the data source method you implemented above, it is a different method. This one is a method on `UITableView`, not `UITableViewDataSource`.

Hope this helps,
Ole

Adam_Stoller · September 12, 2016, 10:14pm

Hmm - interesting to know. Unfortunately, if I do that, then I get NO indication of selection when I click on ANY row. Perhaps I need to make some other changes to account for the change in how I get the cell?

—fish
(Adam Stoller)

···

On Sep 12, 2016, at 18:08, Ole Begemann <ole@oleb.net> wrote:

> The current version of my code seems to satisfy points #1-3, but crashes
> when attempting to deal with #4 - and I’m not sure that what I’ve done
> is “good” or “correct” in terms of satisfying the first three points.

I haven't looked at all of your code, but one thing that could definitely cause problems is that you're calling `dequeueReusableCellWithIdentifier` from `tableView(_:didDeselectRowAtIndexPath:)` and `tableView(_:didSelectRowAtIndexPath`). Don't do that.

`dequeueReusableCellWithIdentifier` creates a new cell (or fetches an unused one from the table view's reuse pool). You should only ever call it from inside `tableView(_:cellForRowAtIndexPath:)`.

In `didSelectRow...` and `didDeselectRow...`, you want to ask the table view for the _existing_ cell at the specified index path. Do this with something like:

   if let cell = tableView.cellForRowAtIndexPath(indexPath) {
       // cell found, do something with it
       ...
   } else {
       // No cell exists at this index path.
       // This probably doesn't happen in these callbacks,
       // but you never know.
   }

Note that while `tableView.cellForRowAtIndexPath(indexPath)` looks very much like the data source method you implemented above, it is a different method. This one is a method on `UITableView`, not `UITableViewDataSource`.

Hope this helps,
Ole

ole · September 12, 2016, 10:41pm

Hmm - interesting to know. Unfortunately, if I do that, then I get

> NO indication of selection when I click on ANY row. Perhaps I need
> to make some other changes to account for the change in how I get the
> cell?

You also need to store your cells' selection state someplace outside of the cells themselves. The cells should not be the "source of truth" for the selection state. Otherwise, when you scroll a cell off screen and then scroll it back, it will lose its state.

So you should store the selection state of each table row somewhere alongside your `locationList` array. Maybe as an array of pairs like this:

     var locationList: [(location: LocationObject, selected: Bool)] = [
         (
             location: LocationObject(name: "name-1", address: "addr-1", phone: "phone-1", latitude: 40.0, longitude: -80.1),
             selected: false
         ),
         (
             location: LocationObject(name: "name-2", address: "addr-2", phone: "phone-2", latitude: 40.0, longitude: -80.1),
             selected: false
         )
     ]

There may be better ways to structure your model data, but this should suffice for now.

Then:

1. In `tableView(_:cellForRowAtIndexPath:)`, use the current value of `item.selected` to configure the cell's selection state:

     override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {
         let cell = tableView.dequeueReusableCellWithIdentifier("resultCell", forIndexPath: indexPath) as! ExistingLocationTableViewCell
         let item = locationList[indexPath.row]

cell.nameLabel.text = item.location.name
cell.locationLabel.text = item.location.address

cell.accessoryType = item.selected ? .Checkmark : .None

return cell
}

One note: on iOS, the convention for table views is that rows should generally not remain selected after the user lifts their finger. Adding the checkmark should be enough to show a cell's selection state. I would only set the checkmark and leave `cell.selected` as is (I left it out in the code above).

2. In `didSelectRow...`, toggle the selection state in your model data:

override func tableView(tableView: UITableView, didSelectRowAtIndexPath indexPath: NSIndexPath) {

         // Update model
         let row = indexPath.row
         locationList[row].selected = !locationList[row].selected

To update the UI, you now have two choices. Either ask the table view for the cell at the index path and modify the cell directly:

         // Either do this:
         if let cell = tableView.cellForRowAtIndexPath(indexPath) {
             cell.accessoryType = locationList[row].selected ? .Checkmark : .None
         }

If you do that, I don't think you need to reload the cell explicitly. Alternatively, tell the table view to reload the cell as you are doing now. It will then call `tableView(_:cellForRowAtIndexPath:)` again, which in turn will configure the cell with your model data:

// Or this:
tableView.reloadRowsAtIndexPaths([indexPath], withRowAnimation: .None)

Finally, fade out the cell selection:

tableView.deselectRowAtIndexPath(indexPath, animated: true)
}

3. If you are okay with keeping the cells deselected unless the user's finger is onscreen, you don't need to implement `didDeselectRow...` at all.

(I typed this mostly without help from the compiler as I don't have a Swift 2.x handy, so there may be some errors in the code above.)

Ole

Jon_Shier · September 12, 2016, 10:43pm

UITableView already has the notion of “selected rows” built in, so I’d suggest starting there.

Jon

···

On Sep 12, 2016, at 6:41 PM, Ole Begemann via swift-users <swift-users@swift.org> wrote:

> Hmm - interesting to know. Unfortunately, if I do that, then I get
> NO indication of selection when I click on ANY row. Perhaps I need
> to make some other changes to account for the change in how I get the
> cell?

You also need to store your cells' selection state someplace outside of the cells themselves. The cells should not be the "source of truth" for the selection state. Otherwise, when you scroll a cell off screen and then scroll it back, it will lose its state.

So you should store the selection state of each table row somewhere alongside your `locationList` array. Maybe as an array of pairs like this:

   var locationList: [(location: LocationObject, selected: Bool)] = [
       (
           location: LocationObject(name: "name-1", address: "addr-1", phone: "phone-1", latitude: 40.0, longitude: -80.1),
           selected: false
       ),
       (
           location: LocationObject(name: "name-2", address: "addr-2", phone: "phone-2", latitude: 40.0, longitude: -80.1),
           selected: false
       )
   ]

There may be better ways to structure your model data, but this should suffice for now.

Then:

1. In `tableView(_:cellForRowAtIndexPath:)`, use the current value of `item.selected` to configure the cell's selection state:

   override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {
       let cell = tableView.dequeueReusableCellWithIdentifier("resultCell", forIndexPath: indexPath) as! ExistingLocationTableViewCell
       let item = locationList[indexPath.row]

       cell.nameLabel.text = item.location.name
       cell.locationLabel.text = item.location.address

       cell.accessoryType = item.selected ? .Checkmark : .None

       return cell
   }

One note: on iOS, the convention for table views is that rows should generally not remain selected after the user lifts their finger. Adding the checkmark should be enough to show a cell's selection state. I would only set the checkmark and leave `cell.selected` as is (I left it out in the code above).

2. In `didSelectRow...`, toggle the selection state in your model data:

   override func tableView(tableView: UITableView, didSelectRowAtIndexPath indexPath: NSIndexPath) {

       // Update model
       let row = indexPath.row
       locationList[row].selected = !locationList[row].selected

To update the UI, you now have two choices. Either ask the table view for the cell at the index path and modify the cell directly:

       // Either do this:
       if let cell = tableView.cellForRowAtIndexPath(indexPath) {
           cell.accessoryType = locationList[row].selected ? .Checkmark : .None
       }

If you do that, I don't think you need to reload the cell explicitly. Alternatively, tell the table view to reload the cell as you are doing now. It will then call `tableView(_:cellForRowAtIndexPath:)` again, which in turn will configure the cell with your model data:

       // Or this:
       tableView.reloadRowsAtIndexPaths([indexPath], withRowAnimation: .None)

Finally, fade out the cell selection:

       tableView.deselectRowAtIndexPath(indexPath, animated: true)
   }

3. If you are okay with keeping the cells deselected unless the user's finger is onscreen, you don't need to implement `didDeselectRow...` at all.

(I typed this mostly without help from the compiler as I don't have a Swift 2.x handy, so there may be some errors in the code above.)

Ole
_______________________________________________
swift-users mailing list
swift-users@swift.org
https://lists.swift.org/mailman/listinfo/swift-users

Adam_Stoller · September 12, 2016, 10:43pm

I will respond to this directly to Ole - as I was informed this was not the correct list for such discussions, and I have since unsubscribed and subscribed to the cocoa-dev list instead. Many thanks…

—fish
(Adam Stoller)

···

On Sep 12, 2016, at 18:41, Ole Begemann <ole@oleb.net> wrote:

> Hmm - interesting to know. Unfortunately, if I do that, then I get
> NO indication of selection when I click on ANY row. Perhaps I need
> to make some other changes to account for the change in how I get the
> cell?

You also need to store your cells' selection state someplace outside of the cells themselves. The cells should not be the "source of truth" for the selection state. Otherwise, when you scroll a cell off screen and then scroll it back, it will lose its state.

So you should store the selection state of each table row somewhere alongside your `locationList` array. Maybe as an array of pairs like this:

   var locationList: [(location: LocationObject, selected: Bool)] = [
       (
           location: LocationObject(name: "name-1", address: "addr-1", phone: "phone-1", latitude: 40.0, longitude: -80.1),
           selected: false
       ),
       (
           location: LocationObject(name: "name-2", address: "addr-2", phone: "phone-2", latitude: 40.0, longitude: -80.1),
           selected: false
       )
   ]

There may be better ways to structure your model data, but this should suffice for now.

Then:

1. In `tableView(_:cellForRowAtIndexPath:)`, use the current value of `item.selected` to configure the cell's selection state:

   override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {
       let cell = tableView.dequeueReusableCellWithIdentifier("resultCell", forIndexPath: indexPath) as! ExistingLocationTableViewCell
       let item = locationList[indexPath.row]

       cell.nameLabel.text = item.location.name
       cell.locationLabel.text = item.location.address

       cell.accessoryType = item.selected ? .Checkmark : .None

       return cell
   }

One note: on iOS, the convention for table views is that rows should generally not remain selected after the user lifts their finger. Adding the checkmark should be enough to show a cell's selection state. I would only set the checkmark and leave `cell.selected` as is (I left it out in the code above).

2. In `didSelectRow...`, toggle the selection state in your model data:

   override func tableView(tableView: UITableView, didSelectRowAtIndexPath indexPath: NSIndexPath) {

       // Update model
       let row = indexPath.row
       locationList[row].selected = !locationList[row].selected

To update the UI, you now have two choices. Either ask the table view for the cell at the index path and modify the cell directly:

       // Either do this:
       if let cell = tableView.cellForRowAtIndexPath(indexPath) {
           cell.accessoryType = locationList[row].selected ? .Checkmark : .None
       }

If you do that, I don't think you need to reload the cell explicitly. Alternatively, tell the table view to reload the cell as you are doing now. It will then call `tableView(_:cellForRowAtIndexPath:)` again, which in turn will configure the cell with your model data:

       // Or this:
       tableView.reloadRowsAtIndexPaths([indexPath], withRowAnimation: .None)

Finally, fade out the cell selection:

       tableView.deselectRowAtIndexPath(indexPath, animated: true)
   }

3. If you are okay with keeping the cells deselected unless the user's finger is onscreen, you don't need to implement `didDeselectRow...` at all.

(I typed this mostly without help from the compiler as I don't have a Swift 2.x handy, so there may be some errors in the code above.)

Ole