Improve item detail fields #1051
Conversation
There was a problem hiding this comment.
Some comments for ItemDetailState.Data struct:
isAttachmentinDatashould probably a constant? "Normal" item can't become an attachment, so that flag won't ever change.creatorsandcreatorIdscan be converted toOrderedDictionarytoo.maxFieldTitleWidth,maxNonemptyFieldTitleWidthdon't see to be used anymore, can they be deleted?- should
attributedTitlemaybe be exluded from equatability of this struct? If for example font size changes (from iOS accessibility settings), theNSAttributedStringwould differ even if title never changed? It's quite an edge case, but no need to compare attributed string when we already compare the original.
What happens, if the user starts editing, the RItem.changesSyncPaused flag is set to true and then the user crashes at some point while editing? When they open the app, that RItem's sync is still disabled, right? Maybe on app launch/activation we should reset all of these flags to false?
I also noticed a UI bug. When you edit some field and quickly tap "Done", the field change is debounced and since we tap "Done" before the debounce triggers, our UI then shows old state. It's stored in database and synced properly.
Zotero/Scenes/Detail/ItemDetail/ViewModels/ItemDetailActionHandler.swift
Show resolved
Hide resolved
Zotero/Scenes/Detail/ItemDetail/ViewModels/ItemDetailActionHandler.swift
Outdated
Show resolved
Hide resolved
Zotero/Scenes/Detail/ItemDetail/ViewModels/ItemDetailActionHandler.swift
Outdated
Show resolved
Hide resolved
Zotero/Scenes/Detail/ItemDetail/ViewModels/ItemDetailActionHandler.swift
Outdated
Show resolved
Hide resolved
| var requests: [DbRequest] = [EndItemDetailEditingDbRequest(libraryId: state.library.identifier, itemKey: state.key)] | ||
| if !updatedFields.isEmpty { | ||
| requests.insert(EditItemFieldsDbRequest(key: state.key, libraryId: state.library.identifier, fieldValues: updatedFields, dateParser: dateParser), at: 0) | ||
| let endEditingRequest = EndItemDetailEditingDbRequest(libraryId: state.library.identifier, itemKey: state.key) |
There was a problem hiding this comment.
So since we're live updating, this is a bit confusing. It made sense before, to have it here, when we had Save + Cancel, but now, not so much.
If you didn't know what this does, when we tap "Done" then fields like "Date" and "Accessed" are converted in some cases, for example if you write "Today" it'll convert to yyyy-mm-dd date format. So this conversion should happen when we're about to store these values to the database, not here.
So we don't even need to update dateModified here and this could be simplified a lot.
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
I guess if nothing changed we could throw an error so that you don't have to check whether dateModified is not nil when processing this request. It could possibly help with debugging some time, we log the error, we don't log this being nil.
| self.perform(request: request) { error in | ||
| guard let error else { return } | ||
| DDLogError("ItemDetailActionHandler: can't store title - \(error)") | ||
| let request = EditItemFieldsDbResponseRequest(key: viewModel.state.key, libraryId: viewModel.state.library.identifier, fieldValues: [keyPair: viewModel.state.data.title], dateParser: dateParser) |
There was a problem hiding this comment.
I'd probably compare base strings instead of NSAttributedStrings on line 967 above (same reason as in comment below).
| DDLogError("ItemDetailActionHandler: can't store title - \(error)") | ||
| let request = EditItemFieldsDbResponseRequest(key: viewModel.state.key, libraryId: viewModel.state.library.identifier, fieldValues: [keyPair: viewModel.state.data.title], dateParser: dateParser) | ||
| perform(request: request, invalidateRealm: false) { [weak viewModel] result in | ||
| switch result { |
There was a problem hiding this comment.
This response is repeated in every function which updates specific field, I guess you could extract and re-use it.
Doneis pressed.