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.

Sign up for GitHub

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

Jump to bottom

Refactor callback contexts #504

Merged
merged 9 commits into from
Jan 11, 2023
Merged

Refactor callback contexts #504

merged 9 commits into from
Jan 11, 2023

Conversation

FelonEkonom
Copy link
Member

@FelonEkonom FelonEkonom commented Jan 4, 2023 •
edited by jira bot
Loading

@FelonEkonom FelonEkonom self-assigned this Jan 4, 2023
@FelonEkonom FelonEkonom force-pushed the refactor-callback-contexts branch 3 times, most recently from 43ef8b5 to 0dfdbd9 Compare January 9, 2023 14:48
@FelonEkonom FelonEkonom force-pushed the refactor-callback-contexts branch 2 times, most recently from b8ee62d to eb44237 Compare January 10, 2023 12:26
@FelonEkonom FelonEkonom marked this pull request as ready for review January 10, 2023 12:37
mat-hek
mat-hek requested changes Jan 10, 2023
View reviewed changes
Copy link
Member

@mat-hek mat-hek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

AFAIR we decided to document callback-specific fields in these callbacks' docs

lib/membrane/core/child/lifecycle_controller.ex Outdated
@@ -7,13 +7,13 @@ defmodule Membrane.Core.Child.LifecycleController do
@spec handle_parent_notification(Membrane.ParentNotification.t(), Membrane.Core.Child.state_t()) ::
Membrane.Core.Child.state_t()
def handle_parent_notification(notification, state) do
context = Component.callback_context_generator(:child, ParentNotification, state)
# context = Component.callback_context(state)
Copy link
Member

Choose a reason for hiding this comment

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

leftover

lib/membrane/core/parent/lifecycle_controller.ex Outdated
@@ -22,13 +22,13 @@ defmodule Membrane.Core.Parent.LifecycleController do
@spec handle_setup(Parent.state_t()) :: Parent.state_t()
def handle_setup(state) do
Membrane.Logger.debug("Setup")
context = Component.callback_context_generator(:parent, Setup, state)
# context = Component.callback_context(state)
Copy link
Member

@mat-hek mat-hek Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Another leftover. There are more, fix them all

lib/membrane/core/element/callback_context.ex Outdated
playback: Membrane.Playback.t(),
resource_guard: Membrane.ResourceGuard.t(),
utility_supervisor: Membrane.UtilitySupervisor.t()
@type option_t ::
Copy link
Member

Choose a reason for hiding this comment

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

  • It seems that the singular form is not needed (we can leave options_t only)
  • This should be called optional_fields_t or callback_specific_fields_t

Copy link
Member Author

Choose a reason for hiding this comment

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

I will change a name, but singular form is used in Membrane.Core.Component

Copy link
Member

Choose a reason for hiding this comment

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

Only to create the plural form again :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but otherwise we would have to duplicate every option in code and put eg. {:incoming_demand, non_neg_integer()} in Membrane.Core.Element.CallbackContext and in Membrane.Core.Componenet as well

Copy link
Member Author

Choose a reason for hiding this comment

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

If you know, how to do it without repeating every key and value type in these 2 modules, let me know

Copy link
Member

Choose a reason for hiding this comment

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

In Membrane.Core.Component there should be an alternative of lists instead of a list with alternative:

options_t :: Membrane.Core.Element.CallbackContext.options_t | Membrane.Core.Bin.CallbackContext.options_t | Membrane.Core.Pipeline.CallbackContext.options_t

@FelonEkonom FelonEkonom requested review from mat-hek and removed request for mat-hek January 11, 2023 10:13
@FelonEkonom FelonEkonom merged commit fff0f40 into master Jan 11, 2023
@FelonEkonom FelonEkonom deleted the refactor-callback-contexts branch January 11, 2023 12:47
@djanda97 djanda97 mentioned this pull request Apr 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mat-hek mat-hek mat-hek approved these changes

@varsill varsill Awaiting requested review from varsill

Assignees

@FelonEkonom FelonEkonom

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FelonEkonom @mat-hek