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

Cloud and Local Files importing #2356

Merged

Conversation

proskd
Copy link
Contributor

@proskd proskd commented Oct 21, 2024

User description

What does this PR do

  1. Fixes code signing for Lite target (technically an add on in this PR)
  2. Cloud and local files should work now.

Where should the reviewer start

There is an issue where we double call to move the ROM to our container, so the second access will fail. This PR does not address this bug.

How should this be manually tested

Any background context you want to provide

What are the relevant tickets

Screenshots (important for UI changes)

Questions


PR Type

enhancement, bug_fix


Description

  • Enhanced handling of security scoped resources across multiple files by adding conditional checks before stopping access.
  • Updated project configuration to support Lite targets, including new bundle identifiers and development team settings.
  • Modified entitlements to use variables for iCloud container and app group identifiers.

Changes walkthrough 📝

Relevant files
Enhancement
DirectoryWatcher.swift
Enhance security scoped resource handling in DirectoryWatcher

PVLibrary/Sources/DirectoryWatcher/DirectoryWatcher.swift

  • Improved handling of security scoped resources.
  • Added conditional check before stopping access to resources.
  • +5/-6     
    iCloudSync.swift
    Enhance security scoped resource handling in iCloudSync   

    PVLibrary/Sources/PVLibrary/Importer/iCloud/iCloudSync.swift

  • Improved handling of security scoped resources.
  • Added conditional check before stopping access to resources.
  • +5/-5     
    PVGameLibraryUpdatesController.swift
    Enhance security scoped resource handling in Game Library

    PVUI/Sources/PVUIBase/Game Library/PVGameLibraryUpdatesController.swift

  • Improved handling of security scoped resources.
  • Added conditional check before stopping access to resources.
  • +5/-4     
    PVAppDelegate+Open.swift
    Enhance security scoped resource handling in PVAppDelegate

    Provenance/PVAppDelegate+Open.swift

  • Improved handling of security scoped resources.
  • Added conditional check before stopping access to resources.
  • +6/-3     
    Configuration changes
    Build.xcconfig
    Add bundle identifier for Lite targets in Build config     

    Build.xcconfig

    • Added PRODUCT_BUNDLE_IDENTIFIER_LITE for Lite targets.
    +1/-0     
    project.pbxproj
    Update project configuration for Lite targets and development team

    Provenance.xcodeproj/project.pbxproj

  • Updated development team to use a variable.
  • Updated product bundle identifiers for Lite targets.
  • +15/-12 
    Provenance-AppStore.entitlements
    Use variables for iCloud and app group identifiers in entitlements

    Provenance/Provenance-AppStore.entitlements

  • Updated iCloud container and app group identifiers to use variables.
  • +4/-4     

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

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added enhancement improvements, enhancements, new features, additions bug_fix labels Oct 21, 2024
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Resource Management
    The new implementation for handling security scoped resources might not release the resource if an exception occurs before the defer block. Consider using a try-finally pattern or moving the resource release to the beginning of the defer block.

    Configuration Consistency
    Ensure that the new PRODUCT_BUNDLE_IDENTIFIER_LITE variable is consistently used across all relevant build configurations and targets. Verify that it's properly defined in the Build.xcconfig file.

    Variable Usage
    Confirm that the newly introduced variables (ICLOUD_CONTAINER_IDENTIFIER and APP_GROUP_IDENTIFIER) are correctly defined and accessible in the build environment. Ensure they are set in the appropriate configuration files.

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Improve error handling for security scoped resources

    Consider using a guard statement to handle the case where
    startAccessingSecurityScopedResource() returns false, and log an error message
    before returning from the function.

    PVLibrary/Sources/DirectoryWatcher/DirectoryWatcher.swift [508-515]

    -let secureDoc = url.startAccessingSecurityScopedResource()
    +guard url.startAccessingSecurityScopedResource() else {
    +    ELOG("Failed to access security scoped resource for file: \(url.lastPathComponent)")
    +    return
    +}
     
     defer {
    -    if secureDoc {
    -        url.stopAccessingSecurityScopedResource()
    -        ILOG("Stopped accessing security scoped resource for file: \(url.lastPathComponent)")
    -    }
    +    url.stopAccessingSecurityScopedResource()
    +    ILOG("Stopped accessing security scoped resource for file: \(url.lastPathComponent)")
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion to use a guard statement for handling the case where startAccessingSecurityScopedResource() returns false is highly relevant. It improves error handling by logging an error message and returning early, which is crucial for maintaining robustness in accessing security scoped resources.

    9
    Improve error handling for security scoped resources in iCloud sync

    Consider using a guard statement to handle the case where
    startAccessingSecurityScopedResource() returns false, and log an error message
    before continuing with the next iteration.

    PVLibrary/Sources/PVLibrary/Importer/iCloud/iCloudSync.swift [381-387]

    -let secureDoc = json.startAccessingSecurityScopedResource()
    +guard json.startAccessingSecurityScopedResource() else {
    +    ELOG("Failed to access security scoped resource for file: \(json.lastPathComponent)")
    +    continue
    +}
     
     defer {
    -    if secureDoc {
    -        json.stopAccessingSecurityScopedResource()
    -    }
    +    json.stopAccessingSecurityScopedResource()
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion enhances error handling by using a guard statement to log an error and continue to the next iteration if startAccessingSecurityScopedResource() fails. This is important for ensuring that the sync process does not proceed with inaccessible resources.

    9
    Improve error handling for security scoped resources in game library updates

    Consider using a guard statement to handle the case where
    startAccessingSecurityScopedResource() returns false, and log an error message
    before returning from the function.

    PVUI/Sources/PVUIBase/Game Library/PVGameLibraryUpdatesController.swift [318-323]

    -let secureDoc = sourceURL.startAccessingSecurityScopedResource()
    +guard sourceURL.startAccessingSecurityScopedResource() else {
    +    ELOG("Failed to access security scoped resource for file: \(sourceURL.lastPathComponent)")
    +    return
    +}
     defer {
    -    if secureDoc {
    -        sourceURL.stopAccessingSecurityScopedResource()
    -    }
    +    sourceURL.stopAccessingSecurityScopedResource()
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion to use a guard statement for handling failures in startAccessingSecurityScopedResource() is valuable. It ensures that the function logs an error and exits early, preventing further operations on inaccessible resources, which enhances the reliability of the code.

    9
    Improve error handling for security scoped resources when opening files

    Consider using a guard statement to handle the case where
    startAccessingSecurityScopedResource() returns false, and log an error message
    before continuing with the function.

    Provenance/PVAppDelegate+Open.swift [135-146]

    -var secureDocument = false
     do {
    +    guard url.startAccessingSecurityScopedResource() else {
    +        ELOG("Failed to access security scoped resource for file: \(url.lastPathComponent)")
    +        return false
    +    }
         defer {
    -        if secureDocument {
    -            url.stopAccessingSecurityScopedResource()
    -        }
    -        
    +        url.stopAccessingSecurityScopedResource()
         }
     
         // Doesn't seem we need access in dev builds?
    -    secureDocument = url.startAccessingSecurityScopedResource()
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion is beneficial as it introduces a guard statement to handle the failure of startAccessingSecurityScopedResource(). By logging an error and returning false, it improves the function's error handling and prevents further operations on inaccessible resources.

    9

    💡 Need additional feedback ? start a PR chat

    @JoeMatt JoeMatt merged commit 00e751b into Provenance-Emu:develop-spm-2024 Oct 21, 2024
    2 of 3 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    bug_fix enhancement improvements, enhancements, new features, additions Review effort [1-5]: 3
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants