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