[tim-janik/beast] Port property Song::loop_enabled to C++ (#64)

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

[tim-janik/beast] Port property Song::loop_enabled to C++ (#64)

Gnome - Beast mailing list

Please merge. I ported and tested this property port.

Remarks: I tried and failed to use something like (doesn't compile).

bool loop_enabled = Bool (... STORAGE SKIP_DEFAULT SKIP_UNDO ...)

Everything works as it is, but this would catch typos. In any case, it is not a big problem.


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

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

Commit Summary

  • BSE: Song::loop_enabled: port property to C++
  • BEAST-GTK: bsttrackview: introduce scoped handle Bse::SongS
  • BEAST-GTK: bsttrackview: use C++ property Song::loop_enabled

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":"Port property Song::loop_enabled to C++ (#64)"}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/64"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/64", "url": "https://github.com/tim-janik/beast/pull/64", "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": "Port property Song::loop_enabled to C++ (#64)", "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": "ff3030f", "value": "BSE: Song::loop_enabled: port property to C++" }, { "name": "9140447", "value": "BEAST-GTK: bsttrackview: introduce scoped handle Bse::SongS" }, { "name": "6ae8525", "value": "BEAST-GTK: bsttrackview: use C++ property Song::loop_enabled" } ] }, { "title": "File Changes", "facts": [ { "name": "Modified", "value": "[beast-gtk/bsttrackview.cc](https://github.com/tim-janik/beast/pull/64/files#diff-0) (66 changes)" }, { "name": "Modified", "value": "[beast-gtk/bsttrackview.hh](https://github.com/tim-janik/beast/pull/64/files#diff-1) (1 changes)" }, { "name": "Modified", "value": "[bse/bseapi.idl](https://github.com/tim-janik/beast/pull/64/files#diff-2) (3 changes)" }, { "name": "Modified", "value": "[bse/bsesong.cc](https://github.com/tim-janik/beast/pull/64/files#diff-3) (45 changes)" }, { "name": "Modified", "value": "[bse/bsesong.hh](https://github.com/tim-janik/beast/pull/64/files#diff-4) (4 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\": 64,\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\": 64\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/64" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/64.patch" } ], "@type": "OpenUri", "name": "View patch" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/64.diff" } ], "@type": "OpenUri", "name": "View diff" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 371515487\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] Port property Song::loop_enabled to C++ (#64)

Gnome - Beast mailing list

Hi Stefan, looks interesting.

I think I would have kept using iview->container plus a few more casts instead of introducing BstTrackView.song, to ensure that iview->container and BstTrackView.song cannot get out of sync.
Your approach might be more convenient and I guess the bst_item_view_set_container() internals ensure that the two members are always set/reset in sync...

There's one other bit missing though. WIth the old proxy interface, signal connections always come in (connect+disconnect) pairs, I'll quote the relevant code lines:

Before your PR:
bse_proxy_disconnect (song.proxy_id(),
"any_signal", track_view_repeat_changed, self,
NULL);
[...]
bse_proxy_connect (song.proxy_id(),
"swapped_signal::property-notify::loop-enabled", track_view_repeat_changed, self,
NULL);

After your PR:
bse_proxy_disconnect (self->song.proxy_id(),
"any_signal", track_view_repeat_changed, self,
NULL);
self->song = NULL; /* disconnect */
[...]
self->song.on ("notify:loop_enabled", self { track_view_repeat_changed (self); });
bse_proxy_connect (self->song.proxy_id(),
NULL);

I.e. you have replaced proxy_connect() with on(), added an event handler disconnect via =NULL but forgot to remove the proxy_disconnect line for track_view_repeat_changed that belongs to the proxy_connect() you removed.

Also please reword "/* disconnect */" to be more explicit: "// disconnects event handlers"


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 #64: Hi Stefan, looks interesting.\r\n\r\nI think I would have kept using iview-\u003econtainer plus a few more casts instead of introducing BstTrackView.song, to ensure that iview-\u003econtainer and BstTrackView.song cannot get out of sync.\r\nYour approach might be more convenient and I guess the bst_item_view_set_container() internals ensure that the two members are always set/reset in sync...\r\n\r\nThere's one other bit missing though. WIth the old proxy interface, signal connections *always* come in (connect+disconnect) pairs, I'll quote the relevant code lines:\r\n\r\nBefore your PR:\r\n bse_proxy_disconnect (song.proxy_id(),\r\n \"any_signal\", track_view_repeat_changed, self,\r\n NULL);\r\n [...]\r\n bse_proxy_connect (song.proxy_id(),\r\n \"swapped_signal::property-notify::loop-enabled\", track_view_repeat_changed, self,\r\n NULL);\r\n\r\nAfter your PR:\r\n bse_proxy_disconnect (self-\u003esong.proxy_id(),\r\n \"any_signal\", track_view_repeat_changed, self,\r\n NULL);\r\n self-\u003esong = NULL; /* disconnect */\r\n [...]\r\n self-\u003esong.on (\"notify:loop_enabled\", [self]() { track_view_repeat_changed (self); });\r\n bse_proxy_connect (self-\u003esong.proxy_id(),\r\n NULL);\r\n\r\nI.e. you have replaced proxy_connect() with on(), added an event handler disconnect via =NULL but forgot to remove the proxy_disconnect line for track_view_repeat_changed that belongs to the proxy_connect() you removed.\r\n\r\nAlso please reword \"/* disconnect */\" to be more explicit: \"// disconnects event handlers\"\r\n"}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/64#issuecomment-415369454"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/64#issuecomment-415369454", "url": "https://github.com/tim-janik/beast/pull/64#issuecomment-415369454", "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] Port property Song::loop_enabled to C++ (#64)", "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\": 64,\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\": 64\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/64#issuecomment-415369454" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 371515487\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] Port property Song::loop_enabled to C++ (#64)

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

@swesterfeld pushed 1 commit.

  • 4cc6f50 BEAST-GTK: bsttrackview: cleanup signal disconnect code


You are receiving this because you are subscribed to this thread.
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 pushed 1 commit in #64"}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/64/files/6ae85257c4b33f96d5151ed3e0502b19e41e3b5d..4cc6f50948989f8949dac78602a10299126ddb88"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/64/files/6ae85257c4b33f96d5151ed3e0502b19e41e3b5d..4cc6f50948989f8949dac78602a10299126ddb88", "url": "https://github.com/tim-janik/beast/pull/64/files/6ae85257c4b33f96d5151ed3e0502b19e41e3b5d..4cc6f50948989f8949dac78602a10299126ddb88", "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": "@swesterfeld pushed 1 commit in #64", "sections": [ { "text": "1 new commit pushed to tim-janik/beast #64:", "activityTitle": "**Stefan Westerfeld**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@swesterfeld", "facts": [ { "name": "4cc6f50", "value": "BEAST-GTK: bsttrackview: cleanup signal disconnect code" } ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/64/files/6ae85257c4b33f96d5151ed3e0502b19e41e3b5d..4cc6f50948989f8949dac78602a10299126ddb88" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 371515487\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] Port property Song::loop_enabled to C++ (#64)

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

There's one other bit missing though. WIth the old proxy interface, signal connections always come in (connect+disconnect) pairs

I.e. you have replaced proxy_connect() with on(), added an event handler disconnect via =NULL but forgot to remove the proxy_disconnect line for track_view_repeat_changed that belongs to the proxy_connect() you removed.

Fixed.

Also please reword "/* disconnect */" to be more explicit: "// disconnects event handlers"

Fixed.

Not sure if you wanted the fixes in an extra commit (I did this) or squashed into the previous commit and delivered as non-linear push.


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 #64: \u003e There's one other bit missing though. WIth the old proxy interface, signal connections always come in (connect+disconnect) pairs\r\n \r\n\u003e I.e. you have replaced proxy_connect() with on(), added an event handler disconnect via =NULL but forgot to remove the proxy_disconnect line for track_view_repeat_changed that belongs to the proxy_connect() you removed.\r\n\r\nFixed.\r\n\r\n\u003e Also please reword \"/* disconnect */\" to be more explicit: \"// disconnects event handlers\"\r\n\r\nFixed.\r\n\r\nNot sure if you wanted the fixes in an extra commit (I did this) or squashed into the previous commit and delivered as non-linear push."}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/64#issuecomment-415386077"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/64#issuecomment-415386077", "url": "https://github.com/tim-janik/beast/pull/64#issuecomment-415386077", "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] Port property Song::loop_enabled to C++ (#64)", "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\": 64,\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\": 64\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/64#issuecomment-415386077" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 371515487\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] Port property Song::loop_enabled to C++ (#64)

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

Merged #64 into master.


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":"Merged #64 into master."}],"action":{"name":"View Pull Request","url":"https://github.com/tim-janik/beast/pull/64#event-1805221122"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/tim-janik/beast/pull/64#event-1805221122", "url": "https://github.com/tim-janik/beast/pull/64#event-1805221122", "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] Port property Song::loop_enabled to C++ (#64)", "sections": [ { "text": "", "activityTitle": "**Tim Janik**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@tim-janik", "facts": [ ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "https://github.com/tim-janik/beast/pull/64#event-1805221122" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 371515487\n}" } ], "themeColor": "26292E" } ]</script>
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast