-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: add preference option to change MiniSim icon #82
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those changes! I've left few comments
MiniSim/Views/Preferences.swift
Outdated
@@ -11,8 +11,26 @@ import KeyboardShortcuts | |||
import LaunchAtLogin | |||
|
|||
struct Preferences: View { | |||
var menuImages = [ "menu_icon_1", "menu_icon_2", "menu_icon_3" ] | |||
@State var menuImageSelected = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use the @AppStorage
property wrapper to save directly to user defaults 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my testing, AppStorage does save directly, but the appearance of the Picker will not change without State. Both cannot be used simultaneously in a single variable (menuImageSelected) either. Two variables I'd say would likely not be advantageous to using onChange, but if you'd like, I can do that as well. How should I implement this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used AppStorage
for Picker in this file: MiniSim/Views/Onboarding/SetupView.swift
- check it out
@@ -31,6 +49,13 @@ struct Preferences: View { | |||
} | |||
} | |||
.frame(minWidth: 650, minHeight: 450) | |||
.onAppear { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you will use @AppStorage
this also won't be needed
Thanks mate, I'll get to it in a few. |
I've made the changes suggested, albeit I do have a concern regarding @AppStorage's usage. I've resolved the suggestions that have been applied. :) |
@@ -27,6 +28,11 @@ extension UserDefaults { | |||
set { set(newValue, forKey: Keys.isOnboardingFinished) } | |||
} | |||
|
|||
@objc dynamic public var menuImage: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make it return MenuImage
enum that you've added?
And also as the default let's use MenuImage.iphone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did that and it works great. Just noting though, Objective-C compliance had to be removed from menuImage, but I was able to rework the observer to that.
@@ -106,13 +112,21 @@ class MiniSim: NSObject { | |||
UserDefaults.Keys.enableiOSSimulators: true | |||
]) | |||
} | |||
|
|||
private func configureMenuImages() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's move this logic to the MenuImage
enum:
enum MenuImage: String, CaseIterable {
case iphone = "iphone"
case ipad = "ipad"
case box = "box"
var image: NSImage? {
guard let itemImage = NSImage(systemSymbolName: self.rawValue, accessibilityDescription: self.rawValue) else {
return nil
}
itemImage.size = NSSize(
width: (itemImage.size.width) * 0.78,
height: (itemImage.size.height) * 0.78)
itemImage.isTemplate = true
return itemImage
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you can call MenuImage.iphone.image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configureMenuImages is only ran once ever (setup) because the image gets smaller and smaller every single time the function is reran, NSImage remembers the previous size every time. Pardon me if I misunderstand, but this change would supposedly assume that MenuImage.image would be called multiple times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Image was getting smaller and smaller because you was mutating the button image in this case each item would have its own image
@@ -11,8 +11,24 @@ import KeyboardShortcuts | |||
import LaunchAtLogin | |||
|
|||
struct Preferences: View { | |||
@State var menuImageSelected: String = "iphone" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using @AppStorage
in MiniSim/Views/Onboarding/SetupView.swift
and it works great with Picker so I'm not sure whats the issue here. Can you check out that file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okwasniewski Sorry for the long delay, I've been a bit busy lately. SetupView works because enableiOSSimulators
and enableAndroidSimulators
are changed before the menu setup is called. With menuImageSelected
, it's a bit different, because the menu needs to change on the fly, as a @State
would. I have seen examples of @AppStorage
acting this way, but it isn't recurring for me for some reason.
For example, if I call enableAndroidSimulators
in this way:
struct Preferences: View {
...
@AppStorage(UserDefaults.Keys.enableAndroidEmulators, store: .standard) var enableAndroidEmulators: Bool = true
var body: some View {
Settings.Container(contentWidth: 400) {
...
Settings.Section(title: "Enable Android Emulators:") {
Toggle(isOn: $enableAndroidEmulators) {
Text("Enable Android Emulators")
}
...
it doesn't mutate whilst it's open, because @AppStorage
doesn't call like @State
, at least in this case.
|
||
private func configureMenuImages() { | ||
MenuImage.allCases.forEach { image in | ||
let itemImage = NSImage(imageLiteralResourceName: image.rawValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to docs this initializer of NSImage shouldn't be used directly
Adds an option in the preferences for the user to change the menu bar icon for MiniSim. Addresses #69 (nice).
MiniSim.Demonstration.1.mp4