@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.
In bse/bsebus.cc:
> @@ -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.
In bse/bsebus.cc:
> + 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.
In bse/bsebus.cc:
> @@ -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.
In bse/bsebus.cc:
> + {
+ 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.
In bse/bsebus.cc:
> 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:
git blame HEAD -- bsebus.cc to find the faulty commit:aaabbbccc bse/bsebus.c swesterfeld return (self == master);
git commit -m '+++ fix commit aaabbbccc'
111222333
git rebase -i masterpick 111222333 +++ fix commit aaabbbccc into a squash or fixup following the culprit:pick aaabbbccc BSE: introduce buggy code
squash 111222333 +++ fix commit aaabbbccc
In bse/bsesong.cc:
> 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.
In bse/bsesong.cc:
> @@ -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.
In bse/bseitem.cc:
> @@ -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, view it on GitHub, or mute the thread.