-
Notifications
You must be signed in to change notification settings - Fork 82
add PersistLoadWatermark #482
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR extends the IWatermark interface to support persistent watermark features by adding a new method. The change originated as a patch in RDK-V and is being brought into ThunderInterfaces.
Changes:
- Added
PersistLoadWatermark(uint32_t id)method to the IWatermark interface
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| virtual bool ShowWatermark(const bool show) = 0; | ||
| virtual bool CreateWatermark(uint32_t id, uint32_t zorder) = 0; | ||
| virtual bool UpdateWatermark(uint32_t id, uint32_t key, uint32_t size) = 0; | ||
| virtual bool PersistLoadWatermark(uint32_t id) = 0; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method name "PersistLoadWatermark" combines two distinct operations (persist and load) which violates the single responsibility principle. This creates ambiguity about what the method does. Consider splitting this into two separate methods: "PersistWatermark" for saving and "LoadWatermark" for loading, or clarify the intended behavior with a more descriptive name like "LoadPersistedWatermark" if it only loads a previously persisted watermark.
| virtual bool PersistLoadWatermark(uint32_t id) = 0; | |
| virtual bool LoadPersistedWatermark(uint32_t id) = 0; |
| @@ -36,6 +36,7 @@ namespace Exchange { | |||
| virtual bool ShowWatermark(const bool show) = 0; | |||
| virtual bool CreateWatermark(uint32_t id, uint32_t zorder) = 0; | |||
| virtual bool UpdateWatermark(uint32_t id, uint32_t key, uint32_t size) = 0; | |||
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method lacks documentation explaining its purpose, parameters, and return value semantics. Since this is an interface method being added to support persistent watermark features, it should include documentation that explains: what "persist and load" means in this context, what the 'id' parameter represents, and what the boolean return value indicates (success/failure conditions). This is especially important for interface methods that will be implemented by external parties.
| virtual bool UpdateWatermark(uint32_t id, uint32_t key, uint32_t size) = 0; | |
| virtual bool UpdateWatermark(uint32_t id, uint32_t key, uint32_t size) = 0; | |
| /** | |
| * Persist and/or load the state of the watermark identified by the given ID. | |
| * | |
| * Implementations typically use this method to store the current configuration | |
| * of a watermark to persistent storage and/or to restore a previously stored | |
| * configuration for the same watermark. The exact persist/load behavior may | |
| * depend on the concrete implementation. | |
| * | |
| * @param id | |
| * The unique identifier of the watermark whose state should be persisted | |
| * and/or loaded. | |
| * | |
| * @return | |
| * true if the persist/load operation completed successfully; false if the | |
| * operation failed (for example due to invalid ID, missing persisted data, | |
| * or storage errors). | |
| */ |
| virtual bool ShowWatermark(const bool show) = 0; | ||
| virtual bool CreateWatermark(uint32_t id, uint32_t zorder) = 0; | ||
| virtual bool UpdateWatermark(uint32_t id, uint32_t key, uint32_t size) = 0; | ||
| virtual bool PersistLoadWatermark(uint32_t id) = 0; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new method to an existing interface is a breaking change for the API. This change will break binary compatibility because any existing implementations of the IWatermark interface will not have this method implemented, causing vtable mismatches. Consider versioning the interface (e.g., IWatermark2) or using a COM versioning strategy to maintain backward compatibility with existing implementations.
RDK-42408 : Extend IWatermark to support Persistent Watermark Feature
this existed as a patch in RDK-V
bringing it to ThunderInterfaces