Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ParameterState: Add scope registration, lock mechanism, ComponentBaseWithState #8683

Merged
merged 18 commits into from
Apr 15, 2024

Conversation

ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented Apr 13, 2024

Description

New syntax to register parameters:

public MudProgressLinear()
{
	using var registerScope = CreateRegisterScope();
	_valueState = registerScope.CreateParameterBuilder<double>(nameof(Value))
		.WithParameter(() => Value)
		.WithChangeHandler(OnParameterChangedShared)
		.WithComparer(DoubleEpsilonEqualityComparer.Default);
	_minState = registerScope.CreateParameterBuilder<double>(nameof(Min))
		.WithParameter(() => Min)
		.WithChangeHandler(OnParameterChangedShared);
	_maxState = registerScope.CreateParameterBuilder<double>(nameof(Max))
		.WithParameter(() => Max)
		.WithChangeHandler(OnParameterChangedShared);
	_bufferValueState = registerScope.CreateParameterBuilder<double>(nameof(BufferValue))
		.WithParameter(() => BufferValue)
		.WithChangeHandler(OnParameterChangedShared);
}

You are not allowed to create more than one scope.
This is necessary for code simplicity:

  1. There is no queue mechanism anymore when you don't have backing-field and do not call explicitly Attach method.
  2. ParameterSet is now readonly allowing to use FrozenDictionary (net8).

The idea is very simple.
The RegisterParameterBuilder<T> is now caching the ParameterStateInternal<T> and every-time the Attach method is called, it returns the same instance.
The backing-fields getting the instance of ParameterState through that Attach, and later on all the ParameterStates will be collected again through the Attach, this is why returning same instance is important.
For the cache we use Lazy, because it's is very cheap, thread safe and the code is very simple, you don't have to worry about many things.

ParameterRegistrationBuilderScope is stashing all the builders (RegisterParameterBuilder<T>), when the scope ends (Dispose) the ParamterSet will collect all the ParameterStateInternal by reading a lazy-collection IEnumerable<IParameterComponentLifeCycle>. ParamterSet is also creating the dictionary lazily.
Therefore any kind of reading in the ParameterSet will initiate the parameters reading (aka initialize the dictionary, so it's auto-locked on first read).

How Has This Been Tested?

New tests, current tests, visually

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added enhancement New feature or request PR: needs review labels Apr 13, 2024
Copy link

codecov bot commented Apr 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.11%. Comparing base (28bc599) to head (5878853).
Report is 52 commits behind head on dev.

❗ Current head 5878853 differs from pull request most recent head c0b1ba8. Consider uploading reports for the commit c0b1ba8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8683      +/-   ##
==========================================
+ Coverage   89.82%   90.11%   +0.28%     
==========================================
  Files         412      418       +6     
  Lines       11878    12046     +168     
  Branches     2364     2365       +1     
==========================================
+ Hits        10670    10855     +185     
+ Misses        681      658      -23     
- Partials      527      533       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ScarletKuro ScarletKuro requested a review from henon April 13, 2024 20:58
@ScarletKuro
Copy link
Member Author

ScarletKuro commented Apr 13, 2024

@henon I can finally say that I really love the current registration mechanism, knowing that everything is now locked, truly readonly and that there is no voodoo magic when the Attach is not called. Also it makes GetState to be more worth (on NET8) since reading TryGetValue is MUCH faster. Also there no more chance to GetState to throw if someone actually managed to call it before the SetParametersAsync and the the old queue didn't register the non attachments, because now everything is done strictly in the ctor scope.

@ScarletKuro
Copy link
Member Author

Added ComponentBaseWithState in this PR

@ScarletKuro ScarletKuro changed the title ParamterState: Add scope registration and lock mechanism ParamterState: Add scope registration and lock mechanism, ComponentBaseWithState Apr 14, 2024
@ScarletKuro ScarletKuro changed the title ParamterState: Add scope registration and lock mechanism, ComponentBaseWithState ParamterState: Add scope registration, lock mechanism, ComponentBaseWithState Apr 14, 2024
@henon
Copy link
Collaborator

henon commented Apr 15, 2024

Awesome! Nice work.

Can we rename CreateParameterBuilder<T>(string) to RegisterParameter<T>(string) ?

It makes the code even more intuitive to read, it s almost like English: using register-scope to register parameter of name "Value" with parameter ... , with change-handler ... , etc.

After:

	using var registerScope = CreateRegisterScope();
	_valueState = registerScope.RegisterParameter<double>(nameof(Value))
		.WithParameter(() => Value)
		.WithChangeHandler(OnParameterChangedShared)
		.WithComparer(DoubleEpsilonEqualityComparer.Default);

And I think we should remove WithName and Attach, they serve no purpose any longer, right?

@henon henon changed the title ParamterState: Add scope registration, lock mechanism, ComponentBaseWithState ParameterState: Add scope registration, lock mechanism, ComponentBaseWithState Apr 15, 2024
@ScarletKuro
Copy link
Member Author

And I think we should remove WithName and Attach, they serve no purpose any longer, right?

is it fine to make Attach internal? Problem is that Attach is used for testing purpose, and I cannot rely on implicit cast because it returns ParameterState while Attach returns ParameterStateInternal and for testing I need the full type. Tho not like making it internal will change much since for us it will be still visible, just not when it will be public API in other projects.

@henon
Copy link
Collaborator

henon commented Apr 15, 2024

No problem, internal is good.

@ScarletKuro ScarletKuro merged commit 3750c02 into MudBlazor:dev Apr 15, 2024
2 checks passed
@ScarletKuro ScarletKuro deleted the state_v5 branch April 15, 2024 22:19
biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants