mit-cml / appinventor-sources

MIT App Inventor Public Open Source
http://appinventor.mit.edu/appinventor-sources/
Apache License 2.0
1.48k stars 2.07k forks source link

Visiblity of `@SimpleProperty` getters and setters #2537

Open shreyashsaitwal opened 3 years ago

shreyashsaitwal commented 3 years ago

Describe the bug

If the userVisible property of a setter type @SimpleProperty is set to false, it's corresponding getter will also be invisible in the components block panel. The same happens with a setter if it's getter is userVisible = false.

Affects

Expected behavior

The visibility of a setter should not affect that of a getter and vice versa. Now, one might suggest simply not creating, for example, the setter block if it is to be kept invisible from the user, but that doesn't really help in case you need a @DesignerProperty. For a @DesignerProperty, a corresponding setter type @SimpleProperty is necessary.

Steps to reproduce

  1. Create an extension with a pair of getter and setter @SimpleProperty. For example:

    // Setter -- Should not be visible to the users
    @SimpleProperty(userVisible = false)
    public void Foo(int i) { /* ... */ }
    
    // Getter -- Should be visible to the users
    @SimpleProperty(userVisible = true)
    public int Foo() { /* ... */ }
  2. Import the extension in AI2. Neither the setter nor the getter for Foo will be visible in extension's block panel.
shreyashsaitwal commented 3 years ago

If someone can confirm this is not an intended behavior but in fact a bug, I'd like to fix it.

pavi2410 commented 3 years ago

https://groups.google.com/g/app-inventor-open-source-dev/c/vcg4a-EIeK0/m/dbSWG9arAgAJ

ewpatton commented 3 years ago

The semantics currently of the userVisible field is that it controls the visibility of the property as a whole w.r.t. the blocks. However, I can see that we may need the ability to control setters and getters independently. Do you want to put in a PR for this? We'll also need to audit the existing usages to make sure that both the getter and setter match to keep the existing behavior for those usages.

shreyashsaitwal commented 3 years ago

Do you want to put in a PR for this?

Sure!

We'll also need to audit the existing usages to make sure that both the getter and setter match to keep the existing behavior for those usages.

I'm not sure if that's what you mean, but how properties affect the behavior of components shouldn't really be affected by fixing this. Theoretically, we would only need to do some minor changes to the component processor's logic.

Also, I'd like to bring to your notice that there might be some blocks which would end up being visible by this change. These are the blocks that were previously invisible because of their corresponding getter/setter,

ewpatton commented 3 years ago

Also, I'd like to bring to your notice that there might be some blocks which would end up being visible by this change. These are the blocks that were previously invisible because of their corresponding getter/setter,

Yes, this is why we need to go through and audit the existing usages. If a component sets only one userVisible to false with the expectation both blocks won't appear, then the corresponding getter/setter will need its visibility corrected.