david-swift / SettingsKit-macOS

Add a settings window to a SwiftUI macOS app
https://david-swift.github.io/SettingsKit-macOS/
MIT License
87 stars 2 forks source link

Use `id` instead of `index` in `.standardActions { }` #9

Closed longseespace closed 12 months ago

longseespace commented 1 year ago

Problem Statement

The current implementation of . standardActions {} uses index instead of id for the remove action. This is quite problematic when the order of the list changes, or when there is a mix of static items and dynamic items.

Let me explain. Given this code:

SettingsTab(.init("Models" as String, systemSymbol: .cpu), id: "model") {
  SettingsSubtab(.init("Static Item 1" as String, systemSymbol: .cpu), id: "model-static-1") {
    StaticItem1()
  }

  SettingsSubtab(.init("Static Item 2" as String, systemSymbol: .cpu), id: "model-static-2") {
    StaticItem2()
  }

  for item in dynamicItemList {
    SettingsSubtab(.init(item.name, systemSymbol: .cpu), id: "model-\(recordId)") {
        DynamicItem(item: item)
    }
  }
}
.standardActions {
    // 
} remove: { index in
    // handling index here can be annoying, for example when I add more Static item in the front
    // will need to increase the check for index each time to find the correct item to be removed
}

When removing an item, I always have to check if the index >= 2 (dynamic) or static. When I add new static item to the settings sub-tabs, it's prone to error.

Suggestion

Maybe support both index and id in the callback parameters? Something like this

.standardActions {
    // 
} remove: { id, index in
    // handling index here can be annoying, for example when I add more Static item in the front
    // will need to increase the check for index each time to find the correct item to be removed
}

IMO, id should be the source of truth, not index.

david-swift commented 1 year ago

I've just released SettingsKit 0.2.1 which implements exactly your suggestion.


@longseespace, note that since 0.2.0, the following code does not build anymore:

SettingsTab(.init("Models" as String, systemSymbol: .cpu), id: "model") {
  SettingsSubtab(.init("Static Item 1" as String, systemSymbol: .cpu), id: "model-static-1") {
    StaticItem1()
  }
}

Instead, use:

SettingsTab(.new(title: "Models", icon: .cpu), id: "model") {
  SettingsSubtab(.new(title: "Static Item 1", icon: .cpu), id: "model-static-1") {
    StaticItem1()
  }
}

The reason for that change is that SettingsKit now supports a sidebar layout additionally to the tab bar layout.