Fix property value updates in InitiatorWidget by implementing FULL/PARTIAL subscription handling #69
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…RTIAL subscription handling - Add missing FULL and PARTIAL constants to MidiCISubscriptionCommand in PropertyCommonRules.hpp - Update PropertyClientFacade::process_subscribe_property() to handle FULL/PARTIAL commands - Extract property values from subscription messages and update observable property list - Use proper namespace qualification for MidiCISubscriptionCommand constants - Mirror Kotlin implementation's property value update mechanism This fixes the issue where property values were not being reflected in InitiatorWidget when choosing properties, as only ResourceData and DeviceInfo were working correctly. Co-Authored-By: Atsushi Eno <atsushieno@gmail.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
- Add status validation (check status == 200) before updating properties in process_get_data_reply() - Extract media type from reply header and pass to updateValue() method - Default to 'application/json' if media type not specified - Fix subscription handling to pass media type parameter to updateValue() - Both subscription and data reply paths now use consistent 3-parameter updateValue() signature - Mirrors Kotlin implementation's approach to property value updates Co-Authored-By: Atsushi Eno <atsushieno@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix property value updates in InitiatorWidget by implementing FULL/PARTIAL subscription handling
Problem
When choosing a property in the InitiatorWidget, property values were not being correctly updated in the text editor widget. Only
ResourceDataandDeviceInfovalues were working correctly, while other properties that rely on subscription messages were not displaying their values.Root Cause
The C++
PropertyClientFacade::process_subscribe_property()method was incomplete compared to the Kotlin implementation. It only handled "NOTIFY" commands by requesting data, but was missing the logic to process "FULL" and "PARTIAL" subscription commands that contain actual property values.Solution
This PR implements the missing functionality to fully mirror the Kotlin implementation:
Added missing constants: Added
FULLandPARTIALconstants toMidiCISubscriptionCommandinPropertyCommonRules.hppEnhanced subscription processing: Updated
PropertyClientFacade::process_subscribe_property()to:ENDcommands (early return)NOTIFYcommands (existing behavior - send GetPropertyData request)FULLandPARTIALcommands (new functionality):properties_->updateValue()Used proper constants: Replaced all string literals with
property_common_rules::MidiCISubscriptionCommandconstants throughout the implementationChanges Made
include/midicci/properties/PropertyCommonRules.hpp: Added FULL and PARTIAL constantssrc/midicci/properties/PropertyClientFacade.cpp:process_subscribe_property()method with complete subscription handlingTesting
This fix ensures that property values are now correctly reflected in the InitiatorWidget when choosing properties, resolving the issue where only ResourceData and DeviceInfo were working.
Link to Devin run: https://app.devin.ai/sessions/c590d62d894b4f408852d70e8775b71d
Requested by: Atsushi Eno (atsushieno@gmail.com)