Update cell when tintColor changes (without crashing) #1492

karlvr · 2018-03-30T08:37:36Z

This is a do-over of #1439, which caused crashes reported #1447.

As in #1439 we update the row when the tint colour changes. This should capture all uses of tintColor in the various update methods throughout the framework.

This PR adds explicit detection of the potential for an infinite loop, which occurs when a cell changes its tintColor in the update method. This change is in Cell.swift.

This PR also removes all the instances where components change the tintColor in their update method. I don't believe that our cells should set their own tintColor as this prevents them from participating in the cascade of tintColor from their parent views. In CheckRow.swift and ListCheckRow.swift I have changed to using tintAdjustmentMode, which I believe is the correct way to achieve what those cells are trying to achieve. Note that there are many other cells in the project that use tintColor dimmed to 30% alpha to indicate a disabled state. This is not what the tintAdjustmentMode does; I believe instead it sets them to grey, but who knows what else it might do. It may be a separate task to make other components match up to the behaviour of tintAdjustmentMode—I feel like it's best to match that, as it's Apple's way ;-)

This PR also corrects the tintColor behaviour of StepperRow.

You can easily test the tintColor behaviour by setting the tintColor on the view in the example app view controllers, such as RowsExampleViewController. If you do that before and after this PR you should see what this PR is fixing!

This reverts commit f873035.

Setting the tintColor explicitly means this component will ignore future parent tintColor changes, which means it won’t work nicely in an app that uses tintColors.

… the tintColor

mats-claassen

We should test this before merging to make sure there are no problems

mats-claassen · 2018-04-04T12:21:22Z

Source/Rows/CheckRow.swift

 if row.isDisabled {
- tintColor = UIColor(red: red, green: green, blue: blue, alpha: 0.3)
+ tintAdjustmentMode = .dimmed


From the Apple docs (of tintAdjustmentMode):

When this property’s value changes (either by the view’s value changing or by one of its superview’s values changing), -the system calls the tintColorDidChange() method to allow the view to refresh its rendering

So it does not break the loop. We should test which version looks better

@mats-claassen thank you for looking at this! The intention of this change was to allow the component to accept future tintColor updates. If you set the tintColor explicitly then you're closed off to changes from your superview, so I think that it's best to use the tintAdjustmentMode to handle this case.

You can see an example of this in the form rows example in Eureka when you set yourself a tintColor. By the time the form rows get told about it these rows have already set their own based on the default tintColor.

So, this change isn't about breaking the loop, it's about making tintColor flowing down work.

mats-claassen · 2018-04-04T12:43:08Z

I have tested on iPhone 7 and it works fine but the reported problems were mainly iPhone X so it would be helpful if someone could test it on iPhone X

sena1or · 2018-04-24T23:03:50Z

Hello guys @karlvr @mats-claassen @mtnbarreto ,

My bad to say that, just that issue is happening for me still.
Particularly, I am setting cell's tintColor at func setup() in my custom cell (I've figured only one just, but seems a bit more affected) and then it go recursively through func update() method.

When I move setting of tintColor into update method, all works well.

So, should be that an issue?

karlvr · 2018-04-25T00:23:32Z

@sena1or thanks for your message. Could you please post the stack trace that demonstrates this issue?

sena1or · 2018-04-25T12:19:24Z

Hi @karlvr

Thank you for the fast response.
Yes, sure, attached to this message

stacktrace.txt

sena1or · 2018-04-25T12:23:52Z

Also, I catch a runtime error when try to set tintColor within init method.
func tintColorDidChange() is trying to access row field, which is referencing to nil at the moment.
The row field will be initialised later, after cell made by provider (See attached screenshot)

karlvr · 2018-04-25T21:22:53Z

@sena1or thank you. So the first stack trace is resulting in a stack overflow, but it seems to be creating lots of new cells. Is that what's happening? I'm guessed that looking at the changing self in the Cell.tintColorDidChange frames.

    frame #390: 0x0000000105b035d9 UIKit`-[UIView setTintColor:] + 109
    frame #391: 0x00000001095942ba CustomUIKit`VerticalTableViewCell.setup(self=0x00007faee2b31600) at VerticalTableViewCell.swift:111
    frame #392: 0x000000010c87d824 Eureka`closure #1 in implicit closure #1 in Row.cell.getter(self=0x00007faedd910f30) at Row.swift:145 [opt]
    frame #393: 0x000000010c875e94 Eureka`Row.updateCell() [inlined] Eureka.Row.cell.getter : Swift.ImplicitlyUnwrappedOptional<A>(self=0x00007faedd910f30) at Row.swift:142 [opt]
    frame #394: 0x000000010c875e6a Eureka`Row.updateCell(self=0x00007faedd910f30) at Row.swift:163 [opt]
    frame #395: 0x000000010c817690 Eureka`Cell.tintColorDidChange(self=0x00007faee2b31000) at Cell.swift:161 [opt]
    frame #396: 0x000000010c8176c7 Eureka`@objc Cell.tintColorDidChange() at Cell.swift:0 [opt]
    frame #397: 0x0000000105efff44 UIKit`-[_UITintColorVisitor _visitView:] + 388
    frame #398: 0x0000000105f00712 UIKit`_UIViewVisitorEntertainVisitors + 106
    frame #399: 0x0000000105eff78c UIKit`_UIViewVisitorRecursivelyEntertainDescendingVisitors + 173
    frame #400: 0x0000000105eff414 UIKit`+[_UIViewVisitor _startTraversalOfVisitor:withView:] + 226
    frame #401: 0x0000000105b03347 UIKit`-[UIView _dispatchTintColorVisitorWithReasons:] + 120
    frame #402: 0x0000000105b035d9 UIKit`-[UIView setTintColor:] + 109
    frame #403: 0x00000001095942ba CustomUIKit`VerticalTableViewCell.setup(self=0x00007faee2b31000) at VerticalTableViewCell.swift:111
    frame #404: 0x000000010c87d824 Eureka`closure #1 in implicit closure #1 in Row.cell.getter(self=0x00007faedd910f30) at Row.swift:145 [opt]
    frame #405: 0x000000010c875e94 Eureka`Row.updateCell() [inlined] Eureka.Row.cell.getter : Swift.ImplicitlyUnwrappedOptional<A>(self=0x00007faedd910f30) at Row.swift:142 [opt]
    frame #406: 0x000000010c875e6a Eureka`Row.updateCell(self=0x00007faedd910f30) at Row.swift:163 [opt]
    frame #407: 0x000000010c817690 Eureka`Cell.tintColorDidChange(self=0x00007faee3881000) at Cell.swift:161 [opt]
    frame #408: 0x000000010c8176c7 Eureka`@objc Cell.tintColorDidChange() at Cell.swift:0 [opt]
    frame #409: 0x0000000105efff44 UIKit`-[_UITintColorVisitor _visitView:] + 388
    frame #410: 0x0000000105f00712 UIKit`_UIViewVisitorEntertainVisitors + 106
    frame #411: 0x0000000105eff78c UIKit`_UIViewVisitorRecursivelyEntertainDescendingVisitors + 173
    frame #412: 0x0000000105eff414 UIKit`+[_UIViewVisitor _startTraversalOfVisitor:withView:] + 226
    frame #413: 0x0000000105b03347 UIKit`-[UIView _dispatchTintColorVisitorWithReasons:] + 120
    frame #414: 0x0000000105b035d9 UIKit`-[UIView setTintColor:] + 109
    frame #415: 0x00000001095942ba CustomUIKit`VerticalTableViewCell.setup(self=0x00007faee3881000) at VerticalTableViewCell.swift:111

I don't understand yet why we're getting a new cell each time through this. Does VerticalTableViewCell.swift do anything to change the cell on the row? That seems unlikely... hmm.

sena1or · 2018-04-25T21:45:51Z

Hi @karlvr

When execution flow becomes to VerticalTableViewCell.setup() first time, the cell of related row (i.e. VerticalRow) field isn't initialised yet. Then my code changing the tintColor property (in that VerticalTableViewCell.setup() function). That is calling tintColorDidChange(), which is trying to access the cell field (that is still isn't fully initialised). Getter see that it doesn't have a _cell object (see Row.swift:144) and try to instantiate appropriate cell (again VerticalTableViewCell). And here we become to VerticalTableViewCell.setup() second time, after that process repeats infinitely.

Second problem, that I mentioned is also related to instantiation process. And all of them cause by the change of intColor on setup stage. I am not sure how to better to fix that, because intention is still not clear for me, my apologies for that.

Thanks,
Gleb

sena1or · 2018-04-25T21:50:17Z

The reason why _cell isn't initialised is the flow can't get out from closure at first access of getter.

karlvr · 2018-04-25T21:56:35Z

@sena1or thanks for the pointer to Row.swift:144. I see the problem.

Could you modify the Eureka source in your project (perhaps in the Pods directory?) and try out a change like this?

After Cell.swift:95 add private var cellUpdated = false
After Cell.swift:127 add cellUpdated = true (this is at the end of the update method, and will be line 128 after the change in 1).
Change Cell.swift:159 (now line 161) from if !updatingCellForTintColorDidChange to if !updatingCellForTintColorDidChange && cellUpdated

I'm not sure this is the best solution, but could we see if this solves the infinite recursion issue?

karlvr · 2018-04-27T03:30:43Z

It seems to me that we could add a method to Row that is updateCellIfExists that first checks if there is a cell, and only calls updateCell if there is. That would get around the issue of an infinite loop through the cell creation, and is a bit more of a general solution.

sena1or · 2019-03-11T20:07:45Z

@karlvr Hello from 2019,

Apologies from being late :-(

I fixed this locally year ago and come back again after dependency update.
So, I would suggest slightly different solution at Row.swif:144 (public var cell: Cell!)

        var result: Cell?
        return _cell ?? result ?? {
            result = cellProvider.makeCell(style: self.cellStyle)
            result.row = self
            result.setup()
            _cell = result
            return _cell
        }()
    }

and get rid of updatingCellForTintColorDidChange in Cell.swift

What would you say?

Thanks!

karlvr · 2019-03-15T22:10:01Z

@sena1or no problem, I know the feeling! I'm now no longer close to this issue so I can't judge this so well. Could you explain what you've done?

sena1or · 2019-03-15T22:24:51Z

Correct version would be

            private var _result: Cell?
	    /// The cell associated to this row.
	    public var cell: Cell! {
	            return _cell ?? _result ?? {
	                    _result = cellProvider.makeCell(style: self.cellStyle)
	                    _result!.row = self
	                    _result!.setup()
	                    _cell = _result
	                    _result = .none
	                    return _cell
	            }()
	    }

@karlvr the main idea is to return a cell object, even if setup method didn't called.

karlvr added 6 commits March 30, 2018 21:18

Revert "Revert "Update cell when tintColor changes""

89e9d83

This reverts commit f873035.

Use tintAdjustmentMode instead of setting the tintColor

3b7c448

Setting the tintColor explicitly means this component will ignore future parent tintColor changes, which means it won’t work nicely in an app that uses tintColors.

Prevent infinite recursion in the case where an update method updates…

15f5fbf

… the tintColor

StepperRow: fix handling of tintColor

5dd4910

ButtonRow: remove double textColor setting

d58727d

Fix detection of infinite recursion in tintColorDidChange

9ced06e

karlvr mentioned this pull request Mar 30, 2018

Shouldn't a ButtonCell's tintColor be the same as the window's tintColor by default? #1175

Closed

mats-claassen reviewed Apr 4, 2018

View reviewed changes

mtnbarreto approved these changes Apr 9, 2018

View reviewed changes

mtnbarreto merged commit f9c6eb1 into xmartlabs:master Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update cell when tintColor changes (without crashing) #1492

Update cell when tintColor changes (without crashing) #1492

karlvr commented Mar 30, 2018

mats-claassen left a comment

mats-claassen Apr 4, 2018

karlvr Apr 4, 2018

mats-claassen commented Apr 4, 2018

sena1or commented Apr 24, 2018

karlvr commented Apr 25, 2018

sena1or commented Apr 25, 2018 •

edited

Loading

sena1or commented Apr 25, 2018 •

edited

Loading

karlvr commented Apr 25, 2018

sena1or commented Apr 25, 2018

sena1or commented Apr 25, 2018

karlvr commented Apr 25, 2018

karlvr commented Apr 27, 2018

sena1or commented Mar 11, 2019 •

edited

Loading

karlvr commented Mar 15, 2019

sena1or commented Mar 15, 2019 •

edited

Loading

Update cell when tintColor changes (without crashing) #1492

Update cell when tintColor changes (without crashing) #1492

Conversation

karlvr commented Mar 30, 2018

mats-claassen left a comment

Choose a reason for hiding this comment

mats-claassen Apr 4, 2018

Choose a reason for hiding this comment

karlvr Apr 4, 2018

Choose a reason for hiding this comment

mats-claassen commented Apr 4, 2018

sena1or commented Apr 24, 2018

karlvr commented Apr 25, 2018

sena1or commented Apr 25, 2018 • edited Loading

sena1or commented Apr 25, 2018 • edited Loading

karlvr commented Apr 25, 2018

sena1or commented Apr 25, 2018

sena1or commented Apr 25, 2018

karlvr commented Apr 25, 2018

karlvr commented Apr 27, 2018

sena1or commented Mar 11, 2019 • edited Loading

karlvr commented Mar 15, 2019

sena1or commented Mar 15, 2019 • edited Loading

sena1or commented Apr 25, 2018 •

edited

Loading

sena1or commented Apr 25, 2018 •

edited

Loading

sena1or commented Mar 11, 2019 •

edited

Loading

sena1or commented Mar 15, 2019 •

edited

Loading