Using different placeholder words in mnemonic #528
Conversation
|
|
||
| // Update words | ||
| if delta > 0 { |
There was a problem hiding this comment.
Since this is all fed to SwiftUI, which should do diffing on each textview before any re-render, we will probably get the same performance if we just recalculate all the models whenever anything changes.
There was a problem hiding this comment.
Or rather, we could skip wordcount and when they press increase the reducer will simply add a placeholder model value, if they press decrease we remove one. Or is there a particular reason to let the wordcount drive things?
There was a problem hiding this comment.
I use wordCount as the canonical source of truth, which is convenient. If we add UI which allows setting the WordCount directly, everything will just work. "Add more words" is a UI thing, what should happen is the wordCount enum should get a new value.
There was a problem hiding this comment.
I would still make the word fields themselves the source of truth, and then put this delta logic in the computed setter of wordCount (rather than willSet of a stored property), with the getter being words.count. Small difference perhaps but it eliminates to possibility that they get out of sync.
There was a problem hiding this comment.
fixed! and also fixed a cornercase bug, where user had inputted a valid mnemonic (e.g 12 words) and then increase word count by 3 => Import Mnemonic button was enabled, but should have been disabled.
|
By the way, shouldn't we use the primary heading, on the left of the cells? |
|
@kugel3 i chose secondary because primary was so prominent, took over focus completely, but I can make it possible to pass in a font... |
|
Or just a boolean, "prominentHeadings: Bool = true" or something that just controls the foreground colors of both the headings, |
| @@ -53,7 +53,7 @@ extension EditPersonaField { | |||
| public var body: some SwiftUI.View { | |||
| WithViewStore(store, observe: ViewState.init(state:), send: { .view($0) }) { viewStore in | |||
| AppTextField( | |||
| primaryHeading: viewStore.primaryHeading, | |||
| primaryHeading: .init(text: viewStore.primaryHeading), | |||
There was a problem hiding this comment.
I was thinking of prominence as a separate property, so that when it's omitted it doesn't affect the primaryHeading parameter, but if you prefer this way then sure.
There was a problem hiding this comment.
I did not wanna further increase property count in AppTextField. The whole thing is a mess anyway...
| @@ -72,15 +70,17 @@ public struct ImportMnemonic: Sendable, FeatureReducer { | |||
| self.wordCount = wordCount | |||
| self.bip39Passphrase = bip39Passphrase | |||
|
|
|||
| self.isAddRowButtonEnabled = wordCount != .twentyFour | |||
| self.isRemoveRowButtonEnabled = wordCount != .twelve | |||
|
|
|||
| let isReadonlyMode = false | |||
| self.isReadonlyMode = isReadonlyMode | |||
| self.words = .init( | |||
There was a problem hiding this comment.
Couldn't this be initialised to[] and then it will be set when you set wordCount below?
| public let indexToWord: [Word.Index: Word] | ||
| public let wordToIndex: [NonEmptyString: Word.Index] |
There was a problem hiding this comment.
Isn't the first one basically the same as the words property? And the second one could be computed, using firstIndex(of:), right?
There was a problem hiding this comment.
The first one is not ordered. I want an ordered set. I tried NonEmpty<OrderedDict, but that does not work because OrderedDict does not conform to Collection.
We have the two of them for fastest possible look up. We don't care about slightly more data, we care about performance. But it was many years ago (I lifted this in from some
Old repo of mine) since ago I wrote this code so yeah, there is room for improvements.
|
The new words are perfect! |
Before
After