Gtk+ › Gnome - Beast

[tim-janik/beast] Smarter property set() implementation (#74)

Classic

List

Threaded

7 messages Options

Gnome - Beast mailing list

[tim-janik/beast] Smarter property set() implementation (#74)

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

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

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
do nothing at all if val equals the value returned by the getter
push undo if property has undo
call the _impl function
notify the property

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.

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

Gnome - Beast mailing list

Smarter property set() implementation (re #74)

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.
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"Smarter property set() implementation (#74)"}],"action":{"name":"View Issue","url":"https://github.com/tim-janik/beast/issues/74"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/issues/74", "url": "https://github.com/tim-janik/beast/issues/74", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Smarter property set() implementation (#74)", "sections": [ { "text": "", "activityTitle": "**Stefan Westerfeld**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@swesterfeld", "facts": [ { "name": "Repository: ", "value": "tim-janik/beast" }, { "name": "Issue #: ", "value": 74 } ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 74,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close issue", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueClose\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 74\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/issues/74" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 374800324\n}" } ], "themeColor": "26292E" } ]</script>

-- 
Yours sincerely,
Tim Janik

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

_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast

Gnome - Beast mailing list

Re: [tim-janik/beast] Smarter property set() implementation (#74)

In reply to this post by Gnome - Beast mailing list

Great input Stefan.

But please take brainstorming and discussions to the mailing list:

https://mail.gnome.org/archives/beast/2018-August/msg00101.html

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

Gnome - Beast mailing list

Re: [tim-janik/beast] Smarter property set() implementation (#74)

In reply to this post by Gnome - Beast mailing list

Closed #74.

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

Stefan Westerfeld

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

In reply to this post by Gnome - Beast mailing list

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
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast

Gnome - Beast mailing list

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

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.

_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast

Stefan Westerfeld

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
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast