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

[Feature request]Add ReadOnlyBindableReactiveProperty #234

Closed
runceel opened this issue Jul 14, 2024 · 7 comments
Closed

[Feature request]Add ReadOnlyBindableReactiveProperty #234

runceel opened this issue Jul 14, 2024 · 7 comments

Comments

@runceel
Copy link

runceel commented Jul 14, 2024

@neuecc Are there any plans to introduce a read-only option for BindableReactiveProperty?
Related: runceel/ReactiveProperty#487 (comment)

This request was made in the GitHub issue mentioned above. In my opinion, this feature would be beneficial if added to the core of R3, so I documented it in this issue.

@neuecc
Copy link
Member

neuecc commented Jul 15, 2024

I agree.
It seems better to have complex things like SubscriptionNode management standardized within R3.
I'd like to implement this!
Once it's completed, I'll likely request a review.

@neuecc
Copy link
Member

neuecc commented Jul 26, 2024

It was difficult to prepare a ReadOnlyBindableReactiveProperty as an alternative implementation of a BindableReactiveProperty.
in v1.2.1, creating an interface IReadOnlyBindableReactiveProperty, but I wonder if that will be okay?
This is not a generic type, but just an object Value {get;}, so it's just an interface for exposing it in XAML.

To be honest, I would be happier if people just used BindableReactiveProperty as it is, ignoring the fact that it is mutable.

@runceel
Copy link
Author

runceel commented Jul 31, 2024

@neuecc Thank you. I have reviewed it and it works fine for XAML (I checked both WPF and WinUI3). However, while it serves well as an interface for XAML, I believe having generic versions would be beneficial for other use cases. Therefore, are there any technical blockers preventing the addition of generic interfaces such as IReadOnlyBindableReactiveProperty<T> and IBindableReactiveProperty<T>?

@neuecc
Copy link
Member

neuecc commented Aug 1, 2024

@runceel
I thought there was a problem with the interface, so I only used non-generic types, but it was all in my head.
I added a generic version in #240.
I also added EnableValidation to the interface, as I thought it would be possible to enable validation without any problems.
If it looks okay, I'll release it, but what do you think?

@runceel
Copy link
Author

runceel commented Aug 5, 2024

@neuecc
Thank you! I have reviewed it, and it looks good. However, the binding system of WPF is reflection-based, so even when using IReadOnlyBindableReactiveProperty<T>, TwoWay binding works.
WinUI 3 uses a code generation-based binding system, so this implementation works fine.

In my opinion, creating a custom implementation for the read-only option would cover all cases, but the current implementation strikes a good balance between functionality and cost.

@neuecc
Copy link
Member

neuecc commented Aug 5, 2024

Thank you for reviewing.
I also tried to completely Hide it by wrapping, but due to event propagation and other factors, it seems like it would cause unnecessary complications.
Since Observable<T> is an abstract class, IReadOnlyBindableReactiveProperty<T> cannot chain operators.
Therefore, I added AsObservable().
ac5974d
(Also, I realized that the return value of TおReadOnlyBindableReactiveProperty<T> wasn't IReadOnly in the first place...!)

@neuecc
Copy link
Member

neuecc commented Aug 6, 2024

I've released in 1.2.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants