Skip to content

Conversation

@darshanparajuli
Copy link
Collaborator

While asMutableState() extension already exists, it works with String and does not preserve selection ranges. TextController.asMutableTextFieldValueState() returns a mutable state of TextFieldValue, making it easy to integrate TextController with text fields.

…xtFieldValue

This makes it easy to use it with text fields while persisting selection state.
@rjrjr
Copy link
Collaborator

rjrjr commented May 1, 2025

  • Could you also update the existing sample usages of the String-based one to use this instead? When I sketched that last month everything worked nicely, tests continued to pass.

  • Keith argued that we should keep both since the String one is a simpler alternative. WDYT about deprecating the old one instead?

@rjrjr
Copy link
Collaborator

rjrjr commented May 1, 2025

Here's where I played with the samples: #1291 Oh, nevermind, I guess I never pushed that.

@darshanparajuli
Copy link
Collaborator Author

Keith argued that we should keep both since the String one is a simpler alternative. WDYT about deprecating the old one instead?

As I understand it, the primary purpose of TextController.asMutableState() is to be used with text fields in compose. Therefore, I think String version can be deprecated in favor of TextFieldValue.

If you only care about String, then you just access .text property:

var foo by fooTextController.asMutableTextFieldValueState()
doStuff(foo.text) // String

Also, I think you mostly care about wanting a String when reading from it rather than writing to it, right? Especially since the write sources are text fields, in which case we do care about selections. Worst case:

var foo by fooTextController.asMutableTextFieldValueState()
SomeTextField(
  value = foo.text,
  onValueChanged = { newString -> foo = TextFieldValue(newString, TextRange(newString.length)) }
)

I admit ☝️ is less ergonomic, but I imagine there are very few use cases like that.

@darshanparajuli darshanparajuli marked this pull request as ready for review May 1, 2025 23:04
@darshanparajuli darshanparajuli requested review from a team as code owners May 1, 2025 23:04
@rjrjr
Copy link
Collaborator

rjrjr commented May 1, 2025

That all makes sense to me.

  • Could you go ahead and deprecate the old one in this PR?

@darshanparajuli
Copy link
Collaborator Author

darshanparajuli commented May 1, 2025

That all makes sense to me.

  • Could you go ahead and deprecate the old one in this PR?

done!

@rjrjr
Copy link
Collaborator

rjrjr commented May 1, 2025

You'll need to update the api spec:

./gradlew workflow-core:apiDump  workflow-ui:core-android:apiDump  workflow-ui:core-common:apiDump workflow-ui:internal-testing-android:apiDump workflow-ui:internal-testing-compose:apiDump workflow-ui:compose:apiDump workflow-ui:compose-tooling:apiDump workflow-runtime:apiDump workflow-testing:apiDump

@darshanparajuli
Copy link
Collaborator Author

You'll need to update the api spec:

./gradlew workflow-core:apiDump  workflow-ui:core-android:apiDump  workflow-ui:core-common:apiDump workflow-ui:internal-testing-android:apiDump workflow-ui:internal-testing-compose:apiDump workflow-ui:compose:apiDump workflow-ui:compose-tooling:apiDump workflow-runtime:apiDump workflow-testing:apiDump

Thanks! forgot about that.

@rjrjr
Copy link
Collaborator

rjrjr commented May 2, 2025

Sorry the CI jobs weren't running, they should from now on.

@rjrjr rjrjr enabled auto-merge May 2, 2025 15:50
@rjrjr rjrjr merged commit bb7541c into square:main May 2, 2025
43 checks passed
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

Successfully merging this pull request may close these issues.

2 participants