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
> <https://github.com/tim-janik/beast/issues/74>, or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AARNH77bBFgJsr7R1ujy2qxtZ8Nq2z-Pks5uWY9wgaJpZM4WVv_E>.
>
--
Yours sincerely,
Tim Janik
---
https://testbit.eu/timj/
Free software author.
_______________________________________________
beast mailing list
beast@gnome.org
https://mail.gnome.org/mailman/listinfo/beast
