-
Notifications
You must be signed in to change notification settings - Fork 78
runtime config clean #258
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
base: dev
Are you sure you want to change the base?
runtime config clean #258
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 introduces runtime configuration capabilities for liblsl to support sandboxed environments where traditional file paths may be inaccessible. It enables applications to specify configuration either via direct string content or custom file paths before LSL initialization.
Changes:
- Added two new C API functions (
lsl_set_config_filenameandlsl_set_config_content) for runtime configuration - Refactored configuration loading logic to support multiple configuration sources with defined precedence
- Updated documentation to reflect the new configuration priority order
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/common.cpp | Implements the new C API wrapper functions for setting config filename and content |
| src/api_config.h | Declares new static methods and member variables for runtime config, updates documentation |
| src/api_config.cpp | Implements configuration loading from content/custom files with proper precedence handling |
| include/lsl/common.h | Adds public C API declarations for the new configuration functions |
Comments suppressed due to low confidence (2)
include/lsl/common.h:1
- The documentation for
lsl_set_config_filenamecontains a sentence fragment. The sentence starting with 'If, and only if...' should be integrated into the previous sentence or rewritten as a complete sentence. Consider: 'This setting only takes effect if this function is called before any other LSL function.'
#pragma once
include/lsl/common.h:1
- The documentation for
lsl_set_config_contentcontains the same sentence fragment aslsl_set_config_filename. The sentence starting with 'If, and only if...' should be integrated into the previous sentence or rewritten as a complete sentence. Consider: 'This setting only takes effect if this function is called before any other LSL function.'
#pragma once
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static void set_api_config_content(const std::string &content) { | ||
| api_config_content_ = content; | ||
| } |
Copilot
AI
Jan 19, 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.
Inconsistent indentation: this line uses spaces instead of tabs like the surrounding code. The function should be indented with a single tab to match the style of other methods in this class.
| static void set_api_config_content(const std::string &content) { | |
| api_config_content_ = content; | |
| } | |
| static void set_api_config_content(const std::string &content) { | |
| api_config_content_ = content; | |
| } |
Comparison with PR #152There's an older PR #152 by @tstenner that also attempted to make the config runtime-configurable. Here's how the two approaches differ: PR #152 (tstenner) - Architectural Refactor
PR #245 (zeyus) - Minimal Feature Addition
// New C API functions
extern LIBLSL_C_API void lsl_set_config_filename(const char *filename);
extern LIBLSL_C_API void lsl_set_config_content(const char *content);Summary
PR #245's approach is simpler and addresses the immediate need for sandboxed platforms. PR #152's set_option() could be added later as an enhancement. I'm planning to cherry-pick the relevant commits from this PR onto a clean branch to resolve the merge conflicts. Credit will be preserved via git history. |
Clean rebase of #245 by @zeyus
Original PR: #245
Which was itself a redo of #234 but from a dedicated branch and targeting
cboulay/apple_framework