Re: Smarter property set() implementation (re #74)

[Tim Janik <tim janik gmail com>]

On 14.09.2018 15:28, Stefan Westerfeld wrote:
> > I hear what you mainly want to simplify though, and I think a good chunk of
> > that can be achieved with convenience macros, even if that's less elegant to
> > the trained C++ eye.
> > Assuming we move properties in to the *Impl classes and out of the C structs
> > at some point, this could look like:
> >
> >
> >   void
> >   SongImpl::bpm (int val)
> >   {
> >     const bool changed = UPDATE_UNDO_PROPERTY (bpm_, val);
> >     if (changed)
> >       this->trigger_other_updates();
> >   }
> >
> > With the following impl details, UPDATE_UNDO_PROPERTY(member_,value) would:
> >
> > 1) compare this->bpm_ != value
> > 2) extract aux_info strings for "bpm", to perforrm range constraints
> > 2) push the old value to undo (there could be an UPDATE_PROPERTY variant
> > without undo)
> > 3) notify("bpm"), the string "bpm" is probably best inferred from __func__,
> > since that's the
> >     one IDL generated piece of information that UPDATE_PROPERTY has access to
> > which
> >     perfectly matches the property name.
> >     We would need a couple test cases to assert that the runtime platform
> >     really assigns the property setter name to __func__ correctly, though. But
> > afaik, that
> >     should be true for g++ (linux, windows) and clang (linux, windows, macos).
> >
> > As long as we keep the C structs around, we probably have to live with
> > something like:
> >
> >   BseSong *self = as<BseSong*>();
> >   UPDATE_UNDO_C_PROPERTY(&self->bpm, val);
> >
> > Note that meta info about "bpm" can still be extracted and notifications for
> > "bpm"
> > can still be sent out by this macro, if we infer the property name from
> > __func__.
> I think we should avoid using a macro here, if we can find an elegant C++
> alternative which does what we need. Especially using the property setter
> __func__ to extract the property name and depend on compiler specific internals
> should't be done. This is too magic for my taste.

That's ok, we know the compilers we're supporting, clang and gcc and
__func__ works perfectly for both of them (it's part of the C++ standard
actually).
The reason I suggested using __func__ here is that this takes one
major possibility for typos / human error away.
If we don't automate the notification step, I doubt the whole effort is
worth the hassle of implementing a "generic" setter mechanism.

> On the other hand I think passing the actual variable is too simplistic, as
> often you want to do more than just an assignment. So I suggest with my
> previous example property to do it like this:
>
>   void
>   SongImpl::denominator (int val)
>   {
>     BseSong *self = as<BseSong*>();
>
>     update ("denominator", val, [&]() {
>       self->denominator = val <= 2 ? val : 1 << g_bit_storage (val - 1);
>       bse_song_update_tpsi_SL (self);
>     });
>   }

As I see it, that hardly saves us anything.
We basically remove three lines:
 
   - return_unless (val != bpm_);
   - push_property_undo ("bpm");
   - notify ("bpm");

But add two new ones for a nested lambda construct (which is fairly
complicated in comparison). I.e. saving 1 out of 3 lines isn't worth
the mental overhead and labour involved for adding a mechanism.
Again, if this was actually *helping* with making things less error
prone, I'd be more inclined to consider it, but that got lost with still
requiring the user to duplicate the property name as string.

> With clamp I'm not sure whether we should force range enforcement always to be
> done automatically if you use update() at all, or if not. But we could add two
> more function, namely
>
>   update_clamp ("bpm", val, &bpm_);
>   update_clamp_undo ("bpm", val, &bpm_);

Nah, that's useful only for a fraction of the properties we expose. More below.

> As for whether to parse the range for the range check step (2) from the strings
> or not, it seems to be a matter of taste. You currently already generate
> __aida_get__ so I see no reason not to generate
>
>   bool
>   SongIface::__aida_range__ (const std::string &__n_, Aida::Any& __min_, Aida::Any& __max_) const
>   {
>   ...
>     if (__n_ == "bpm")           { __min_.set (1); __max_.set (1024); return true; }
>   ...
>   }

Here is the reason: clamping the property value into a range is only useful
for a fraction of the (numeric) properties that are being used.

The IDL file mainly covers what you'd usually have in a C++ file as well, the
storage type (int, float, string), the name and the enclosing class.
Optionally, *arbitrary* meta data can be "attached" to a property, it's best
to think of this as a key=value list of strings (because it is),
termed __aida_aux_data__ in aida.hh and provided as a string vector.

The IDL file allows specification of these auxillary key=value string lists, the
C++ binding implementation provides these via an accessor (including
inheritance from base classes), the C++ client API exposes these, as does
the Javascript client API.

That way, we can add *arbitrary* auxillary data in the future, without having to
adapt the middleware and all programming language bindings. E.g. we can add
minimum int/float, maximum int/float, stepping int/float or paging int/float values.
Also we can add valid/invalid charsets for strings (e.g. to constrain date inputs),
add flags like STORAGE, GUI_ONLY, DEPRECATED for all sorts of properties.
We can add unit hints (Hz, %, Cents), add a base for logarithmic scales, etc, etc.

All of these annotations can be added by the IDL authors and optionally be
supported by UI front ends or storage backends, *without* middle layer adaptions.

The __aida_range__ proposal does away with that benefit. And it poses a question
about how many other property kinds warrant extra API. And it causes more
language binding implementation effort, it does that *despite* the actual
information already being available through the existing __aida_aux_data__
vector.

There're two cases in the Beast code base that warrant an extension of the property
accessor API beyond __aida_get__ + __aida_set__, those are:

1) Exposing whether a property is *temporarily* read-only (e.g. during playback);
2) Listing candidates that can be used in the setter, this only applies to
objects though
    (enums are covered by a static enum introspection API).

Both are useful for *dynamic* property behaviour, i.e. the candidate list can change
during runtime, as can the temporary read-only state. At the moment, our old and new
APIs ignore (1) and (2) can be implemented at the BseObject level without needing
middleware support.

> Yes, its tricky because you need to know which string is min and which string is
> max, but on the other hand, if you want to parse the generated strings, you also
> need that information in order to perform the range check.
>
> Or be radical and always force implementors to write even in simple cases:
>
>   update ("bpm", val, [&]() { val = CLAMP (val, 1, 1024);  bpm_ = val; });

We have maybe 100 or 200 properties, any significant middleware extensions
should simplify or significantly improve the *majority* of accessor implementations
to warrant the efforts involved. If not, we're better off using our time for just
porting the remaining cases.

As an aside, we have not yet considered automation. Looking at DAWs out there,
we probably want to make *all* float + int properties automatable in the future
anyway (maybe even enums like filter/oscillator types). And that could potentially
change the int/float/enum accessors a whole lot yet again. Input on that front is
welcome also, but should go into a separate thread.

>    Cu... Stefan


-- 
Yours sincerely,
Tim Janik

---
https://testbit.eu/timj/
Free software author.