Skip to content

Conversation

@lollipopkit
Copy link
Owner

@lollipopkit lollipopkit commented Sep 2, 2025

Fixes #795

Summary by Sourcery

Introduce a ContainerStatus enum to standardize container state handling, refactor existing DockerPs and PodmanPs models to use status instead of a running flag, update UI components to rely on status properties, and add comprehensive tests for status parsing and utility methods.

Enhancements:

  • Replace the boolean running flag with a ContainerStatus enum in ContainerPs implementations
  • Add ContainerStatus enum with parsing methods for Docker states and Podman exited values and utility getters (isRunning, canStart, canStop, canRestart, displayName)
  • Update UI code to derive running counts and display text from ContainerStatus properties instead of the deprecated running flag

Tests:

  • Add tests for Docker and Podman status detection covering various state strings and exited values
  • Add unit tests for ContainerStatus utility methods (isRunning, canStart, canStop, canRestart)

@sourcery-ai
Copy link

sourcery-ai bot commented Sep 2, 2025

Reviewer's Guide

This PR refactors container state handling by introducing a ContainerStatus enum (with parsing, display, and utility methods) to replace the old running boolean, updates both DockerPs and PodmanPs to emit status, adjusts UI code to use the new status API, and expands tests to validate parsing and status logic.

Class diagram for ContainerPs, DockerPs, PodmanPs, and ContainerStatus changes

classDiagram
  class ContainerPs {
    <<sealed>>
    String? image
    String? name
    String? cmd
    ContainerStatus status
    String? cpu
    String? mem
    void parseStats(String s)
  }

  class DockerPs {
    String? state
    ContainerStatus status
    void parseStats(String s)
    ContainerStatus get status => ContainerStatus.fromDockerState(state)
  }
  ContainerPs <|-- DockerPs

  class PodmanPs {
    bool? exited
    ContainerStatus status
    void parseStats(String s)
    ContainerStatus get status => ContainerStatus.fromPodmanExited(exited)
  }
  ContainerPs <|-- PodmanPs

  class ContainerStatus {
    <<enum>>
    running
    exited
    created
    paused
    restarting
    removing
    dead
    unknown
    bool get isRunning
    bool get canStart
    bool get canStop
    bool get canRestart
    static ContainerStatus fromDockerState(String? state)
    static ContainerStatus fromPodmanExited(bool? exited)
    String get displayName
  }
Loading

File-Level Changes

Change Details Files
Define ContainerStatus enum with parsing and utility methods
  • Added enum values for all container states
  • Implemented isRunning, canStart, canStop, canRestart getters
  • Added fromDockerState and fromPodmanExited parsing methods
  • Added displayName mapping
lib/data/model/container/status.dart
Replace running boolean with status property in ContainerPs
  • Changed ContainerPs.running to ContainerStatus status
  • Implemented status getters in DockerPs and PodmanPs using parsing methods
lib/data/model/container/ps.dart
Update view to use status API
  • Use status.isRunning instead of running boolean for counters
  • Show status.displayName in item labels
  • Pass status.isRunning to menu builder
lib/view/page/container/container.dart
Expand tests to cover status parsing and utilities
  • Updated existing DockerPs parse test to expect status
  • Added tests for DockerPs.fromDockerState and PodmanPs.fromPodmanExited
  • Added tests for ContainerStatus utility methods (isRunning, canStart, etc.)
test/container_test.dart

Assessment against linked issues

Issue Objective Addressed Explanation
#795 Correct the logic for counting running Docker containers so that only containers with a truly 'running' status are counted as running.
#795 Update the code to accurately parse and distinguish container states (e.g., running, exited, created, etc.) for Docker containers.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@lollipopkit
Copy link
Owner Author

@claude review

@claude
Copy link

claude bot commented Sep 2, 2025

Claude finished @lollipopkit's task —— View job


Docker Status Parsing Refactor Review

This is a well-executed refactoring that addresses Docker container status parsing. Here's my comprehensive review:

Todo List:

  • Read all changed files to understand the refactoring
  • Analyze the new Docker status parsing implementation
  • Review test coverage and implementation
  • Check for code quality and best practices
  • Provide comprehensive review feedback

🎯 Overall Assessment: EXCELLENT

This refactoring significantly improves the container status handling system with better type safety, extensibility, and robustness.

Strengths

1. Strong Type Safety Improvement

  • Before: Simple bool running property with inconsistent logic
  • After: Rich ContainerStatus enum with 8 distinct states (running, exited, created, paused, etc.)
  • Impact: Eliminates potential bugs from boolean oversimplification

2. Robust Docker State Parsing (lib/data/model/container/status.dart:29-43)

static ContainerStatus fromDockerState(String? state) {
  if (state == null || state.isEmpty) return ContainerStatus.unknown;
  final lowerState = state.toLowerCase();
  if (lowerState.startsWith('up')) return ContainerStatus.running;
  // ... comprehensive parsing logic
}
  • Case-insensitive parsing handles real-world Docker output variations
  • Null safety with proper fallback to unknown status
  • Comprehensive coverage of Docker container states

3. Excellent Test Coverage (test/container_test.dart:35-117)

  • 87 new test lines covering edge cases, null handling, and various state formats
  • Tests include case-insensitive parsing ('UP 30 seconds', 'up 5 days')
  • Validates utility methods (canStart, canStop, canRestart)
  • Regression protection for the original parsing functionality

4. Clean API Design

The new ContainerStatus enum provides intuitive helper methods:

bool get isRunning => this == ContainerStatus.running;
bool get canStart => this == ContainerStatus.exited || this == ContainerStatus.created || this == ContainerStatus.dead;

5. Backward Compatibility

  • All existing running property usage replaced with status.isRunning
  • UI correctly displays both raw state and parsed status: ps.state ?? ps.status.displayName

💡 Minor Suggestions

1. Documentation Enhancement

Consider adding class-level documentation for the ContainerStatus enum:

/// Represents the various states a container can be in.
/// Supports both Docker and Podman container status parsing.
enum ContainerStatus {

2. Potential Edge Case (lib/data/model/container/status.dart:34-40)

The current Docker parsing uses contains() for some states. Consider if this could cause conflicts:

if (lowerState.contains('exited')) return ContainerStatus.exited;

This should work fine, but for future robustness, more specific parsing might be beneficial.

🔍 Code Quality Assessment

Aspect Rating Notes
Type Safety ⭐⭐⭐⭐⭐ Excellent enum-based approach
Error Handling ⭐⭐⭐⭐⭐ Comprehensive null/edge case handling
Test Coverage ⭐⭐⭐⭐⭐ Thorough testing with edge cases
API Design ⭐⭐⭐⭐⭐ Intuitive helper methods
Performance ⭐⭐⭐⭐⭐ No performance concerns
Maintainability ⭐⭐⭐⭐⭐ Well-structured and extensible

Verification Points

  • Docker parsing handles Up X minutes/hours/days correctly
  • Podman parsing properly converts exited boolean to status
  • UI integration maintains display functionality
  • Menu system correctly uses isRunning for action availability
  • Edge cases like null states handled gracefully

🏆 Conclusion

This refactoring successfully addresses issue #795 by implementing a robust, type-safe container status parsing system. The comprehensive test coverage and thoughtful API design make this a high-quality improvement that enhances both reliability and maintainability.

Recommendation: APPROVE


Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes - here's some feedback:

  • Consider moving the hard-coded status.displayName strings into your l10n resources to support localization instead of using English literals.
  • Unify the UI status display by using ContainerStatus.displayName for DockerPs items as well (instead of raw state) so both Docker and Podman show statuses consistently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving the hard-coded status.displayName strings into your l10n resources to support localization instead of using English literals.
- Unify the UI status display by using ContainerStatus.displayName for DockerPs items as well (instead of raw state) so both Docker and Podman show statuses consistently.

## Individual Comments

### Comment 1
<location> `lib/data/model/container/status.dart:53` </location>
<code_context>
+  }
+
+  /// Get display string for the status
+  String get displayName {
+    return switch (this) {
+      ContainerStatus.running => 'Running',
</code_context>

<issue_to_address>
Hardcoded display names may limit localization support.

Consider using a localization mechanism for displayName to support multiple languages if your app requires internationalization.

Suggested implementation:

```
  /// Get localized display string for the status
  String displayName(BuildContext context) {
    final localizations = AppLocalizations.of(context);
    return switch (this) {
      ContainerStatus.running => localizations.containerStatusRunning,
      ContainerStatus.exited => localizations.containerStatusExited,
      ContainerStatus.created => localizations.containerStatusCreated,
      ContainerStatus.paused => localizations.containerStatusPaused,
      ContainerStatus.restarting => localizations.containerStatusRestarting,
      ContainerStatus.removing => localizations.containerStatusRemoving,
      ContainerStatus.dead => localizations.containerStatusDead,
      ContainerStatus.unknown => localizations.containerStatusUnknown,
    };
  }

```

1. You must add the corresponding keys (e.g., `containerStatusRunning`, `containerStatusExited`, etc.) to your `intl` ARB files and regenerate your localization classes.
2. Update all usages of `displayName` to pass a `BuildContext` parameter, e.g., `status.displayName(context)`.
3. Ensure `AppLocalizations` is imported: `import 'package:flutter_gen/gen_l10n/app_localizations.dart';`
</issue_to_address>

### Comment 2
<location> `lib/view/page/container/container.dart:223` </location>
<code_context>
-              final PodmanPs ps => ps.running ? l10n.running : l10n.stopped,
-              final DockerPs ps => ps.state,
+              final PodmanPs ps => ps.status.displayName,
+              final DockerPs ps => ps.state ?? ps.status.displayName,
             }}',
             style: UIs.text13Grey,
</code_context>

<issue_to_address>
Fallback to displayName may cause inconsistent UI if state is not localized.

Ensure both ps.state and ps.status.displayName are consistently localized to avoid mismatched or untranslated status values in the UI.

Suggested implementation:

```
            '${item.image ?? l10n.unknown} - ${switch (item) {
              final PodmanPs ps => l10n.localizeStatus(ps.status.displayName),
              final DockerPs ps => l10n.localizeStatus(ps.state ?? ps.status.displayName),
            }}',

```

If `l10n.localizeStatus` does not exist, you need to implement it in your localization class. It should accept a status string and return the localized version, falling back to the original string if no translation is available. Example:

```dart
// In your l10n class
String localizeStatus(String status) {
  switch (status) {
    case 'Running':
      return running; // already localized
    case 'Stopped':
      return stopped; // already localized
    // Add other status cases as needed
    default:
      return status;
  }
}
```
Make sure to update the localization files if you add new status values.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

}

/// Get display string for the status
String get displayName {
Copy link

Choose a reason for hiding this comment

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

suggestion: Hardcoded display names may limit localization support.

Consider using a localization mechanism for displayName to support multiple languages if your app requires internationalization.

Suggested implementation:

  /// Get localized display string for the status
  String displayName(BuildContext context) {
    final localizations = AppLocalizations.of(context);
    return switch (this) {
      ContainerStatus.running => localizations.containerStatusRunning,
      ContainerStatus.exited => localizations.containerStatusExited,
      ContainerStatus.created => localizations.containerStatusCreated,
      ContainerStatus.paused => localizations.containerStatusPaused,
      ContainerStatus.restarting => localizations.containerStatusRestarting,
      ContainerStatus.removing => localizations.containerStatusRemoving,
      ContainerStatus.dead => localizations.containerStatusDead,
      ContainerStatus.unknown => localizations.containerStatusUnknown,
    };
  }

  1. You must add the corresponding keys (e.g., containerStatusRunning, containerStatusExited, etc.) to your intl ARB files and regenerate your localization classes.
  2. Update all usages of displayName to pass a BuildContext parameter, e.g., status.displayName(context).
  3. Ensure AppLocalizations is imported: import 'package:flutter_gen/gen_l10n/app_localizations.dart';

final PodmanPs ps => ps.running ? l10n.running : l10n.stopped,
final DockerPs ps => ps.state,
final PodmanPs ps => ps.status.displayName,
final DockerPs ps => ps.state ?? ps.status.displayName,
Copy link

Choose a reason for hiding this comment

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

suggestion: Fallback to displayName may cause inconsistent UI if state is not localized.

Ensure both ps.state and ps.status.displayName are consistently localized to avoid mismatched or untranslated status values in the UI.

Suggested implementation:

            '${item.image ?? l10n.unknown} - ${switch (item) {
              final PodmanPs ps => l10n.localizeStatus(ps.status.displayName),
              final DockerPs ps => l10n.localizeStatus(ps.state ?? ps.status.displayName),
            }}',

If l10n.localizeStatus does not exist, you need to implement it in your localization class. It should accept a status string and return the localized version, falling back to the original string if no translation is available. Example:

// In your l10n class
String localizeStatus(String status) {
  switch (status) {
    case 'Running':
      return running; // already localized
    case 'Stopped':
      return stopped; // already localized
    // Add other status cases as needed
    default:
      return status;
  }
}

Make sure to update the localization files if you add new status values.

@lollipopkit lollipopkit merged commit 9290612 into main Sep 2, 2025
1 check passed
@lollipopkit lollipopkit deleted the lollipopkit/issue795 branch September 2, 2025 05:22
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.

docker界面,对于容器状态的统计信息有误

2 participants