Skip to content
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

Fix multiple app filtering in Linux #100

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

fotiDim
Copy link
Contributor

@fotiDim fotiDim commented Nov 20, 2024

PR Type

enhancement, bug_fix


Description

  • Improved the BLE device filtering mechanism by simplifying the scan filter logic and removing unnecessary custom filter checks.
  • Added stream subscriptions to handle device addition and removal events, improving the responsiveness of the application.
  • Implemented proper setup and disposal of listeners to manage device events efficiently.
  • Removed redundant code, including an extension method for checking custom filters, to streamline the codebase.

Changes walkthrough 📝

Relevant files
Enhancement
universal_ble_linux.dart
Improve BLE device filtering and event handling in Linux 

lib/src/universal_ble_linux/universal_ble_linux.dart

  • Added stream subscriptions for device addition and removal.
  • Simplified the scan filter logic by removing custom filter checks.
  • Implemented listener setup and disposal for device events.
  • Removed redundant extension method for custom filter check.
  • +16/-35 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Resource Management
    Verify that stream subscriptions are properly cleaned up in all error/edge cases, not just during normal stopScan flow

    Potential Bug
    The removal of service filtering logic could allow unwanted devices to be discovered. Verify that removing the service filter checks doesn't break expected filtering behavior

    Error Handling
    The adapter.startDiscovery() call should be wrapped in try-catch to handle potential failures gracefully

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Reset filter state during cleanup to prevent stale filter settings from persisting

    Initialize _bleFilter.scanFilter to null when stopping the scan to ensure proper
    cleanup and prevent stale filter settings from affecting future scans.

    lib/src/universal_ble_linux/universal_ble_linux.dart [87-91]

    -// Dispose listeners
    +// Dispose listeners and reset filter
     _deviceAdded?.cancel();
     _deviceRemoved?.cancel();
     _deviceAdded = null;
     _deviceRemoved = null;
    +_bleFilter.scanFilter = null;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Resetting the filter state during cleanup is crucial to prevent stale filter settings from affecting subsequent scan operations, which could lead to incorrect device filtering behavior.

    8
    Add proper error handling for asynchronous stream cancellation operations

    Add error handling for stream subscription cancellation to follow Effective Dart
    error handling guidelines.

    lib/src/universal_ble_linux/universal_ble_linux.dart [88-89]

    -_deviceAdded?.cancel();
    -_deviceRemoved?.cancel();
    +await _deviceAdded?.cancel();
    +await _deviceRemoved?.cancel();
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding 'await' for stream cancellation is important as it ensures proper cleanup of resources and prevents potential memory leaks or race conditions.

    7

    💡 Need additional feedback ? start a PR chat

    @fotiDim fotiDim merged commit ce06a7f into main Nov 20, 2024
    1 check passed
    @fotiDim fotiDim deleted the Fix-multiple-app-filtering-in-Linux branch November 20, 2024 15:22
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants