Skip to content

Conversation

@danirabbit
Copy link
Member

@danirabbit danirabbit commented Aug 13, 2025

Setup playbackmanager actions inside the playbackmanager. Makes sure playback manager functions are private so they're only used through actions. Manage state internally. Simplify setup

@danirabbit danirabbit changed the title PlaybackManager: create play_pause_action PlaybackManager: create actions internally Aug 13, 2025
@danirabbit danirabbit force-pushed the danirabbit/playbackmanager-actions branch from d75f298 to 7e224cf Compare August 13, 2025 17:38
@danirabbit danirabbit marked this pull request as ready for review August 13, 2025 17:44
@danirabbit danirabbit requested a review from a team August 13, 2025 17:46
@danirabbit
Copy link
Member Author

@ryonakano Should I move the action constants to PlaybackManager as well? I wasn't sure if those should stay all centralized or be with the action creation

@ryonakano
Copy link
Member

ryonakano commented Aug 14, 2025

@danirabbit Hmm it's difficult to judge, but maybe they would be better to be kept in Application?

These constants themselves are no longer used in Application but the actions invoked by using these constants are still registered to it. So thinking about where we use them, I feel the former (current) code makes sense and hide internal implementation that the handler of the actions are actually no longer in Application but in PlaybackManager:

        // example 1
        var play_button = new Gtk.Button () {
            action_name = Application.ACTION_PREFIX + Application.ACTION_PLAY_PAUSE,
            child = play_pause_image,
            halign = Gtk.Align.CENTER,
        };

        // example 2
        var play_pause_action = GLib.Application.get_default ().lookup_action (Application.ACTION_PLAY_PAUSE);
        play_pause_action.bind_property ("enabled", seekbar, "sensitive", BindingFlags.SYNC_CREATE);

rather than:

        // example 1
        var play_button = new Gtk.Button () {
            // I think this implicitly make the users of this action, us programmers, to be aware of the internal implementation
            action_name = Application.ACTION_PREFIX + PlaybackManager.ACTION_PLAY_PAUSE,
            child = play_pause_image,
            halign = Gtk.Align.CENTER,
        };

        // example 2
        // Why do we use the constants of PlaybackManager to lookup actions in Application……?
        var play_pause_action = GLib.Application.get_default ().lookup_action (PlaybackManager.ACTION_PLAY_PAUSE);
        play_pause_action.bind_property ("enabled", seekbar, "sensitive", BindingFlags.SYNC_CREATE);

@danirabbit danirabbit requested a review from ryonakano August 14, 2025 14:26
Copy link
Member

@ryonakano ryonakano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed these actions still works. One small nitpicking otherwise LGTM.

@danirabbit danirabbit requested a review from ryonakano August 15, 2025 01:11
Copy link
Member

@ryonakano ryonakano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup! 🧹 ✨

@danirabbit danirabbit merged commit c03e6de into main Aug 15, 2025
3 checks passed
@danirabbit danirabbit deleted the danirabbit/playbackmanager-actions branch August 15, 2025 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants