Re: [tim-janik/beast] BSE: fix undo problems that occur after removing a bus (#82)

Ok, I'll try to give a more detailed account of the events that lead to the bug. For the sake of this discussion, we only need to do three things:


  1. First let us instrument the function
bse_container_uncross_undoable (BseContainer *container,
                                BseItem      *child)
  BseItem *ancestor;

  assert_return (BSE_IS_CONTAINER (container));
  assert_return (BSE_IS_ITEM (child));
  assert_return (child->parent == (BseItem*) container);

  /* backup source channels state */
  if (BSE_IS_SOURCE (child))
      printf ("[1] clearing inputs and outputs for child: %s\n", bse_object_debug_name (child));
      bse_source_backup_ochannels_to_undo (BSE_SOURCE (child));
      bse_source_clear_ochannels (BSE_SOURCE (child));
      bse_source_backup_ichannels_to_undo (BSE_SOURCE (child));
      bse_source_clear_ichannels (BSE_SOURCE (child));
  /* dispose cross references, those backup themselves */
  ancestor = BSE_ITEM (container);
      bse_container_uncross_descendant (BSE_CONTAINER (ancestor), child);
      ancestor = ancestor->parent;
  while (ancestor);

with a printf satement, and repeat the steps. We get this output:

[1] clearing inputs and outputs for child: "BseBus::Bus-1"

So if we delete the bus Bus-1, the first thing that happens is that all connections from and to the bus get removed in this function.

  1. To see how this affects our undo step, lets instrument another related function:
bse_bus_disconnect (BseBus  *self,
                    BseItem *trackbus)
  BseSource *osource;
  if (BSE_IS_TRACK (trackbus))
    osource = bse_track_get_output (BSE_TRACK (trackbus));
  else if (BSE_IS_BUS (trackbus))
    osource = BSE_SOURCE (trackbus);
    return Bse::Error::SOURCE_TYPE_INVALID;
  if (!osource || !self->summation || !sfi_ring_find (self->inputs, trackbus))
    return Bse::Error::SOURCE_PARENT_MISMATCH;
  bse_object_unproxy_notifies (trackbus, self, "notify::inputs");
  bse_object_unproxy_notifies (self, trackbus, "notify::outputs");
  bse_item_cross_unlink (BSE_ITEM (self), BSE_ITEM (trackbus), bus_uncross_input);
  self->inputs = sfi_ring_remove (self->inputs, trackbus);
  trackbus_update_outputs (trackbus, NULL, self);
  Bse::Error error1 = bse_source_unset_input (self->summation, 0, osource, 0);
  Bse::Error error2 = bse_source_unset_input (self->summation, 1, osource, 1);
  printf ("[2] disconnecting summation %s output source %s:\n[2] errors: %s and %s\n",
      bse_object_debug_name (self->summation), bse_object_debug_name (osource),
      bse_error_blurb (error1), bse_error_blurb (error2));
  g_object_notify (G_OBJECT (self), "inputs");
  g_object_notify (G_OBJECT (trackbus), "outputs");
  return error1 != 0 ? error1 : error2;

which produces the output

[2] disconnecting summation "BseSummation::Summation" output source "BseBus::Bus-1":
[2] errors: Input/Output channels not connected and Input/Output channels not connected

(the numbers indicate order, so this happens after [1]). So what happens is that this function returns an error code, because the connections between Summation output source Bus-1 don't exist (were removed in [1]).

  1. Finally this is what causes the undo step recording to be omitted as we can find by adding a printf to
BusImpl::disconnect_bus (BusIface &busi)
  BseBus *self = as<BseBus*>();
  BusImpl &bus = dynamic_cast<BusImpl&> (busi);
  Error error = bse_bus_disconnect (self,<BseItem*>());
  printf ("[3] disconnect_bus returned: error %s\n", bse_error_blurb (error));
  if (error == 0)
      // an undo lambda is needed for wrapping object argument references
      UndoDescriptor<BusImpl> bus_descriptor = undo_descriptor (bus);
      auto lambda = [bus_descriptor] (BusImpl &self, BseUndoStack *ustack) -> Error {
        return self.connect_bus (self.undo_resolve (bus_descriptor));
      push_undo (__func__, *this, lambda);
  return error;

Which results in this output:

[3] disconnect_bus returned: error Input/Output channels not connected

So to summarize this, the output when deleting Bus-1 will be

[1] clearing inputs and outputs for child: "BseBus::Bus-1"
[2] disconnecting summation "BseSummation::Summation" output source "BseBus::Bus-1":
[2] errors: Input/Output channels not connected and Input/Output channels not connected
[3] disconnect_bus returned: error Input/Output channels not connected

which we can summarize as the input/outputs of Bus-1 are disconnected in bse_container_uncross_undoable [1], due to this bse_bus_disconnect [2] fails and finally the undo step is not recorded in BusImpl::disconnect_bus [3]. The patch that I've proposed fixes this by ignoring the error code Error::SOURCE_NO_SUCH_CONNECTION in [3]. Note that this doesn't irgnore all possible errors, just specialcase the case that there was no connection.

Because the connection did exist, but was removed in [1].

Honestly, I don't know the answer to this. Maybe the fix should go somewhere else, or if we want to keep this, we could add state to the bus that indicates whether the bus is connected or not (independantly of whether or not the channels are connected).

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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]