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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Gnome - Beast mailing list

To fix the problem, ensure that an appropriate undo step is always recorded.

This is a fix for issue #79


You can view, comment on, or merge this pull request online at:

  https://github.com/tim-janik/beast/pull/82

Commit Summary

  • BSE: fix undo problems that occur after removing a bus

File Changes

Patch Links:


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

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"BSE: fix undo problems that occur after removing a bus (#82)"}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/82"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/82", "url": "https://github.com/tim-janik/beast/pull/82", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "BSE: fix undo problems that occur after removing a bus (#82)", "sections": [ { "text": "", "activityTitle": "**Stefan Westerfeld**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@swesterfeld", "facts": [ ] }, { "title": "Commit Summary", "facts": [ { "name": "e10c9ee", "value": "BSE: fix undo problems that occur after removing a bus" } ] }, { "title": "File Changes", "facts": [ { "name": "Modified", "value": "[bse/bsebus.cc](https://github.com/tim-janik/beast/pull/82/files#diff-0) (7 changes)" } ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 82,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"pullRequestId\": 82\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/82" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/82.patch" } ], "@type": "OpenUri", "name": "View patch" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/82.diff" } ], "@type": "OpenUri", "name": "View diff" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 380351199\n}" } ], "themeColor": "26292E" } ]</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

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

Gnome - Beast mailing list

Hey Stefan, thanks for looking into this.
I don't fully follow the argument you make in your comment though.

  • Why should an undo step be recorded for restoring a connection that doesn't exist in the first place?
  • Also, with your code, calling disconnect_bus() repeatedly will record more and more undo steps, for a connection that's not existing and could have been restored at most only once anyway.

It look like you're onto something here, but AFAIU the "fix" in this PR is almost certainly not the right one.


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

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"PERSON","message":"@tim-janik in #82: Hey Stefan, thanks for looking into this.\r\nI don't fully follow the argument you make in your comment though.\r\n\r\n* Why should an undo step be recorded for restoring a connection that doesn't exist in the first place?\r\n* Also, with your code, calling disconnect_bus() repeatedly will record more and more undo steps, for a connection that's not existing and could have been restored at most only once anyway.\r\n\r\nIt look like you're onto something here, but AFAIU the \"fix\" in this PR is almost certainly not the right one."}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/82#issuecomment-425030568"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/82#issuecomment-425030568", "url": "https://github.com/tim-janik/beast/pull/82#issuecomment-425030568", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [tim-janik/beast] BSE: fix undo problems that occur after removing a bus (#82)", "sections": [ { "text": "", "activityTitle": "**Tim Janik**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@tim-janik", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 82,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"pullRequestId\": 82\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/82#issuecomment-425030568" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 380351199\n}" } ], "themeColor": "26292E" } ]</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

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

Gnome - Beast mailing list
In reply to this post by Gnome - Beast mailing list

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:

  • create song
  • create bus - this will be called Bus-1
  • delete this bus - no undo step will be recorded (which causes problems later)

Why?

  1. First let us instrument the function
void
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);
  do
    {
      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::Error
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);
  else
    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
Error
BusImpl::disconnect_bus (BusIface &busi)
{
  BseBus *self = as<BseBus*>();
  BusImpl &bus = dynamic_cast<BusImpl&> (busi);
  Error error = bse_bus_disconnect (self, busi.as<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.

  • Why should an undo step be recorded for restoring a connection that doesn't exist in the first place?

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

  • Also, with your code, calling disconnect_bus() repeatedly will record more and more undo steps, for a connection that's not existing and could have been restored at most only once anyway.

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.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"PERSON","message":"@swesterfeld in #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:\r\n* create song\r\n* create bus - this will be called **Bus-1**\r\n* delete this bus - no undo step will be recorded (which causes problems later)\r\n\r\nWhy?\r\n\r\n1. First let us instrument the function\r\n```\r\nvoid\r\nbse_container_uncross_undoable (BseContainer *container,\r\n BseItem *child)\r\n{\r\n BseItem *ancestor;\r\n\r\n assert_return (BSE_IS_CONTAINER (container));\r\n assert_return (BSE_IS_ITEM (child));\r\n assert_return (child-\u003eparent == (BseItem*) container);\r\n\r\n /* backup source channels state */\r\n if (BSE_IS_SOURCE (child))\r\n {\r\n printf (\"[1] clearing inputs and outputs for child: %s\\n\", bse_object_debug_name (child));\r\n bse_source_backup_ochannels_to_undo (BSE_SOURCE (child));\r\n bse_source_clear_ochannels (BSE_SOURCE (child));\r\n bse_source_backup_ichannels_to_undo (BSE_SOURCE (child));\r\n bse_source_clear_ichannels (BSE_SOURCE (child));\r\n }\r\n /* dispose cross references, those backup themselves */\r\n ancestor = BSE_ITEM (container);\r\n do\r\n {\r\n bse_container_uncross_descendant (BSE_CONTAINER (ancestor), child);\r\n ancestor = ancestor-\u003eparent;\r\n }\r\n while (ancestor);\r\n}\r\n```\r\nwith a printf satement, and repeat the steps. We get this output:\r\n```\r\n[1] clearing inputs and outputs for child: \"BseBus::Bus-1\"\r\n```\r\n\r\nSo 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.\r\n\r\n2. To see how this affects our undo step, lets instrument another related function:\r\n\r\n```\r\nBse::Error\r\nbse_bus_disconnect (BseBus *self,\r\n BseItem *trackbus)\r\n{\r\n BseSource *osource;\r\n if (BSE_IS_TRACK (trackbus))\r\n osource = bse_track_get_output (BSE_TRACK (trackbus));\r\n else if (BSE_IS_BUS (trackbus))\r\n osource = BSE_SOURCE (trackbus);\r\n else\r\n return Bse::Error::SOURCE_TYPE_INVALID;\r\n if (!osource || !self-\u003esummation || !sfi_ring_find (self-\u003einputs, trackbus))\r\n return Bse::Error::SOURCE_PARENT_MISMATCH;\r\n bse_object_unproxy_notifies (trackbus, self, \"notify::inputs\");\r\n bse_object_unproxy_notifies (self, trackbus, \"notify::outputs\");\r\n bse_item_cross_unlink (BSE_ITEM (self), BSE_ITEM (trackbus), bus_uncross_input);\r\n self-\u003einputs = sfi_ring_remove (self-\u003einputs, trackbus);\r\n trackbus_update_outputs (trackbus, NULL, self);\r\n Bse::Error error1 = bse_source_unset_input (self-\u003esummation, 0, osource, 0);\r\n Bse::Error error2 = bse_source_unset_input (self-\u003esummation, 1, osource, 1);\r\n printf (\"[2] disconnecting summation %s output source %s:\\n[2] errors: %s and %s\\n\",\r\n bse_object_debug_name (self-\u003esummation), bse_object_debug_name (osource),\r\n bse_error_blurb (error1), bse_error_blurb (error2));\r\n g_object_notify (G_OBJECT (self), \"inputs\");\r\n g_object_notify (G_OBJECT (trackbus), \"outputs\");\r\n return error1 != 0 ? error1 : error2;\r\n}\r\n```\r\nwhich produces the output\r\n```\r\n[2] disconnecting summation \"BseSummation::Summation\" output source \"BseBus::Bus-1\":\r\n[2] errors: Input/Output channels not connected and Input/Output channels not connected\r\n```\r\n(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]).\r\n\r\n3. Finally this is what causes the undo step recording to be omitted as we can find by adding a printf to\r\n\r\n```\r\nError\r\nBusImpl::disconnect_bus (BusIface \u0026busi)\r\n{\r\n BseBus *self = as\u003cBseBus*\u003e();\r\n BusImpl \u0026bus = dynamic_cast\u003cBusImpl\u0026\u003e (busi);\r\n Error error = bse_bus_disconnect (self, busi.as\u003cBseItem*\u003e());\r\n printf (\"[3] disconnect_bus returned: error %s\\n\", bse_error_blurb (error));\r\n if (error == 0)\r\n {\r\n // an undo lambda is needed for wrapping object argument references\r\n UndoDescriptor\u003cBusImpl\u003e bus_descriptor = undo_descriptor (bus);\r\n auto lambda = [bus_descriptor] (BusImpl \u0026self, BseUndoStack *ustack) -\u003e Error {\r\n return self.connect_bus (self.undo_resolve (bus_descriptor));\r\n };\r\n push_undo (__func__, *this, lambda);\r\n }\r\n return error;\r\n}\r\n```\r\nWhich results in this output:\r\n```\r\n[3] disconnect_bus returned: error Input/Output channels not connected\r\n```\r\n\r\n\r\nSo to summarize this, the output when deleting **Bus-1** will be\r\n```\r\n[1] clearing inputs and outputs for child: \"BseBus::Bus-1\"\r\n[2] disconnecting summation \"BseSummation::Summation\" output source \"BseBus::Bus-1\":\r\n[2] errors: Input/Output channels not connected and Input/Output channels not connected\r\n[3] disconnect_bus returned: error Input/Output channels not connected\r\n```\r\nwhich 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.\r\n\r\n\u003e * Why should an undo step be recorded for restoring a connection that doesn't exist in the first place?\r\n\r\nBecause the connection did exist, but was removed in [1].\r\n\r\n\u003e * Also, with your code, calling disconnect_bus() repeatedly will record more and more undo steps, for a connection that's not existing and could have been restored at most only once anyway.\r\n\r\nHonestly, 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)."}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/82#issuecomment-433392380"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/82#issuecomment-433392380", "url": "https://github.com/tim-janik/beast/pull/82#issuecomment-433392380", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [tim-janik/beast] BSE: fix undo problems that occur after removing a bus (#82)", "sections": [ { "text": "", "activityTitle": "**Stefan Westerfeld**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@swesterfeld", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 82,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"pullRequestId\": 82\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/82#issuecomment-433392380" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 380351199\n}" } ], "themeColor": "26292E" } ]</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

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

Gnome - Beast mailing list
In reply to this post by Gnome - Beast mailing list

For the sake of this discussion, we only need to do three things:

[...snip...] removing lots of symptom investigation [...snip...]

You never get to the root cause here, which is removal of connections without undo recording in the first place.

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.

  • Why should an undo step be recorded for restoring a connection that doesn't exist in the first place?

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

No, you're just looking at one possible way and one possible time to call a function, a connection did not necessarily exist previously, and only if it did would it be valid to record an undo step.

  • Also, with your code, calling disconnect_bus() repeatedly will record more and more undo steps, for a connection that's not existing and could have been restored at most only once anyway.

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).

Thanks for tracing this down, a clear problem understanding is part of what's needed for a fix. As for the actual patch, we must make sure to only record undo steps when appropriate though like described earlier.


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

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/tim-janik/beast","title":"tim-janik/beast","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/tim-janik/beast"}},"updates":{"snippets":[{"icon":"PERSON","message":"@tim-janik in #82: \u003e For the sake of this discussion, we only need to do three things:\r\n\r\n[...snip...] removing lots of *symptom* investigation [...snip...]\r\n\r\nYou never get to the root cause here, which is removal of connections without undo recording in the first place.\r\n\r\n\u003e 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.\r\n\u003e \r\n\u003e \u003e * Why should an undo step be recorded for restoring a connection that doesn't exist in the first place?\r\n\u003e \r\n\u003e Because the connection did exist, but was removed in [1].\r\n\r\nNo, you're just looking at one possible way and one possible time to call a function, a connection did not *necessarily* exist previously, and only if it did would it be valid to record an undo step.\r\n\r\n\u003e \u003e * Also, with your code, calling disconnect_bus() repeatedly will record more and more undo steps, for a connection that's not existing and could have been restored at most only once anyway.\r\n\u003e \r\n\u003e 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).\r\n\r\nThanks for tracing this down, a clear problem understanding is part of what's needed for a fix. As for the actual patch, we must make sure to only record undo steps when appropriate though like described earlier."}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/82#issuecomment-434403855"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/82#issuecomment-434403855", "url": "https://github.com/tim-janik/beast/pull/82#issuecomment-434403855", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [tim-janik/beast] BSE: fix undo problems that occur after removing a bus (#82)", "sections": [ { "text": "", "activityTitle": "**Tim Janik**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@tim-janik", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"issueId\": 82,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"tim-janik/beast\",\n\"pullRequestId\": 82\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/82#issuecomment-434403855" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 380351199\n}" } ], "themeColor": "26292E" } ]</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast