Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port property Song::loop_enabled to C++ #64

Merged
merged 4 commits into from Aug 23, 2018

Conversation

@swesterfeld
Copy link
Collaborator

@swesterfeld swesterfeld commented Aug 23, 2018

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.

swesterfeld added 3 commits Aug 23, 2018
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@tim-janik
Copy link
Owner

@tim-janik tim-janik commented Aug 23, 2018

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"

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@swesterfeld
Copy link
Collaborator Author

@swesterfeld swesterfeld commented Aug 23, 2018

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.

@tim-janik tim-janik merged commit 4cc6f50 into tim-janik:master Aug 23, 2018
1 check passed
tim-janik added a commit that referenced this issue Aug 23, 2018
* swesterfeld-song-loop-enabled:
  BEAST-GTK: bsttrackview: cleanup signal disconnect code
  BEAST-GTK: bsttrackview: use C++ property Song::loop_enabled
  BEAST-GTK: bsttrackview: introduce scoped handle Bse::SongS
  BSE: Song::loop_enabled: port property to C++

Signed-off-by: Tim Janik <timj@gnu.org>
@swesterfeld swesterfeld deleted the pport-song-loop-enabled branch Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants