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

Text field Rewrite #701

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

AlexKempen
Copy link

@AlexKempen AlexKempen commented Jun 4, 2024

This PR fixes the following bugs/strange behaviors associated with the NumberTextField component and the RenamableTitle component:

  1. Previously, activating a NumberText field for editing would result in the cursor always jumping to the end of the text. This PR tweaks the behavior to instead select the entire text.
  2. This PR corrects a bug where moving the mouse over other elements in the path tree while edits to the NumberTextField were pending would result in the edits being suddenly discarded.
  3. This PR resolves Increase text field number precision to 3 decimals #693 by allowing all number fields to display values up to three decimals of precision (as needed) and technically allows users to enter values with arbitrary precision (although only three decimals of precision will be displayed).

Other minor tweaks/changes this PR makes include:

  1. NumberTextField is now a controlled component, taking and rendering a value directly.
  2. NumberTextField no longer takes a controller.
  3. NumberTextField now takes a lowerBound as an argument to clean up its usage.
  4. The nav grid edit dialog has been broken out into its own widget.
  5. The arrow key increment logic has been greatly simplified.

This is my first time working with Flutter/Dart, so definitely don't hesitate to point out any issues/flaws you see! I'm especially curious to see if there's a good way to combine the common code between NumberTextField and RenamableTitle - I took a crack at it with an abstract class extending State, but it was pretty ugly. Not sure if there's a better way extending TextField or something else.

@github-actions github-actions bot added the GUI Changes to the PathPlanner GUI label Jun 4, 2024
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jun 5, 2024
@mjansen4857
Copy link
Owner

mjansen4857 commented Jun 7, 2024

Appreciate the contribution, but I have a few requests:

  • The general structure of the NumberTextField widget should remain as it was. Having the option to pass in a controller is important for making more custom functionality if needed, and taking in a string for the value of the text field is more useful than passing in the number directly since it allows the parent to display it as either an int or double, and choose whatever precision is needed. It also doesn't need to store its own value since that will be passed to it when the parent is rebuilt.
  • Using an enum for the lower bound functionality seems unnecessary, as you can just pass in a num to represent the lower bound, then return max(lowerBound, value) when submitted. The default lower bound can be double.negativeInfinity so that it effectively has no bound by default. Also, if lower bound functionality is added, then you might as well add upper bound as well.
  • It doesn't seem like RenamableTitle or NumberTextField need to be a stateful widget, as you are only ever calling setState to change a variable that does not actually affect the rendering of the widget. So, you don't need to use setState to change it. Changing this back to a StatelessWidget is essentially free performance.

No rush on any of this, since I likely won't merge any other PRs until after I get #694 finished

@AlexKempen
Copy link
Author

AlexKempen commented Jun 7, 2024

Per this stack overflow thread, using a TextEditingController with an InputField can only be done in a StatefulWidget. I believe this was the cause of the bug mentioned above where pending input field edits would suddenly get discarded when moving the mouse over certain other elements of the GUI.

With that in mind, it also isn't idiomatic to pass the controller to the widget, as the lifecycle of the controller is managed by the StatefulWidget directly (via the onInit and dispose methods). Note this doesn't actually break any existing usage patterns since the component now takes a value directly (which might've gotten a little lost in the git compare when I broke out the nav grid dialog, oops).

I think for the current usage it's slightly better to take a number, as determining the number of decimals to display is something that the component is good at abstracting. We could add a displayDecimals parameter if we want to control the maximum number of decimals on a component by component basis.

The main reason I used an enum was to avoid having to invent an arbitrarily-small-but-not-zero number to replace the >0 case. I have seen other software do that, however, so I don't have a problem making that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file GUI Changes to the PathPlanner GUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase text field number precision to 3 decimals
2 participants