Smarter property set() implementation (re #74)

[Tim Janik <tim janik gmail com>]

Hey Stefan,

since you're brain storming here and started a discussion, I'm taking this to the list and off the bug tracker.

There are a few reasons I have not attempted to add enforcement with the new IDL API.

First, the range information the IDL layer has is informal and mostly passed from *.idl to the
__aida_aux_data__() method as opaque string.

Second, even if the IDL layer was extended to parse the information and we'd start to formally
specify what information has and has not to be present per property, it's not clear to me at what
layer we'd generate automatic enforcement.

Note that it gets even more tricky in the actual implementation, see my inline remarks below.

On 31.08.2018 21:33, Stefan Westerfeld wrote:

From our last BseSong merge:

There's one other thing I noticed during review. With the old GParamSpec + GObject based property API, the GObject machinery ensures that the GValue passed in to property setters complies with the GParamSpec value range. With our IDL API, nothing like that is enforced anymore. That means, an int32 property between 1-256 can now be set to 0 or -2^31 through the API. For a denominator to become 0 that could affect stability.
Please keep that in mind for ported properties and watch out if we shouldn't add extra guards, like:

denominator = CLAMP (input, 0, 256)

I think it might be better to re-add range enforcement. Right now, I would have to write this setter code:

void
SongImpl::denominator (int val) 
{
  BseSong *self = as<BseSong*>();
  val = CLAMP (val, 1, 256);
  if (int (self->denominator) != val) 
    {    
      const char *prop = "denominator";
      push_property_undo (prop);

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

      notify (prop);
    }    
}

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.

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.

Furthermore, whether a value can currently be changed (like the instrument setup used
on a track) may depend on whether a project is currently in playback mode or not (that's
the BSE_SOURCE_PREPARED flag in libbse), so entirely up to the implementation.

Note that we duplicate the defaults here, which is not ideal. So we could also check against the pspec, somewhat like

  ...
  val = prop_denominator.clamp (val);
  // prop_denominator would be generated and contains min(), max(), def(), name()
  ...

There's no "pspec" in the new API, while we still generate compat pspecs
for the old beast-gtk code from the IDL aux info, there's no such thing in
ebeast. I.e. checks can only be performed against __aida_aux_data__ strings.
And without special knowledge about properties, etc, those look just like a
bunch of foreign gibberish characters.

But I think ultimately what I really want to write is this code:

void
SongImpl::denominator_impl (int val) 
{
  BseSong *self = as<BseSong*>();
 
  self->denominator = val <= 2 ? val : 1 << g_bit_storage (val - 1);
  bse_song_update_tpsi_SL (self);   
}

and have the actual denominator (int val) function be generated automatically, which should

• constrain val to param spec

As I said, above, we don't have that anymore.

• do nothing at all if val equals the value returned by the getter

That would break the SynDrum trigger button.

• push undo if property has undo

That highly depends on the property and I'd rather not push that
into the IDL layer as well.

• call the _impl function
• notify the property

Notification is implemented at the Bse::Object layer, not the
IDL layer below that, so that'd be tricky also.

Consider this a brianstorming idea I at least wanted to share. Maybe the actual implementation will be a bit different, but I think the general idea to have each setter always execute some steps at the beginning and some steps at the end of setting a property is likely to make our actual code simpler.

Yeah, I appreciate that in principle, but I hope I have succeeded in outlining
why I thought there's no trivial and obvious way to implement this.

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

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

-- 
Yours sincerely,
Tim Janik

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