Re: Constraining properties

[Stefan Westerfeld <stefan space twc de>]

   Hi!

On Sun, Apr 07, 2019 at 02:45:23AM +0200, Tim Janik wrote:
> as requested, I've prototyped a basic macro that helps with property setting by:
>
> * constraining the property value (for int, float, enum)
> * testing for value changes
> * pushes undo (untested)
> * emitting notification events
> * actually setting the constrained value
>
> Usage looks like this:
>
> void
> SongImpl::musical_tuning (MusicalTuning tuning)
> {
>   if (!prepared())
>     {
>       BseSong *self = as<BseSong*>();
>       if (APPLY_IDL_PROPERTY (self->musical_tuning, tuning))
>         {
>           SfiRing *ring;
>           for (ring = self->parts; ring; ring = sfi_ring_walk (ring, self->parts))
>             bse_part_set_semitone_table ((BsePart*) ring->data, bse_semitone_table_from_tuning 
> (self->musical_tuning));
>         }
>     }
> }
>
> You can find the code here:
>       https://github.com/tim-janik/beast/tree/wip/constrain-properties
>
> Please review the changes the branch introduces and let me know if that 
> corresponds to the ideas you had about constraining properties.

Thanks for implementing this, this looks like the functionality I wanted to
have. I've committed my own fixes on top of that

  https://github.com/swesterfeld/beast/tree/constrain-properties-plus

So mainly I'll comment my changes here:

| commit 12b9c83d7fe9423ddcfecc4cad7ea9f937a0bfb7
| Author: Stefan Westerfeld <stefan space twc de>
| Date:   Sun Apr 28 16:31:10 2019 +0200
| 
|     BSE: bseitem: fix APPLY_IDL_PROPERTY() bugs for non primitive types
|     
|     - if property was changed => update it
|     - call push_property_undo just like for primitive types
|     
|     Signed-off-by: Stefan Westerfeld <stefan space twc de>
| 

(1) I think this is just an obvious correction that spells out what you wanted
to do for non-primitive types.

| commit 976efd46f5954b4f89d0c75c5e8ec4809fc87118
| Author: Stefan Westerfeld <stefan space twc de>
| Date:   Sun Apr 28 16:37:57 2019 +0200
| 
|     BSE: bseitem: check if undo is needed in APPLY_IDL_PROPERTY()
|     
|     If "skip-undo" hint for the property is set, don't call push_property_undo.
|     
|     Signed-off-by: Stefan Westerfeld <stefan space twc de>
| 

(2) Here we have the question of how to handle undo. I think we should at least
allow using the macro for the non undo case. Without this fix, undo is done
unconditionally. I saw two possible solutions: one would be an extra
APPLY_IDL_PROPERTY_NO_UNDO macro, which would make the skip-undo hint
meaningless, and make whether a property is undoable or not an implementation
detail (not part of the interface).

I didn't do it this way, because we may have properties in plugins that are
not undoable, where the skip-undo hint would be very helpful, so I thought
it would be more consistent to do it like that everywhere.

| commit 3e37306f5181b50da213693e7edc577de9f9dcc1 (HEAD -> constrain-properties-plus, 
origin/constrain-properties-plus)
| Author: Stefan Westerfeld <stefan space twc de>
| Date:   Sun Apr 28 16:39:22 2019 +0200
| 
|     BSE: bsesong: use APPLY_IDL_PROPERTY for loop_enabled
|     
|     Signed-off-by: Stefan Westerfeld <stefan space twc de>
| 

(3) This is more a suggestion of how to deal with locks: I'm not sure if I like
my own code here; the question is what happens if we have to take a lock before
setting a value. We could do (like in the commit).

  bool value = self->loop_enabled_SL;

  if (APPLY_IDL_PROPERTY (value, enabled))
    {
      BSE_SEQUENCER_LOCK ();
      self->loop_enabled_SL = value;
      BSE_SEQUENCER_UNLOCK ();
    }

effectively using a temp value to be able to lock before we write the new
value, or we could do

  BSE_SEQUENCER_LOCK ();
  APPLY_IDL_PROPERTY (self->loop_enabled_SL, enabled);
  BSE_SEQUENCER_UNLOCK ();

which looks more sane, but at the expense of taking the lock for a slightly
longer duration. I'm not sure if reading a variable without lock is even
safe (its done often in bsesong.cc); in principle there might be platforms
which could provide half-written values.

The last option here would be not to use the macro at all, and preserve
the manually written code which handles stuff just fine.

(4) As last comment (without code), I'd like to point out that your macro
doesn't live up to your own standards. You wrote

http://gtk.10911.n7.nabble.com/tim-janik-beast-Smarter-property-set-implementation-74-td94379.html#a94525

| The above actually needs to be changed to the following order:
| 
| 
|   if (int (self->denominator) != val)
|     {
|       val = CLAMP (val, 1, 256);
|       // ...
|       notify (prop);
|     }
| 
| Here's why:
| 
| a) A user enters -77 at the UI, which is reflected by a text entry or slider.
| b) The UI sends -77 through the IDL layer.
| c) The impl refuses -77 and CLAMPs it to 0, or maybe retains the old value, e.g. 10.
| d) Because the value the UI displays (-77) and the value that was sent (-77) is
|     different from the resulting value in the impl (e.g. 0 or 10), a change
| notification
|     *must* be sent back to the UI (and other monitoring instances), in order for the
|     UI to be notified that the value being displayed has to be updated by querying
|     the impl again.
| 
| Now, whether the impl CLAMPs or ignores the setting is entirely up to the
| implementation.

So if I understand it correctly, if you enter -77 at the GUI, you want to send
a notification in every case. However, your implementation will return false
early for this case. Look at the FIXME below.

  /// Constrain and assign property value if it has changed, emit notification.
  template<class T> bool
  apply_idl_property (T &lvalue, const T &cvalue, const String &propname)
  {
    if (std::is_integral<T>::value || std::is_floating_point<T>::value || std::is_enum<T>::value)
      { // avoid value copy for non primitive types
        T rvalue = cvalue;
        StringVector kvlist = find_prop (propname);
        if (!constrain_idl_property (rvalue, kvlist))
          return false;
        if (lvalue == rvalue)
          return false; // FIXME
        if (apply_idl_property_need_undo (kvlist))
          push_property_undo (propname);
        lvalue = std::move (rvalue);
      }
    else
      {
        if (lvalue == cvalue)
          return false;
        if (apply_idl_property_need_undo (find_prop (propname)))
          push_property_undo (propname);
        lvalue = cvalue;
      }
    auto lifeguard = shared_from_this();
    exec_now ([this, propname, lifeguard] () { this->notify (propname); });
    return true;
  }

   Cu... Stefan
-- 
Stefan Westerfeld, http://space.twc.de/~stefan