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

[MediaController] Add new API to check Response request #5672

Merged

Conversation

hsgwon
Copy link
Contributor

@hsgwon hsgwon commented Oct 30, 2023

Description of Change

Add new API to check whether sender ask Respose for command or not.
Fix crash issue when request_id is null.

API Changes

  • TCSACR-560

@hsgwon hsgwon added ACR Required API12 Platform : Tizen 9.0 / TFM: net6.0-tizen9.0 labels Oct 30, 2023
@hsgwon hsgwon requested a review from eunhaechoi October 30, 2023 03:22
@TizenAPI-Bot
Copy link
Collaborator

Public API Changed

Please follow the ACR process for the changed API below.

Added: 1, Removed: 0, Changed: 0

Added

+ /// <since_tizen>12</since_tizen
+ System.Boolean Tizen.Multimedia.Remoting.Command::NeedToResponse()

}

/// <summary>
/// Gets the status of response.
Copy link
Member

Choose a reason for hiding this comment

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

how it sounds suggests that it can change, like when Response is supplied it would be set to false then,
suggestion: "Indicates whether a response is expected for this command." or "Indicates if the command should be responded to" or shorter brief "Indicates if the response is needed."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to 'Indicates if the response is needed.'

/// <summary>
/// Gets the status of response.
/// </summary>
/// <remarks>If false, the receiver should not response for the received command.</remarks>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <remarks>If false, the receiver should not response for the received command.</remarks>
/// <remarks>If false, the receiver should not respond to the received command.</remarks>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// </summary>
/// <remarks>If false, the receiver should not response for the received command.</remarks>
/// <since_tizen> 12 </since_tizen>
public bool NeedToResponse => _requestId != null;
Copy link
Member

Choose a reason for hiding this comment

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

please change the name of this property, like "IsResponseNeeded", "ResponseNeeded", "NeedToRespond", it cannot be [to + noun]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to 'IsResponseNeeded'. But 'NeedToResponse' is already used in VD, so I'll keep it internal api only.

@@ -90,6 +103,11 @@ internal void SetResponseInformation(string receiverId, string requestId)
/// <param name="bundle">The extra data.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Could you change the description at line 99 (and for the next method)? Like "Sends command response to the client" or "Responds to the clients command"
Unfortunately we cannot change the name of the method, "Response" is a noun and method names should be verb (or centered around verb), so it should be Respond() or SendResponse()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to Responds to the clients command..

@TizenAPI-Bot
Copy link
Collaborator

Public API Changed

Please follow the ACR process for the changed API below.

Added: 17, Removed: 0, Changed: 0

Added

+ /// <since_tizen>12</since_tizen
+ System.Boolean Tizen.Multimedia.Remoting.Command::IsResponseNeeded()

+ /// <since_tizen>none</since_tizen
+ System.Void Tizen.NUI.Scene3D.Model::Dispose(Tizen.NUI.DisposeTypes)

+ /// <since_tizen>10</since_tizen
+ Tizen.System.PowerLockStateChangedEventArgs

+ /// <since_tizen>none</since_tizen
+ Tizen.System.PowerLock Tizen.System.PowerLockStateChangedEventArgs::PowerLockType()

+ /// <since_tizen>none</since_tizen
+ Tizen.System.PowerLockState Tizen.System.PowerLockStateChangedEventArgs::PowerLockState()

+ /// <since_tizen>10</since_tizen
+ Tizen.System.PowerStateChangeRequestEventArgs

+ /// <since_tizen>none</since_tizen
+ System.Int32 Tizen.System.PowerStateChangeRequestEventArgs::Retval()

+ /// <since_tizen>none</since_tizen
+ Tizen.System.PowerState Tizen.System.PowerStateChangeRequestEventArgs::PowerState()

+ /// <since_tizen>10</since_tizen
+ Tizen.System.PowerStateWaitEventArgs

+ /// <since_tizen>none</since_tizen
+ System.UInt64 Tizen.System.PowerStateWaitEventArgs::WaitCallbackId()

+ /// <since_tizen>none</since_tizen
+ Tizen.System.PowerState Tizen.System.PowerStateWaitEventArgs::NextState()

+ /// <since_tizen>none</since_tizen
+ Tizen.System.PowerState Tizen.System.PowerStateWaitEventArgs::PrevState()

+ /// <since_tizen>none</since_tizen
+ Tizen.System.PowerTransitionReason Tizen.System.PowerStateWaitEventArgs::TransitionReason()

+ /// <since_tizen>10</since_tizen
+ Tizen.System.PowerTransientStateWaitEventArgs

+ /// <since_tizen>none</since_tizen
+ System.UInt64 Tizen.System.PowerTransientStateWaitEventArgs::WaitCallbackId()

+ /// <since_tizen>none</since_tizen
+ Tizen.System.PowerTransientState Tizen.System.PowerTransientStateWaitEventArgs::TransientState()

+ /// <since_tizen>none</since_tizen
+ Tizen.System.PowerTransitionReason Tizen.System.PowerTransientStateWaitEventArgs::TransitionReason()

Internal API Changed

Added: 110, Removed: 5, Changed: 0

@TizenAPI-Bot
Copy link
Collaborator

Public API Changed

Please follow the ACR process for the changed API below.

Added: 1, Removed: 0, Changed: 0

Added

+ /// <since_tizen>12</since_tizen
+ System.Boolean Tizen.Multimedia.Remoting.Command::IsResponseNeeded()

Internal API Changed

Added: 1, Removed: 0, Changed: 0

Added

+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.Boolean Tizen.Multimedia.Remoting.Command::NeedToResponse()

@hsgwon hsgwon merged commit cac41f2 into Samsung:master Jan 2, 2024
3 checks passed
@hsgwon hsgwon deleted the api12.MediaController.FixCrash.request_id branch January 2, 2024 01:18
everLEEst pushed a commit that referenced this pull request Jan 2, 2024
* [MediaController] Add new API to check Response request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACR Required API12 Platform : Tizen 9.0 / TFM: net6.0-tizen9.0 Internal API Changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants