CAShapeLayer not going away

Question

I have a class that adds a progress view to a given UIView. I use this in a collection view in its cells. It looks good on the first couple of cells, but after swiping a few times you can see faint marks that should be there. I will attach screenshots of what I mean. Not sure why this is happening?

Here is cellForItemAt

guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: self.cellId, for: indexPath) as? TakeCollectionViewCell else {return UICollectionViewCell()}
let service = TakeProgressView()
let random = Double(indexPath.row) / 10
service.progress = random
service.addView(view: cell.takeProgressView, withParticipants: false)
return cell

where takeProgressView is a UIView implemented in storyboard

Here is the function that adds the view:

func addView(view: UIView, withParticipants: Bool){
        disagreePath.removeAllPoints()
        agreePath.removeAllPoints()
        path.removeAllPoints()
        circlePath.removeAllPoints()
        agreeLayer.removeFromSuperlayer()
        disagreeLayer.removeFromSuperlayer()
        backgroundFill.removeFromSuperlayer()
    
        let width:CGFloat = 270
        let height:CGFloat = 360

        
        let viewX = (view.frame.size.width - width) / 2
        let viewY = (view.frame.size.height - height) / 2
        
        subView = UIView(frame: CGRect(x: viewX - 20, y: viewY, width: width + 40, height: height))

        path = UIBezierPath(ovalIn: CGRect(x: 20, y: 0, width: width, height: height))
        
        let rect = CGRect(x: 60, y: 40, width: width - 80, height: height - 80)
        
        circlePath = UIBezierPath(ovalIn: rect)
        path.append(circlePath)
        path.usesEvenOddFillRule = true
        
                
        backgroundFill = CAShapeLayer()
        backgroundFill.path = path.cgPath
        backgroundFill.lineWidth = 40.0
        backgroundFill.fillRule = .evenOdd
        backgroundFill.fillColor = #colorLiteral(red: 0.9371728301, green: 0.9373074174, blue: 0.9371433854, alpha: 1)
        backgroundFill.opacity = 0.9

        
        disagreePath = UIBezierPath(ovalIn: CGRect(x: 30, y: 10, width: width - 20, height: height - 20))
        disagreeLayer = CAShapeLayer()
        disagreeLayer.path = disagreePath.cgPath
        disagreeLayer.fillColor = UIColor.clear.cgColor
        disagreeLayer.strokeColor = #colorLiteral(red: 1, green: 0.4117892087, blue: 0.4117360711, alpha: 1)
        disagreeLayer.strokeStart = 0.0
        disagreeLayer.strokeEnd = CGFloat(1 - progress)
        disagreeLayer.lineWidth = 20.0
        disagreeLayer.lineCap = .round
        
        backgroundFill.addSublayer(disagreeLayer)
        
        agreePath = UIBezierPath(ovalIn: CGRect(x: 50, y: 30, width: width - 60, height: height - 60))
        agreeLayer = CAShapeLayer()
        agreeLayer.path = agreePath.cgPath
        agreeLayer.fillColor = UIColor.clear.cgColor
        agreeLayer.strokeColor = UIColor.uStadium.primary.cgColor
        agreeLayer.strokeStart = CGFloat(1 - progress) + 0.02
        agreeLayer.strokeEnd = 0.98
        agreeLayer.lineWidth = 20.0
        agreeLayer.lineCap = .round
        
        
        backgroundFill.addSublayer(agreeLayer)
        subView.layer.addSublayer(backgroundFill)
        view.addSubview(subView)
    }

Cells are reused, so each time you add a subview... So either keep one, or remove the previously added before... — Larme, Aug 31 '21 at 15:29
right but wouldn't removing the layers suffice? at the top of the function — Noah Iarrobino, Aug 31 '21 at 15:31
The issue is that you're doing `view.addSubview(subView)` each time. So each time, you are adding a new `TakeProgressView()` on it. Plus, all the path should be empty since you are creating a new instance of `TakeProgressView` each time... Dirty fix? `cell.view.forEach{ ($0 as? TakeProgressView)?.removeFromSuperview() }` — Larme, Aug 31 '21 at 15:33
Even that dirty fix won't work, because subview is a `UIView` not a `TakeProgressView`. That's so unclear on why you use really use `TakeProgressView`. — Larme, Aug 31 '21 at 15:40
TakeProgressView is just a class that takes in a UIView, and adds the progress view to it — Noah Iarrobino, Aug 31 '21 at 15:41
In `cell.takeProgressView`, there is only the progress? If so, `cell.takeProgressView.forEach { $0.removeFromSuperview() }` — Larme, Aug 31 '21 at 15:45
My apologies for the confusion. TakeProgressView is a class that facilitates the addition of a the progress view. In that function what happens is I take in the UIView I want to add it to, I then create a new UIView where I put the progress layers on top of, then I add that subview to the UIView parameter — Noah Iarrobino, Aug 31 '21 at 15:48
cell.takeProgressView also contains a UILabel that I cannot remove, otherwise it crashes when trying to set the text — Noah Iarrobino, Aug 31 '21 at 15:50
You should decouple the adding of views and layers (which you should do during `init`) from the configuration of those views and layers (which you should initiate from `cellForItemAt`). If necessary, your cell class can reset itself in `prepareForReuse`. — Rob, Aug 31 '21 at 15:51

score 2 · Accepted Answer · answered Aug 31 '21 at 15:58

Cells (in UITableView or UICollectionView) are reused.

Your code is too much coupled, you should change the architecture of it.

If I understood the logic:

TakeCollectionViewCell should have a property TakeProgressView:

class TakeCollectionViewCell: UICollectionViewCell {
    var takeProgress = TakeProgressView()
    ...

}

So your code should be then:

guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: self.cellId, for: indexPath) as? TakeCollectionViewCell else {return UICollectionViewCell()}
let service = TakeProgressView()
let random = Double(indexPath.row) / 10
cell.takeProgress.progress = random
cell.takeProgress.addView(view: cell.takeProgressView, withParticipants: false)
return cell

In func addView(view: UIView, withParticipants: Bool), you only use view for: frame calculation + addSuview:

...
let viewX = (view.frame.size.width - width) / 2
let viewY = (view.frame.size.height - height) / 2
...
view.addSubview(subView)
...

So instead, let's return the subview and give the frame (or even the size since that's what you use):

func progressSubview(with size: CGSize, withParticipants: Bool) -> UIView {
    ...
    // let viewX = (view.frame.size.width - width) / 2
    let viewX = (size.width - width) / 2
    // let viewY = (view.frame.size.height - height) / 2
    let viewY = (size.height - height) / 2
    ...
    // view.addSubview(subView)
    return subview
    ...
}

Let's keep that subview as a property of TakeCollectionViewCell and let's use the new method.

class TakeCollectionViewCell: UICollectionViewCell {
    var subProgressView: UIView?
    ...
    func updateProgressView(with progress: Double, withParticipants: Bool) {
        takeProgress.progress = progress
        subProgressView = takeProgress.progressSubview(with: self.takeProgressView.size, withParticipants: withParticipants)
    }
}

Now let's override the reuse:

class TakeCollectionViewCell: UICollectionViewCell {
    override func prepareForReuse() {
        super.prepareForReuse()
        subProgressView?.removeFromSuperview()
    }
}

In cellForRow should be now:

guard let cell = collectionView.dequeueReusableCell(withReuseIdentifier: self.cellId, for: indexPath) as? TakeCollectionViewCell else {return UICollectionViewCell()}
let random = Double(indexPath.row) / 10
cell.updateProgressView(with: random, withParticipants: false)
return cell

Additional rework may be done, but that should be a start...

CAShapeLayer not going away

1 Answers1