Skip to content

Conversation

CarterLi
Copy link
Member

No description provided.

CarterLi and others added 30 commits July 31, 2025 14:34
Moves module option structs out of global config and removes dynamic allocation, using static buffers and size assertions for safer, simpler management.

Refactors module initialization and destruction to work with isolated option structs, eliminating the need for the previous modules container and related code.

Unifies module option header usage and updates detection logic to leverage statically-sized option buffers, improving encapsulation and maintainability.

Simplifies module function signatures and reduces coupling between modules and config.

Enhances future extensibility and reliability by enforcing max size constraints and removing unnecessary indirections.
@CarterLi CarterLi requested a review from Copilot August 14, 2025 03:05
Copy link
Contributor

@Copilot Copilot AI left a 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 pull request refactors the module system architecture by removing the FFModuleBaseInfo moduleInfo field from module option structures and making module info externally accessible. This architectural change reduces memory usage and improves modularity.

Key changes include:

  • Removed moduleInfo field from all module option structures
  • Made module info globally accessible via extern declarations
  • Added size assertions to ensure option structures fit within maximum allowed size
  • Updated module initialization to use external module info references

Reviewed Changes

Copilot reviewed 300 out of 363 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/modules/swap/option.h Removed moduleInfo field and added size assertion
src/modules/sound/sound.h Added extern declaration for module info
src/modules/sound/sound.c Refactored module info to be external and updated parsing
src/modules/sound/option.h Removed moduleInfo field and added size assertion
src/modules/shell/shell.h Added extern declaration for module info
src/modules/shell/shell.c Refactored module info to be external and updated parsing
src/modules/shell/option.h Removed moduleInfo field and added size assertion
src/modules/separator/separator.h Added extern declaration for module info
src/modules/separator/separator.c Refactored module info and fixed hardcoded TODO comment
src/modules/separator/option.h Removed moduleInfo field and added size assertion
src/modules/publicip/publicip.h Added extern declaration and fixed naming inconsistency
src/modules/publicip/publicip.c Refactored module info to be external and updated parsing
src/modules/publicip/option.h Fixed struct naming and removed moduleInfo field
Comments suppressed due to low confidence (3)

src/modules/physicaldisk/physicaldisk.c:95

  • The condition dev->temperature == dev->temperature is always true and doesn't check for FF_PHYSICALDISK_TEMP_UNSET. This should be dev->temperature != FF_PHYSICALDISK_TEMP_UNSET.
            if (dev->temperature != FF_PHYSICALDISK_TEMP_UNSET)

src/modules/gpu/gpu.c:49

  • The condition gpu->temperature == gpu->temperature is always true and doesn't check for FF_GPU_TEMP_UNSET. This should be gpu->temperature != FF_GPU_TEMP_UNSET.
        if(gpu->temperature != FF_GPU_TEMP_UNSET)

src/modules/gpu/gpu.c:129

  • The condition gpu->coreUsage == gpu->coreUsage is always true and doesn't check for FF_GPU_CORE_USAGE_UNSET. This should be gpu->coreUsage != FF_GPU_CORE_USAGE_UNSET.
        if (gpu->coreUsage != FF_GPU_CORE_USAGE_UNSET)

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.


ffStrbufInit(&options->namePrefix);
options->defaultRouteOnly =
#if __ANDROID__
Copy link
Preview

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

The conditional compilation directive was changed from #if __ANDROID__ || __OpenBSD__ to just #if __ANDROID__, removing the OpenBSD condition. This change in behavior should be verified as intentional.

Suggested change
#if __ANDROID__
#if defined(__ANDROID__) || defined(__OpenBSD__)

Copilot uses AI. Check for mistakes.

""
#elif __OpenBSD__
""
#elif __Haiku__
Copy link
Preview

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

[nitpick] Adding a new platform condition for Haiku without a corresponding icon assignment (empty string). Consider adding an appropriate icon for consistency.

Copilot uses AI. Check for mistakes.

setlocale(LC_CTYPE, "");
mbstate_t state = {};
bool fqdn = instance.config.modules.title.fqdn;
bool fqdn = true; // TODO: Make this configurable
Copy link
Preview

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The hardcoded value true with a TODO comment suggests this should be made configurable. Consider addressing this technical debt or creating a tracking issue.

Suggested change
bool fqdn = true; // TODO: Make this configurable
bool fqdn = options->fqdn; // Now configurable via FFSeparatorOptions; ensure struct is updated accordingly

Copilot uses AI. Check for mistakes.

@CarterLi CarterLi merged commit a99b6d6 into master Aug 14, 2025
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.

6 participants