tim-janik requested changes on this pull request.
Please fix review comments I left for you and remove:
a) the commit introducing the block/unblock API,
b) the float64 workaround.
If that means you updated PR is broken, point out the bugs you're runing into
and I'll give you a hand with the fixes. In particualy, I'm not even sure you
can trigger buggy behaviour due to not blocking undo, if you find a way to
trigger that, that'd be really helpful. As for fixing the float parsing, I
looked into that earlier and am quite confident I can fix parse_paren_rest. I
just needed an actual test case that triggered breakage, which I didn't have at
the time.
> @@ -1003,6 +993,25 @@ BusImpl::BusImpl (BseObject *bobj) :
BusImpl::~BusImpl ()
{}
+bool
+BusImpl::mute() const
+{
+ BseBus *self = const_cast<BusImpl*> (this)->as<BseBus*>();
+
+ return self->muted;
+}
+
+void
+BusImpl::mute (bool val)
+{
+ BseBus *self = const_cast<BusImpl*> (this)->as<BseBus*>();
+
+ if (APPLY_IDL_PROPERTY (self->muted, val))
+ {
Please avoid excessive newlines and braces. This function has 3 lines of code
but is pumped up to 6 lines in total. The following is prefectly readable:
```
BseBus *self = const_cast<BusImpl*> (this)->as<BseBus*>();
if (APPLY_IDL_PROPERTY (self->muted, val))
bus_volume_changed (self);
```
> @@ -89,8 +89,24 @@ bus_build_param (BstBusEditor *self,
const gchar *editor,
const gchar *label)
{
- GParamSpec *pspec = bse_proxy_get_pspec (self->item, property);
- self->params = sfi_ring_prepend (self->params, bst_param_new_proxy (pspec,
self->item));
+ GxkParam *gxk_param = nullptr;
This should be moved down to be closest to the site of use.
> @@ -89,8 +89,24 @@ bus_build_param (BstBusEditor *self,
const gchar *editor,
const gchar *label)
{
- GParamSpec *pspec = bse_proxy_get_pspec (self->item, property);
- self->params = sfi_ring_prepend (self->params, bst_param_new_proxy (pspec,
self->item));
+ GxkParam *gxk_param = nullptr;
+
+ /* aida property? */
+ Bse::BusH bus = Bse::BusH::__cast__ (bse_server.from_proxy (self->item));
There is only a single caller for this function, so the __cast__ should be
moved up into the caller and avoided here.
> @@ -89,8 +89,24 @@ bus_build_param (BstBusEditor *self,
const gchar *editor,
const gchar *label)
{
- GParamSpec *pspec = bse_proxy_get_pspec (self->item, property);
- self->params = sfi_ring_prepend (self->params, bst_param_new_proxy (pspec,
self->item));
+ GxkParam *gxk_param = nullptr;
+
+ /* aida property? */
+ Bse::BusH bus = Bse::BusH::__cast__ (bse_server.from_proxy (self->item));
+ const Bse::StringSeq meta = bus.find_prop (property);
Instead of `meta` we've been using `kvlist` elsewhere.
The following lines should be wrapped into `if (!meta.empty()) {
...pspec_from_key_value_list }`
And `bse_proxy_get_pspec` should only be called if `meta` (`kvlist`) was empty.
> + if (BSE_IS_SONG (parent))
+ {
+ BseSong *song = BSE_SONG (parent);
+ return song->solo_bus == self;
+ }
+ return false;
+}
+
+void
+BusImpl::solo (bool is_solo)
+{
+ BseBus *self = as<BseBus*>();
+
+ if (solo() != is_solo)
+ {
+ auto prop = "solo";
This can be `const`.
> @@ -842,9 +824,10 @@ bus_restore_finish (BseObject *object,
{
BseBus *self = BSE_BUS (object);
/* restore real sync setting */
- g_object_set (self, /* no undo */
- "sync", self->saved_sync,
- NULL);
+ auto impl = self->as<Bse::BusImpl*>();
+ impl->block_property_undo();
+ impl->sync (self->saved_sync);
+ impl->unblock_property_undo();
Please get rid of the block/unblock calls here, I think we need to handle that
elsewhere.
This code is only called during restore(), and the result of the undo stack
during restore is thrown away anyway (in `ProjectImpl::restore_from_file():
bse_project_clear_undo`). Actually we *should* be using `bse_undo_stack_dummy`
during restore anyway.
> @@ -82,22 +82,28 @@ bus_editor_release_item (SfiProxy item,
bst_bus_editor_set_bus (self, 0);
}
+static GxkParam *
+get_property_param (BstBusEditor *self,
+ const gchar *property)
+{
+ Bse::BusH bus = Bse::BusH::__cast__ (bse_server.from_proxy (self->item));
+ const Bse::StringSeq meta = bus.find_prop (property);
+
+ GParamSpec *cxxpspec = Bse::pspec_from_key_value_list (property, meta);
Use `kvlist`.
> @@ -82,22 +82,28 @@ bus_editor_release_item (SfiProxy item,
bst_bus_editor_set_bus (self, 0);
}
+static GxkParam *
+get_property_param (BstBusEditor *self,
+ const gchar *property)
+{
+ Bse::BusH bus = Bse::BusH::__cast__ (bse_server.from_proxy (self->item));
We are moving to Aida handles everywhre, so always use ItemH, BusH, etc instead
of proxies, i.e. simply pass a handle into this function.
> + {
+ BseSong *song = BSE_SONG (parent);
+ BseBus *master = bse_song_find_master (song);
+ return (self == master);
+ }
+ return false;
+}
+
+void
+BusImpl::master_output (bool val)
+{
+ BseBus *self = as<BseBus*>();
+
+ if (val != master_output())
+ {
+ auto prop = "master_output";
Add `const`
> @@ -741,6 +741,16 @@ storage_parse_property_value (BseStorage *self, const
> std::string &name, Bse::An
{
assert_return (BSE_IS_STORAGE (self), G_TOKEN_ERROR);
GScanner *scanner = bse_storage_get_scanner (self);
+ if (any.kind() == Aida::FLOAT64) /* float format cannot be handled by
scanner_parse_paren_rest */
I don't think any.kind() will always hold the desired type here for all code
paths.
This is more a workaround than a fix, I think scanner_parse_paren_rest should
be fixed instead.
> if (BSE_IS_SONG (parent))
{
BseSong *song = BSE_SONG (parent);
BseBus *master = bse_song_find_master (song);
- return (self == master);
+ return self == master;
Code like this can be merged down into the culprit of your current branch.
Here's how you do that:
1) Use `git blame HEAD -- bsebus.cc` to find the faulty commit:
```
aaabbbccc bse/bsebus.c swesterfeld return (self == master);
```
2) Commit the fix as in:
```
git commit -m '+++ fix commit aaabbbccc'
111222333
```
3) Assuming your current branch started from master, merge this down with: `git
rebase -i master`
4) Move `pick 111222333 +++ fix commit aaabbbccc` into a `squash` or `fixup`
following the culprit:
```
pick aaabbbccc BSE: introduce buggy code
squash 111222333 +++ fix commit aaabbbccc
```
> Bse::BusImpl *bus = child->as<Bse::BusImpl*>();
+ bus->block_property_undo();
Get rid of the block/unblock here, I'd like this to be handled elsewhere.
> @@ -681,7 +683,7 @@ SongImpl::remove_bus (BusIface &bus_iface)
BseItem *child = bus.as<BseItem*>();
BseUndoStack *ustack = bse_item_undo_open (self, __func__);
// reset ::master-output property undoable
The comment about undoable here can be removed.
> @@ -1335,4 +1337,16 @@ ItemImpl::apply_idl_property_need_undo (const
> StringVector &kvlist)
return !Aida::aux_vector_check_options (kvlist, "", "hints", "skip-undo");
}
+void
+ItemImpl::block_property_undo()
+{
I really don't want to introduce these. We already have other mechanism that
check if undo steps should be recorded at all, and we have a dummy undo stack
`bse_undo_stack_dummy` that could be used if we'd want to temporarily ignore
undo steps. Also, the *C++* developer shouldn't be bothered with having to use
block/unblock in pairs, so for the future, similar API should make use of
ctor/dtor pairs like std::lock_guard does.
Assuming your branch starts off from `master`, you can get rid of this commit
with `git rebase -i` and then remove the `pick ...` line that refers to this
commit.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/tim-janik/beast/pull/105#pullrequestreview-240008566_______________________________________________
beast mailing list
beast@gnome.org
https://mail.gnome.org/mailman/listinfo/beast
