[Core] Run only one forceSizeUpdate per 16 milliseconds

[AndreiMisiukevich]

Description of Change

fixes #2039

Changed:

• object Cell => OnForceUpdateSizeRequested

Behavioral Changes

Run only one forceSizeUpdate per 16 milliseconds

[adrianknight89]

Seems like indentation is out of place here and on other lines as well.

[AndreiMisiukevich]

@adrianknight89 sorry, it was my gap.. I had to uncheck "converting tabs to spaces"

[jassmith]

build

[jassmith]

I dont understand the point of this? Screen refresh rates can be greater than 60hz

[jassmith]

Compile error here:

Cells\Cell.cs(23,41): error CS0117: 'Task' does not contain a definition for 'CompletedTask' [C:\agent_work\2\s\Xamarin.Forms.Core\Xamarin.Forms.Core.csproj]

[AndreiMisiukevich]

Fixed this error by replacing Task.CompletedTask by Task.FromResult(true)

[AndreiMisiukevich]

@jassmith I can't answer, nevertheless I noticed, that the implementation doesn't correspond to presented comment - "don't run more than once per 16 milliseconds"

look through initial code
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/Cells/Cell.cs#L211-L218

So, I aligned code according to presented comment.

[AndreiMisiukevich]

Now, I hope, it will be built successfully

[jassmith]

I apologize, I was skipping through these notifications very fast and failed to properly read the diff. I remember why this is rate limited now. Its SUPER expensive :P

[jassmith]

build

[rmarinho]

@AndreiMisiukevich unit test failed

[hartez]

@AndreiMisiukevich I do not understand why the original version would run more than once every 16 milliseconds. Are you worried about this being called from multiple threads?

[AndreiMisiukevich]

@hartez yes, exactly. If write smth like code below

	class VCell : ViewCell
	{
		protected override void OnTapped()
		{
			base.OnTapped();

			foreach(var task in Enumerable.Range(0, 10).Select(i => new Task(ForceUpdateSize)))
			{
				task.Start();
			}
		}
	}

[jassmith]

Calling ForceUpdateSize from a non-UI thread is not supported.

[hartez]

@AndreiMisiukevich @jassmith I am guessing that most handlers of that event need to run on the main thread. Perhaps it would make more sense to marshal all the calls to OnForceUpdateSizeRequested onto the main thread in the first place. Something like:

public void ForceUpdateSize()
{
	if (_nextCallToForceUpdateSizeQueued)
		return;

	if ((Parent as ListView)?.HasUnevenRows == true)
	{
		Device.BeginInvokeOnMainThread(() => {
			_nextCallToForceUpdateSizeQueued = true;
			OnForceUpdateSizeRequested();
		});
	}
}

[AndreiMisiukevich]

Okay, i see.
But I think it would be better if we wait only necessary time (without default 16 milliseconds waiting every call)

I added new commit. see it above

[jassmith]

Thats not a bad point. Restarting your builds again. You probably should just null check that Task instead of Task.Delay(0) it, that is just waste.

[AndreiMisiukevich]

so @jassmith, what do you think about Device.BeginInvokeInMainThread(OnForceUpdateSizeRequested) as @hartz said?

[jassmith]

I dont want us to auto-marshal everything no. Supporting out of UI thread calls in just this one area will cause us more confusion than its worth.

[AndreiMisiukevich]

Colleagues, I think these changes aren't very important, are they?
Should I close/discard this PR?

[rmarinho]

Does it show any visual change' like is the list faster , smoother ?

[rmarinho]

We will not be taking this at this time.

[karlingen]

@AndreiMisiukevich @rmarinho Why is this closed? This is still an issue. See #2735