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

Better naming etc. in Rust crate #5882

Open
Enyium opened this issue Aug 21, 2024 · 1 comment
Open

Better naming etc. in Rust crate #5882

Enyium opened this issue Aug 21, 2024 · 1 comment
Labels
a:language-rust Rust API and codegen (mO,mS) breaking change Anything that probably require a semver bump

Comments

@Enyium
Copy link
Contributor

Enyium commented Aug 21, 2024

I find some parts of Slint's API to not properly follow Rust's conventions. Here are some things that stood out to me:

  • Some names are unnecessarily long and redundant:
    • MapModel::source_model() (also on similar types) should just be source()
    • The Model trait could have more concise names:
      • tracker() instead of model_tracker()
      • "row" could be established as a placeholder term for a data item managed by the model, which would shorten row_data() to row() and set_row_data() to set_row().
        • When there's a need to refer to a row by index (rarer), simply the word "index" should be used (like VecModel already does in parameter names): FilterModel::unfiltered_row() becomes FilterModel::unfiltered_index(), SortModel::unsorted_row() becomes SortModel::unsorted_index().
          • This would also have to be consistently applied to parameter names.
      • row_count() could become len(). This would also affect user types that implement Model and that may not have "Model" in their name. But a model is inherently a list, and user types implementing Model can be expected to have this flair. There's also already the generally phrased iter() on the trait. (If required, an alternative would be num_rows(), which I'd prefer over something with "count".)
  • It isn't bad that you can create empty instances of ModelRc and VecModel via default(). But there should also be a parameter-less new() constructor. GPT-4o agrees that, if it makes sense for a type to have a parameter-less constructor, it should be named new(). Only if the type needs parameters in every constuctor may new() have parameters. Constructors like the current ModelRc::new() should be named from_...() (see "conversion constructors" in Rust's API guidelines).
  • The fact that the constructor named like a conversion constructor VecModel::from_slice() returns something other than a VecModel is just inconsistent. Shouldn't users rather call .into(), like Rust always requires devs to be explicit?
  • The Rust API guidelines say: "The get_ prefix is not used for getters in Rust code." But property getters use that prefix. I understand that using no prefix/suffix at all makes name conflicts with other methods possible that generated types may provide. Maybe Slint could use a prop_ prefix instead? This could also be seen as adhering to Rust's philosophy of being explicit. For consistency, it should then probably also be set_prop_...().
  • The Global trait has a get() function to fetch a component's global-singleton instance. The Rust API guidelines say: "The get naming is used only when there is a single and obvious thing that could reasonably be gotten by a getter. For example Cell::get accesses the content of a Cell." Global::get() doesn't get a "single and obvious thing", but is dependent on a component. As far as I know, getInstance() is a popular name in general for the instance getter of a singleton. Rust's avoidance of a get_ prefix would shorten this to instance(). If you go after this list of programming abbreviations, inst() would be a suitable, common abbreviation.

To make for a seamless transition, you could duplicate the functions where the ones with the old names call through to the ones with the new names, which would contain the current implementations. The functions with the old names then get a #[deprecated] attribute with a message saying what is to be used instead and that the function will be removed in Slint v2. This way, users get warnings with nice messages what to replace what with, while Slint v1 is still the current major version.

EDIT:

  • Timer also has the default()/new() problem.
  • I think the no-frame attribute on Window in .slint code should be phrased positively instead of negatively, so maybe has-frame with a default of true (although built-in has-... properties always seem to be out-properties). Positive phrasing should generally be preferred to avoid possible double-negatives.
@ogoffart ogoffart added breaking change Anything that probably require a semver bump a:language-rust Rust API and codegen (mO,mS) labels Aug 21, 2024
@ogoffart
Copy link
Member

Thanks for the suggestion.

I think most of it make sense, and it can indeed be done by adding a new and deprecating the old one. Although i don't think that can be done for trait method

row_data() to row()

Not sure about this one. row() could be mistaken with the row number.

property getters use that prefix. I understand that using no prefix/suffix at all makes name conflicts with other methods possible that generated types may provide. Maybe Slint could use a prop_ prefix instead?

I think it's fine to break the guideline in this case.

This was referenced Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:language-rust Rust API and codegen (mO,mS) breaking change Anything that probably require a semver bump
Projects
None yet
Development

No branches or pull requests

2 participants