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

Adds String Functions #6657

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

TheColorRed
Copy link
Contributor

@TheColorRed TheColorRed commented Oct 25, 2024

// Sub string
slice("Hello World World", 1, 3);
slice-by-len("Hello World World", 1, 3);

// Replace strings
replace-first("Hello World", "Hello", "Goodbye");
replace-last("Hello World", "Hello", "Goodbye");
replace-nth("Hello World", "o", "0", 2);
replace-all("Hello World", "o", "0");

// Trim strings
trim("    Hello World    ");
trim-start("    Hello World    ");
trim-end("    Hello World    ");

// Contains
contains("Hello World", "Hello");
starts-with("Hello World", "Hello");
ends-with("Hello World", "World");

@TheColorRed TheColorRed changed the title String functions Adds String Functions Oct 25, 2024
@TheColorRed TheColorRed marked this pull request as ready for review October 25, 2024 21:53
@Enyium
Copy link
Contributor

Enyium commented Oct 26, 2024

I don't think their names should be derived from JavaScript functions at the cost of intuitiveness.

  • If having a replace function that replaces only one the first occurence, it shouldn't be named replace(), but, e.g., replace-first().
  • The difference between substring() and substr() is also not readily obvious, especially if you're not using JavaScript. First, they should probably (like others) be type-associated functions (usable after value.). Then, these may be better names: slice-by-range(), slice-at-by-length(). (Because it means something else there, the term "slice" in this context may not spill into Slint's Rust library.)
  • includes() should probably become contains(). According to GPT-4o, the term "contains" is used more often in programming languages. On https://rosettacode.org/wiki/String_matching, you find many occurrences of contains(, but not of includes( (the JavaScript code is implemented differently for some reason). (BTW, it's also the choice in Rust.)

@Enyium
Copy link
Contributor

Enyium commented Oct 26, 2024

len is common in Rust, but Slint didn't set the precedent of abbreviating wildly. Its models, e.g., have the property length.

@TheColorRed
Copy link
Contributor Author

@Enyium I have made all the requested changes, any other feedback?

@Enyium
Copy link
Contributor

Enyium commented Oct 26, 2024

  • Are these functions now already usable using the syntax my-string.replace("\n", " "), or only using replace(my-string, "\n", " ")? It's desirable to be able to use the first syntax, and to avoid global-namespace pollution. This applies to all of these functions.
  • I'd use SharedString::new() instead of SharedString::default(). As I remarked in Better naming etc. in Rust crate #5882, Slint's API should generally be designed more like the conventions established in std in this regard.
  • You're extending the Rust lib's SharedString API with slice[_by_len](). But the term "slice" already means something else in a Rust context (in a technical way) and shouldn't be used in another sense. If you're extending the Rust interface, you should generally consider how the same would be done with the non-Slint std types. I think, functions made available in .slint code don't necessarily need to be made available in the Rust lib, if the std types also don't offer an equivalent and require you to do it differently. Slicing a String using &my_string[start..end], e.g., doesn't yield a String, but &str, which you can, if required, convert into and owned version yourself. Even if it may not be technically true, I think you can think of slicing a String as first dereffing to a &str representing the whole string, then taking a subslice of that, which is again a &str. And Slint's SharedString already derefs to &str, allowing you to do with it what std offers, including .replace(), e.g. ref_str.replace() returns a String, but as I remarked in impl From<String> for SharedString shouldn't unnecessarily clone (Rust) #6597, converting it into a SharedString again should ideally become cheap in the future.
  • You aren't observing UTF-8 char boundaries; indices could lie in the middle of UTF-8 char byte sequences. It would need to be defined what the meaning of indices in Slint string functions should be. It could be byte indices like in Rust, in which case the supplied indices should maybe be floored and/or ceiled to the nearest UTF-8 char boundary. But perhaps, it would also be better to define them as char indices, which would require you to use Rust functions like chars() or char_indices() in the implementation.

@ogoffart
Copy link
Member

Thanks a lot for the contribution! This is a solid patch, and we appreciate the effort you've put in.

I'm curious about the motivation behind adding these string functions directly to Slint. Since the Slint language is meant to separate UI and logic, with the latter handled by native code, I'm wondering what use cases you have in mind that would require performing these operations within Slint itself. Some of these functions could be useful, but I’m not sure if we need the full range you've provided.

A few specific points:

  • Some of the functions take indices as parameters (UTF-8 codepoints). This might be unintuitive, especially for JavaScript developers, and it leads to issues if an index falls in the middle of a multi-byte character. We should avoid panics in such cases. It may be better to leave out these functions for now.
  • You've added these under a Strings namespace, but I believe they would be more consistent as member functions (similar to to-float). There is no need for the Strings namespace
  • Tests are necessary to ensure the functionality is robust — could you add some in tests/cases? You can use tests/cases/types/string_to_float.slint as an example.
  • The PR extends the Rust SharedString API. I think we should limit these additions to only those functions that match the standard std::String API.

Overall, this is a great patch, but I’d recommend pausing further work until we’re convinced these functions are a strong fit for Slint’s goals. Please let us know if you have use case or motivation for these functions.

Thanks again!

@TheColorRed
Copy link
Contributor Author

TheColorRed commented Oct 27, 2024

@ogoffart The biggest reason I wanted to add this is because I wanted a replace function to format input easily without leaving the slint language. In my app, I have an input that I want to only accept numbers, but display a %, so I wanted to replace the % when the user focuses the input and then add it back after it is edited, and I felt like adding a callback to the rust app seemed like overkill. The other functions I just decided to add as well since it was easy enough to add replace.

Here is some pseudo code:

LineEdit {
    input-type: number;
    text: "100%";
    changed has-focus => {
        if (self.has-focus) {
            // Remove the % when focused
            self.text = replace-all(self.text, "%", "");
        }
    }
    accepted => {
        // Add it back when the input is accepted
        self.text = self.text + "%";
    }
}

@ogoffart
Copy link
Member

Thanks for the justification.
I think indeed a replace function make sense. But only one variant (replace replace all instances, i don't think we need replace-first or other)

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.

3 participants