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

Textarea fields in usermods #4217

Open
wants to merge 1 commit into
base: 0_15
Choose a base branch
from
Open

Conversation

obar
Copy link

@obar obar commented Oct 22, 2024

A textarea with a label ending in > will be presented as a textarea, allowing for multi-line text.

It's a small change, I think it opens up possibilities for usermods that need more than just a single line but don't need their own custom settings page.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please respect indentation of existing code.

Other consideration is that textarea fields allow huge amounts of data to be entered by user. ESP is not well suited for such task and content may either be trimmed or rejected entirely. This may be baffling for users as it will work for some texts but not others.

I do not approve.

@DedeHai
Copy link
Collaborator

DedeHai commented Oct 23, 2024

I agree, this needs to be done properly i.e. limit to a size that does not overflow any buffers.
Other than that: am I right in assuming that this tries to solve a currently non-existing use case? If so adding code 'just for convenience' is not a good idea due to the stringent flash memory restrictions.

@softhack007
Copy link
Collaborator

softhack007 commented Oct 23, 2024

I think the usecase question from @DedeHai is a very valid point.
We already have a type for "text" which is good for one-line text (e.g. MQTT topic name). With textarea fields, we could have multi-line input like you mom's postal address including phone number and a birthday congratulation letter. Or a complete GIF PNG file encoded as octet-stream.

a) it may create new issues, as the textblock must be copied into a buffer (stack?) for processing, it must be added to the global JSON object (heap), and then written out to cfg.json while preserving line breaks and special characters including ", [ and :. Finally - if the textblock is saved in cfg.json - it must be read in each time we read the config file (heap, filesystem, read/write performance, ws communication overhead).

b) how would I create such a text area for usermod settings?

Today we typically do this in a usermod:

    uint8_t soundSquelch = 8;
    void addToConfig(JsonObject& root) override {
      JsonObject top = root.createNestedObject(FPSTR(_name));
      JsonObject cfg = top.createNestedObject(FPSTR(_config));
      cfg[F("squelch")] = soundSquelch;
   }
   
   bool readFromConfig(JsonObject& root) override {
      JsonObject top = root[FPSTR(_name)];
      bool configComplete = !top.isNull();
      configComplete &= getJsonValue(top[FPSTR(_config)][F("squelch")], soundSquelch);
      return configComplete;

What's necessary to create a textarea? String is not enough, I assume?

@softhack007 softhack007 added discussion backburner low priority compared to other issues labels Oct 23, 2024
@obar
Copy link
Author

obar commented Oct 23, 2024

Good point about indentation. There's a mix of tabs and spaces in that chunk of the text file and I should take a closer look at how it's done here, I just trusted my text editor to figure it out.

It's true that a lot of text could be entered in a textarea, but the same is true for input type="text" as well, both have a maxlength that can be specified but that's up to the browser to enforce. It's still a String, just one with \n included. There are already textareas in the stock UI for power users (like people activating usermods), when configuring presets. If a usermod would make use of this feature in a way that's potentially risky, I agree with the three of you: there should be a warning and maybe even guard rails.

My use case: I've got a usermod I'd like to contribute that uses a textarea for a reasonable string length, where the use of newlines to delimit makes it legible. You can see the usermod in action in #showcase on Discord, I just posted a clip a few minutes ago!

I messed up this pull request in other ways than the tabs regardless (ended up making a new branch on main and not 0_15), and I should also include the npm-generated files, as well as an addition to the Example usermod... maybe a new pull request is in order! Will resubmit soonish and I hope I can win you over on the UI benefit of a tastefully used textarea ;)

@obar
Copy link
Author

obar commented Oct 24, 2024

Didn't need to open a new pull request in the end, amended to fix the tabs/spaces issue @blazoncek pointed out, and put in an example as @softhack007 suggested. Looks like npm doesn't touch the stylesheet and the different branch doesn't matter in this case. Thanks all for the help and suggestions :)

A textarea with a label ending in `>` will be presented as a textarea,
allowing for multi-line text.
@DedeHai
Copy link
Collaborator

DedeHai commented Oct 25, 2024

I do not see the benefit. While I like what you did in your usermod, would this be an acceptable workaround?

  • add a config field to your UM with "number of groups" (or get the number of groups some other way)
  • add a one line input field for each group (dynamically)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backburner low priority compared to other issues discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants