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

RDK-51273: Add IAnalytics interface #379

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adrianM27
Copy link
Contributor

No description provided.

@adrianM27 adrianM27 force-pushed the master branch 3 times, most recently from 9aa4cdd to d5455c3 Compare October 29, 2024 13:26
struct EXTERNAL IAnalytics : virtual public Core::IUnknown {
enum { ID = ID_ANALYTICS };

virtual ~IAnalytics() override = default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for virtual and override are mutual exclusive. Use one or the other. So just remove virtual here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// @param uptimeTimestamp: Uptime timestamp of the event
// @param eventPayload: Payload of the event

virtual Core::hresult SendEvent(const string& eventName /* @in @text eventName*/,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can define @text:keep on interface level, than you do not need to use the @text on all paramaters. Might be easier to maintain..

See https://rdkcentral.github.io/Thunder/plugin/interfaces/tags/

Snippit taken from there: "When used for a whole interface it must be used in the form '@text:keep'. It will in this case make the JSON generator use the name of the above items in JSON code exactly as specified in the interface, including the case of the text. Please note that of course the interface designer is responsible for making the interface compliant and consistent with the interface guideliness in use (even more perhaps then with the other uses of @text as now the whole interface is influenced). Please see an example on how to apply this form of @text below in the examples."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@adrianM27 adrianM27 force-pushed the master branch 4 times, most recently from 7d918b7 to 85c9da6 Compare November 12, 2024 12:36
@adrianM27
Copy link
Contributor Author

Thank you @pwielders , I updated with requested changes

struct EXTERNAL IAnalytics : virtual public Core::IUnknown {
enum { ID = ID_ANALYTICS };

virtual ~IAnalytics() = default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@adrianM27 one small remark ;-) As we are inheriting from Core::IUnknown, the destructor is already virtual. Using the override keyword the compiler will check that the base (destructor of Core::IUnknown) is virtual. Hence why I assumed you would drop the virtual keywrd and have the compiler check that the destructor of the base class is really virtual.
So suggest to state here: ~IAnalytics() override = default;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pwielders , updated

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.

2 participants