GNOME.org
 

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

   Hi!

On Fri, Sep 14, 2018 at 05:59:43PM +0200, Tim Janik wrote:

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


An important step in the context of this discussion.


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).


Ok. Nowerdays it is even supported by msvc compiler, so all right, lets use it,
I can't think of a case where it would break compilation.


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.


Ok. There are two ways actually to achieve this: one would be extending the
generic setter to use __func__. That will have to use macro, but I'll suggest
two versions below. The other is different: avoid passing strings in the API.
Often there are multiple properties that need to be notified, for instance the
setter part of BusImpl::volume_db:

  auto prop = "volume_db";
  push_property_undo (prop);
  [...]
  notify ("left_volume");
  notify ("right_volume");
  notify ("pan");
  notify (prop);

Even if the generic setter would eliminate prop, but there are three strings
that the compiler cannot check. If we would write

  notify (p_left_volume);
  notify (p_right_volume);
  notify (p_pan);

for the remaining notifications, we would at least catch typos here. And
p_something would be an automatically generated static data member of the base
class, in the easiest case a string, later if we have ported everything to C++,
we could make this a Property class, and enforce that notify only gets called
with a property class.


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");


Right. And clamping. I'll address this below.


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.


All right, if we have to get rid of the "denominator" here, and writing
p_denominator as mentioned above is also not good enough, here are two macro
versions. I've tested this with a complete C++ file, but I'll just show the
macro definition and usage here. The first case is close to my original
suggestion, and the macro only replaces the invocation with some lambda thing
that adds __func__ to the invocation of the real update function

#define UPDATE [&, what__ = __func__](auto& val, auto l){ update (what__, val, l); }

So this would look like this:

void
SongImpl::denominator (int val)
{
  BseSong *self = as<BseSong*>();

  UPDATE (val, [&]() {
    self->denominator = val <= 2 ? val : 1 << g_bit_storage (val - 1);
    bse_song_update_tpsi_SL (self);
  });
}

I like the lambda as a way of expressing that the body update code may or may
not be executed. Also its easy to executed the notification after the body.
Once the variables are stored in the C++ class, things will get simpler.

Another way would be to make the macro basically act like an if-statement. This
again expresses conditional execution. To do notification after the setter, we
need a helper class that does this in the destructor, if the conditional
execution took place, as

#define IF_UPDATE(val__) auto uh__ = UpdateHelper (__func__, this, get (__func__) != val__); if (uh__.changed)

void
SongImpl::denominator (int val)
{
  BseSong *self = as<BseSong*>();

  IF_UPDATE (val)
    {
      self->denominator = val <= 2 ? val : 1 << g_bit_storage (val - 1);
      bse_song_update_tpsi_SL (self);
    }
}

I'm not sure if this is good practice, as the IF_UPDATE expands to something
that is not a complete statements, but only the "if"-part.


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.


Ok, then maybe my proposal was too specific. You don't want it in the generated
language binding, and that in itself is ok. However, I'll still say that we
should have *something* like __aida_range__.

For the JavaScript UI, you will have code to query the range of a property.
For beast-gtk, as long as it lives, you will have code to query the range of a
property.  So I argue that for libbse (the backend, the core), we should have
some way to query the range of a property. This can then be used in the generic
setter code.

Whether you parse the range information for a property from the key=value
strings, cache the parsed value, or whatever else, I'd like to have (for
numeric properties) a way to say

  val = range_check ("bpm", val);

I say this because the alternative would be to duplicate all range information for
numeric properties, and this doesn't feel right

  val = CLAMP (val, 1, 1024);


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.


Ok, as I said: accessing the range of a property is not an esoteric request, but
it seems clear that both ebeast and beast-gtk need that, so it might as well be
a useful information for the core itself.


As an aside, we have not yet considered automation. [...]


Right. But right now I think property porting is my main priority, so it is
better to focus on that for a while.

   Cu... Stefan
-- 
Stefan Westerfeld, http://space.twc.de/~stefan
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]