WIP: Xcode Extension: Possible to enable / disable Rules to be apply #211
Conversation
This reverts commit 7822d82.
VinceBurn
changed the title
| if #available(OSX 10.13, *) { | ||
| tableView.usesAutomaticRowHeights = true | ||
| } else { | ||
| tableView.rowHeight = 30 |
|
|
||
| class RuleViewModel { | ||
| let name: String | ||
| var isEnable: Bool { |
There was a problem hiding this comment.
Could you change this to isEnabled - it's more grammatical.
Sources/RulesStore.swift
Outdated
|
|
||
| init(_ store: UserDefaults? = UserDefaults(suiteName: UserDefaults.groupDomain)) { | ||
| guard let store = store else { | ||
| fatalError("The UserDefaults Store is invalid") |
There was a problem hiding this comment.
Under what circumstances would this fail? It doesn't seem right to use fatalError if it's not a programmer error.
There was a problem hiding this comment.
I can see that UserDefaults.init is a failable constructor, but it's a bit misleading for the RulesStore init to take an optional store since it's not actually optional.
Maybe something like this would be better?
static var defaultSuite: UserDefaults = {
guard let defaults = UserDefaults(suiteName: UserDefaults.groupDomain) else {
fatalError("The UserDefaults Store is invalid")
}
return defaults
}()
init(_ store: UserDefaults = RulesStore.defaultSuite) { ... }
There was a problem hiding this comment.
good, I was also not happy with that
There was a problem hiding this comment.
Generally looks good to me. I'll let @tonyarnold cast an eye over it if he has time, as he knows more about Mac app development idioms.
|
FYI, the test failures are nothing to worry about - it's just because I was lazy and hard-coded the file counts. Just update the counts to fix them. |
There was a problem hiding this comment.
Overall this is good and works as advertised 👍 - thank you!
The UI could be more consistently/standardly spaced, but we can fix that once the functionality is merged.
I've made a few comments here, the rest are inline in the code:
- I'm not a fan of making
NSUserInterfaceItemIdentifierexpressible by string - AppKit lets us use properly-typed identifiers and names. I'd suggest using something to generate extensions onNSUserInterfaceItemIdentifierto vend the various identifiers, or doing so by hand. I have a project that will scan an AppKit project and generate these for you - you just need to ensure that you've set reasonable IDs within the storyboards/XIBs: https://github.com/tonyarnold/resourceror - I don't think the separate XIB is necessary for the table view cell - you can set this up within the storyboard.
- Source formatting is a bit off in some of these classes. It seems like you're using tabs and spaces - I believe the project uses spaces.
|
|
||
| required init?(coder: NSCoder) { | ||
| super.init(coder: coder) | ||
| title = "Rules" |
There was a problem hiding this comment.
This is unnecessary - set the title of the view controller from within the Storyboard.
| <!--Rules View Controller--> | ||
| <scene sceneID="v8f-un-92m"> | ||
| <objects> | ||
| <viewController storyboardIdentifier="RulesViewController_sid" id="9ZR-Qg-hHT" customClass="RulesViewController" customModule="SwiftFormat_for_Xcode" customModuleProvider="target" sceneMemberID="viewController"> |
There was a problem hiding this comment.
Make sure you set the view controller's "Presentation" to "Single" within the storyboard - we only want one of these per-app.
|
|
||
| @IBOutlet var tableView: NSTableView! { | ||
| didSet { | ||
| tableView.dataSource = self |
There was a problem hiding this comment.
These all seem like items that could be wired up or configured from within the Storyboard.
There was a problem hiding this comment.
yes, it could be done in the Storyboard, but I don't think I can do the register nib in the storyboard. And I don't like to have those config split between storyboard and code. I can move it if you insist.
|
Hi @tonyarnold
I'm not sure how you do the equivalent of the table view register nib method from the storyboard. I'm not use to AppKit. |
|
I'm happy with what's here - thank you for taking my feedback on board @VinceBurn. There are a few items I'd like to change a bit, but I reckon we merge as-is. @nicklockwood if you can hold off on releasing an update with this in it until I've had a chance to submit a follow-up PR, I have some time to look at this today/tomorrow. |
|
@VinceBurn awesome work, thanks for doing this 👍 |
Add a UI to allow user to select the rules they want to apply in Xcode
Configure a group domain for NSUserDefaults sharing between application & extension
Apply the user selected rules to both Xcode extension method
Still TODO: