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

[Stefan Westerfeld <stefan space twc de>]

   Hi!

On Fri, Aug 31, 2018 at 11:53:07PM +0200, Tim Janik via beast wrote:
> since you're brain storming here and started a discussion, I'm taking this to
> the list and off the bug tracker.

I won't comment on each thing you stated, yes, there were issues with my
suggestion. Basically

- you want a lightweight solution
- you want to avoid code generation if possible
- you want it to be optional and not force the same steps for every setter
- there were minor technical issues to by suggestion, but these could be
  fixed while keeping the proposal

> Now, whether the impl CLAMPs or ignores the setting is entirely up to the
> implementation.

All right, I'll keep that in mind.

> And whether a property needs undo recording (e.g. a bpm change) or no undo
> recording
> (like a play position or loop pointer), is also entirely up to the
> implementation.

Right. We should also mention here that properties that do need undo recording
need a way of suppressing undo. In the old codebase you use g_object_set if
you don't want undo while setting a property.

Also see "Most Bus properties ported to C++ #78".

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

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

and implement a generic:

  void update (const char *what, int& val, std::function<void()> func);

which

(1) checks if get ("denominator") != value    (aida has generic getters)
- if this is true
  (2) performs range checks and _modifies_ val if necessary
  (3) pushes undo
  (4) notifies "denominator"

not using a macro here also allows us to pass a pointer to the variable which
stores the value as an overloaded version, i.e.

  update ("bpm", val, &bpm_);

Finally, undo: as far as I can see, the old way of doing this is defining
SKIP_UNDO in the idl file, so that the property can query whether undo
information should be recorded or not. So if I understand you correctly
you want me to remove this information from the .idl file, and instead
do it in the implementatation only. So we can simply use the function
name to encode this feature. So we could use

  update_undo ("bpm", val, &bpm_);

if we need undo, and the C++ file would be the only place where we can see if a
property is recording undo information or not.

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

But if we add this, it should cover all cases.

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; }
  ...
  }

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

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