slint-ui / slint

Slint is a declarative GUI toolkit to build native user interfaces for Rust, C++, or JavaScript apps.
https://slint.dev
Other
16.93k stars 566 forks source link

public, private and readonly properties #191

Closed ogoffart closed 1 year ago

ogoffart commented 3 years ago

We want to be able to declare the properties as public, private, or protected. Currently all top level properties are public.

The motivation is not only for performance (since knowing that a property cannot be accessed or written from the outside would allow to optimize things) but also for code hygiene. For example, do we really want to allow changing property in states that could also be set from the outside.

Updated suggestion:

We classify the property in the different category:

Example of property declarations:

   in property <brush> background: blue; 
   out property <bool> pressed: touch-area.pressed;
   in-out property <int> value <=> slider.value;
   property <int> past_value: 32;  // this is private

We will also recommend inheriting from a "base" element that doesn't have any properties to it so that it would not be exposed.

Unresolved question:

Original suggestion * Requiring `public`/`private`/`readonly` annotation when defining properties * Require a `public`/`private`/`readonly` annotation when defining components to specify the default access of inherited property * Allow to change the visibility of inherited property with the `public foo;`/`private bar;`/`readonly buz;` ``` Button := private Rectangle { // None of Rectangle's properties can be used by users of Button readonly property pressed; private property internal_state; } Button1 := public Rectangle { // All properties of Rectangle are readable/writable to users of Button } Button2 := readonly Rectangle { // All of Rectangle's properties become read-only to users of Button, but // Button2 itself can still assign bindings color: pressed ? blue : red; } Foo := private Button { // Should be allowed to override and create a new property (but we can also make it forbidden for now) private property internal_state: "Foo"; } Button := private Rectangle { // We want the Rectangle's color to be public public color; } Button := public Rectangle { // Just hide the Rectangle's color private color; readonly border-color; } ``` Questions: - Do we want to force the use of such a keyword, or should there be a default, and if there is a default, which one? - Properties that are not at the top level don't matter since they can only be private, so no keyword required. Maybe non top level property does make sense? - Syntax: Are these keyword the best we can come up? Should we be able to elide the `property` keyword: ``` private foo; public bar; readonly xxx; ```
bwalter commented 3 years ago

My 2 cents here: while it's definitely an improvement, the experience has shown that such "code hygiene" issues are better solved by avoiding inheritance in favor of composition. I'm wondering how it would be properly solved for 60fps but this is definitely one of the architectural issues which make QML projects hard to maintain.

ogoffart commented 3 years ago

@bwalter: In our implementation we are using composition, so this issue is merely about the syntax and, as you've identifier, hygiene to allow developers to define a good encapsulation for their components. In your opinion, what would be the alternative syntax?

bwalter commented 2 years ago

I have just noticed that my answer is still missing, sorry!

Thanks for taking care of this issue. My proposal would be to completely get rid of this syntax:

MyGreatButton := Rectangle {
  ...
}

in favor of:

component MyGreatButton {
  ...
  Rectangle {
    ...
  }
}

Because the usage of the Rectangle is an implementation detail, it should be fully encapsulated and the users of MyGreatButton should not have to know:

As a result, we would never mix new and existing (hidden) properties, which is a clear simplification and makes the "public" interface explicit (no hidden variable behind the inherited component).

And as a bonus, we do not need the 2nd and 3rd suggestions that you gave ("Require a public/private/readonly annotation when defining components to specify the default access of inherited property" and "Allow to change the visibility of inherited property with the public foo;/private bar;/readonly buz;") which is another clear simplification :)

ogoffart commented 2 years ago

@bwalter thanks for the great feedback. This approach is also solving another syntax issue we had with the := which we also used for global, struct, and soon enum, but we don't really need it.

So we could deprecate the := syntax, and allow to declare stuff like so:

struct MyStruct {  foo: string, bar: int }; 
global MyGlobal { 
   property <int> foo;
   property <int> bar: 42;
}
component MyButton { // or is `element` a better name?
   // public properties
   property <int> text;
   property <bool> pressed;  // <- i would still like to annotate that this is an "output" property (read-only)
   Rectangle {
       property <string> my_private: "42";
       TouchArea {
             property <string> another_private: "42";
             // ....
       }
   }

   // Would this be an error?  maybe not the two just have the same geometry.
   Rectangle { /*..*/ }
}
ogoffart commented 2 years ago

Note that i always fellt that declaring property in inner element (like my_private or another_private in the previous comment) a bit awkward. We juse support it because QML does that, but i know @tronical feels differently. One problem for example is that what happens if TouchArea gets a another_private property in a future version of SixtyFPS. (we also have more problems if that happens, but that's one of them)

each element anyway gets the geometry and other builtin properties that every element have (like x, height, colspan, ...)

bwalter commented 2 years ago

Fully agree with getting rid of the custom properties in inner elements. It's still time to simplify a lot which is a win-win for both maintainers and users (+ easier documention ;)).

Another idea for huge simplification would be in my opinion to tackle issues like bi-directional bindings and (accidental) binding removal (e.g. when imperatively setting a bound property) which were for many Qt developers a big source of confusion and made QML development non-idiomatic...

tronical commented 2 years ago

Great feedback and suggestions! I love the consistency that it brings. I don’t mind the lack of inner properties. I like them because of the locality they help maintain when components become large, but that’s probably minor compared to the advantages you’re both outlining. Count me in :)

glaebhoerl commented 2 years ago

(accidental) binding removal (e.g. when imperatively setting a bound property)

I think it'd be great if there were a strict separation between (a) readonly properties, whose RHSes may be dynamic and which get reevaluated automatically, vs. (b) mutable properties, which can be manually overwritten in imperative code, but whose initializer RHS may only be a 'static value', i.e. it can't reference other properties, only literals. Readonly properties could still reference mutable ones as usual, so I don't think this loses much expressiveness, you just have to be up-front about what the purpose of a given property is.

bwalter commented 2 years ago

Simple proposal using read-only by default and a new 'imperative' keyword:

component MyButton {
    property<int> foo: bar * 2 + 3  // read-only with bindings
    property<int> foo: imperative { bar * 2 + 3 }  // read/write, imperatively set/initialized
    property <int> foo  // must be set (either with bindings or imperatively)
    property <int> foo: imperative  // must be imperatively set/initialized
    ...
}

I think it'd be great if there were a strict separation between (a) readonly properties, whose RHSes may be dynamic and which get reevaluated automatically, vs. (b) mutable properties, which can be manually overwritten in imperative code, but whose initializer RHS may only be a 'static value', i.e. it can't reference other properties, only literals. Readonly properties could still reference mutable ones as usual, so I don't think this loses much expressiveness, you just have to be up-front about what the purpose of a given property is.

ogoffart commented 1 year ago

We discussed that again and will finally tackle change in the language soon. I split the component declaration in another issue: https://github.com/slint-ui/slint/issues/1682 I updated the description to mention the new keyword we discueesed: input output and inout

ogoffart commented 1 year ago

There is now an implementation of this in master behind the SLINT_EXPERIMENTAL_SYNTAX feature gate.

After implementing this, it turns out that inout and input are two keywords that looks really similar to eachother and are easy to confuse. We might want to rename inout to something else.

glaebhoerl commented 1 year ago

I like the new access control keywords, that was indeed another source of ambiguity and bugs on the QML codebase I worked on (we adopted ext_foo as a naming convention for input properties as a mitigation).

As a somewhat orthogonal matter to access control, you're not as concerned apparently about property bindings getting inadvertently broken by mutation?

ogoffart commented 1 year ago

As a somewhat orthogonal matter to access control, you're not as concerned apparently about property bindings getting inadvertently broken by mutation?

I hope this will fix most cases, since one cannot set input property from within the component, and one cannot set binding to output property outside of the component.

Could you give an example of problematic code? Do you have a suggestion on how to fix it?

ogoffart commented 1 year ago

it turns out that inout and input are two keywords that looks really similar to eachother and are easy to confuse.

Maybe we could write that input output property<...> foo with two words

tronical commented 1 year ago

That seems like a reasonable compromise to me.

ogoffart commented 1 year ago

@glaebhoerl So you would like to have an extra qualified for "assignable" or something like that? or maybe that would be a more an annotation on the binding.

Rectangle { 
   final background: blue; // can't be assigned anywhere else
   final property <length> xxx: 42px + parent.x;    // same
}

Or would you prefer if there was an assignable label, or replaceable that mean the opposite?

I wonder if that is not a bit too much.

bjorn commented 1 year ago

After implementing this, it turns out that inout and input are two keywords that looks really similar to eachother and are easy to confuse. We might want to rename inout to something else.

Did you consider renaming the other two instead, so you have in, out and inout / in out? I think "input" and "output" are already rather verbose, and from C# many people are already used to simply in and out on function parameters with similar meaning.

jturcotte commented 1 year ago
  input output property<string> text;
  property<int> internal-state; //Private by default

Just my feeling, but I think that for learning the language and quick prototyping, having to remember and type input output everywhere is tedious. For performance and maintenance it's bad, but for new users and development I think that input output is a better default.

Also I find that calling internal-state a property a bit misleading, but since there are no local variable in bindings this is what users need to use for any intermediate state. So overall, as a random user with only a part of the picture, I'd favor property<string> text; defaulting to input output (thus the long term could be removed from the spec), and I'd rename private properties to something like variable<int> internal-state.

ogoffart commented 1 year ago

@bjorn you're right. in, out, and inout might be better. In general we try to avoid abbreviation because readability is more important than verbosity. But it is true that these are already used keywords. I'll think about it.

@jturcotte This is not about performance (we actually already auto-detect things that aren't change in most cases), but this is about self-documenting code and correctness by avoiding mistake. And also being explicit about what properties should be shown for editing in a design tool. I think the most common properties are not input output but most properties in my experience are just input. But the default is very important for correctness, that's why it should be private. But you have a point saying that it is not really a property.

ogoffart commented 1 year ago

We're going with in, out and in-out in https://github.com/slint-ui/slint/pull/1824

ogoffart commented 1 year ago

The 0.3.2 already has the new keywords

tachyon-ops commented 1 year ago

Docs and examples do not reflect this (for instance the tile game)

tronical commented 1 year ago

In the next release it will: https://slint-ui.com/snapshots/master/docs/tutorial/rust/polishing_the_tile.html :-)