Skip to content
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.

Sign up for GitHub

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

Improve item detail fields #1051

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve item detail fields #1051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbffc40
Fix item detail editing for attachment type
mvasilak Jan 8, 2025
2d1430f
Update Item date modified with each saved change
mvasilak Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 41 additions & 8 deletions Zotero/Controllers/Database/Requests/EditItemFieldsDbRequest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,18 @@ import Foundation

import RealmSwift

struct EditItemFieldsDbRequest: DbRequest {
let key: String
let libraryId: LibraryIdentifier
let fieldValues: [KeyBaseKeyPair: String]
let dateParser: DateParser
protocol EditItemFieldsBaseRequest {
var key: String { get }
var libraryId: LibraryIdentifier { get }
var fieldValues: [KeyBaseKeyPair: String] { get }
var dateParser: DateParser { get }

var needsWrite: Bool { return true }
func processAndReturnResponse(in database: Realm) throws -> Date?
}

func process(in database: Realm) throws {
guard !fieldValues.isEmpty, let item = database.objects(RItem.self).uniqueObject(key: key, libraryId: libraryId) else { return }
extension EditItemFieldsBaseRequest {
func processAndReturnResponse(in database: Realm) throws -> Date? {
guard !fieldValues.isEmpty, let item = database.objects(RItem.self).uniqueObject(key: key, libraryId: libraryId) else { return nil }

var didChange = false

Expand Down Expand Up @@ -60,6 +62,37 @@ struct EditItemFieldsDbRequest: DbRequest {
item.changes.append(RObjectChange.create(changes: RItemChanges.fields))
item.changeType = .user
item.dateModified = Date()
return item.dateModified
}

return nil
Copy link
Contributor

@michalrentka michalrentka Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

struct EditItemFieldsDbRequest: EditItemFieldsBaseRequest, DbRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anywhere? Why separate into 2 same requests where the distinction is that one returns and the other one doesn't? Can't we just ignore the response when it's not needed?

Copy link
Contributor Author

@mvasilak mvasilak Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in PDFReaderActionHandler where it is used in perform(writeRequests: requests), that's why I differentiated them like that. For more streamlining, we can consider changingDbRequest to something like:

protocol DbRequest: DbResponseRequest where Response == Void

let key: String
let libraryId: LibraryIdentifier
let fieldValues: [KeyBaseKeyPair: String]
let dateParser: DateParser

var needsWrite: Bool { return true }

func process(in database: Realm) throws {
_ = try processAndReturnResponse(in: database)
}
}

struct EditItemFieldsDbResponseRequest: EditItemFieldsBaseRequest, DbResponseRequest {
typealias Response = Date?

let key: String
let libraryId: LibraryIdentifier
let fieldValues: [KeyBaseKeyPair: String]
let dateParser: DateParser

var needsWrite: Bool { return true }

func process(in database: Realm) throws -> Date? {
return try processAndReturnResponse(in: database)
}
}
1 change: 0 additions & 1 deletion Zotero/Controllers/Database/Requests/EndItemDetailEditingDbRequest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ struct EndItemDetailEditingDbRequest: DbRequest {

func process(in database: Realm) throws {
guard let item = database.objects(RItem.self).uniqueObject(key: itemKey, libraryId: libraryId) else { return }
item.dateModified = Date()
item.changesSyncPaused = false
item.changeType = .user
}
Expand Down
79 changes: 31 additions & 48 deletions Zotero/Scenes/Detail/ItemDetail/ItemDetailDataCreator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
//

import Foundation
import OrderedCollections

import CocoaLumberjackSwift

Expand Down Expand Up @@ -67,7 +68,7 @@ struct ItemDetailDataCreator {
throw ItemDetailError.cantCreateData
}

let (fieldIds, fields, hasAbstract) = try fieldData(for: itemType, schemaController: schemaController, dateParser: dateParser, urlDetector: urlDetector, doiDetector: doiDetector)
let (fields, hasAbstract) = try fieldData(for: itemType, schemaController: schemaController, dateParser: dateParser, urlDetector: urlDetector, doiDetector: doiDetector)
let date = Date()
let attachments: [Attachment] = child.flatMap({ [$0] }) ?? []
let data = ItemDetailState.Data(
Expand All @@ -79,7 +80,6 @@ struct ItemDetailDataCreator {
creators: [:],
creatorIds: [],
fields: fields,
fieldIds: fieldIds,
abstract: (hasAbstract ? "" : nil),
dateModified: date,
dateAdded: date
Expand Down Expand Up @@ -125,10 +125,9 @@ struct ItemDetailDataCreator {
}
}

let (fieldIds, fields, _) = try fieldData(for: item.rawType, schemaController: schemaController, dateParser: dateParser,
urlDetector: urlDetector, doiDetector: doiDetector, getExistingData: { key, _ in
let (fields, _) = try fieldData(for: item.rawType, schemaController: schemaController, dateParser: dateParser, urlDetector: urlDetector, doiDetector: doiDetector) { key, _ in
return (nil, values[key])
})
}

var creatorIds: [String] = []
var creators: [String: ItemDetailState.Creator] = [:]
Expand Down Expand Up @@ -185,7 +184,6 @@ struct ItemDetailDataCreator {
creators: creators,
creatorIds: creatorIds,
fields: fields,
fieldIds: fieldIds,
abstract: abstract,
dateModified: item.dateModified,
dateAdded: item.dateAdded
Expand All @@ -209,27 +207,27 @@ struct ItemDetailDataCreator {
urlDetector: UrlDetector,
doiDetector: (String) -> Bool,
getExistingData: ((String, String?) -> (String?, String?))? = nil
) throws -> ([String], [String: ItemDetailState.Field], Bool) {
) throws -> (OrderedDictionary<String, ItemDetailState.Field>, Bool) {
guard var fieldSchemas = schemaController.fields(for: itemType) else {
throw ItemDetailError.typeNotSupported(itemType)
}

var fieldKeys = fieldSchemas.map({ $0.field })
let abstractIndex = fieldKeys.firstIndex(of: FieldKeys.Item.abstract)
var hasAbstract: Bool = false
let titleKey = schemaController.titleKey(for: itemType)
var fields: OrderedDictionary<String, ItemDetailState.Field> = [:]
for schema in fieldSchemas {
let key = schema.field
// Remove title and abstract keys, those 2 are used separately in Data struct.
if key == FieldKeys.Item.abstract {
hasAbstract = true
continue
}

// Remove title and abstract keys, those 2 are used separately in Data struct
if let index = abstractIndex {
fieldKeys.remove(at: index)
fieldSchemas.remove(at: index)
}
if let key = schemaController.titleKey(for: itemType), let index = fieldKeys.firstIndex(of: key) {
fieldKeys.remove(at: index)
fieldSchemas.remove(at: index)
}
if key == titleKey {
continue
}

var fields: [String: ItemDetailState.Field] = [:]
for (offset, key) in fieldKeys.enumerated() {
let baseField = fieldSchemas[offset].baseField
let baseField = schema.baseField
let (existingName, existingValue) = (getExistingData?(key, baseField) ?? (nil, nil))

let name = existingName ?? schemaController.localized(field: key) ?? ""
Expand All @@ -241,45 +239,30 @@ struct ItemDetailDataCreator {
additionalInfo = [.dateOrder: order]
}
if key == FieldKeys.Item.accessDate, let date = Formatter.iso8601.date(from: value) {
additionalInfo = [.formattedDate: Formatter.dateAndTime.string(from: date),
.formattedEditDate: Formatter.sqlFormat.string(from: date)]
additionalInfo = [.formattedDate: Formatter.dateAndTime.string(from: date), .formattedEditDate: Formatter.sqlFormat.string(from: date)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't add comment for unchanged lines, but I'd convert these ifs to switch:

switch (key, baseKey) {
case (FieldKeys.Item.date, _), (_, FieldKeys.Item.date):
    additionalInfo = [.dateOrder: Formatter.iso8601.date(from: value)]

case (FieldKeys.Item.accessDate, _):
    additionalInfo = [.formattedDate: Formatter.iso8601.date(from: value).flatMap({ Formatter.dateAndTime.string(from: $0) }), .formattedEditDate: Formatter.sqlFormat.string(from: date)]
}

}

fields[key] = ItemDetailState.Field(key: key,
baseField: baseField,
name: name,
value: value,
isTitle: false,
isTappable: isTappable,
additionalInfo: additionalInfo)
fields[key] = ItemDetailState.Field(key: key, baseField: baseField, name: name, value: value, isTitle: false, isTappable: isTappable, additionalInfo: additionalInfo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add isEditable to Field struct so that we don't have to add special cases throughout all kinds of places, we can control exceptions from here.

For example I believe this could be changed:

zotero-ios/Zotero/Scenes/Detail/ItemDetail/Views/ItemDetailCollectionViewHandler.swift

Lines 222 to 228 in 757aa35

if !isEditing || viewModel.state.data.isAttachment {
return collectionView.dequeueConfiguredReusableCell(using: fieldRegistration, for: indexPath, item: (.field(field), titleWidth))
}
if multiline {
return collectionView.dequeueConfiguredReusableCell(using: fieldMultilineEditRegistration, for: indexPath, item: (field, titleWidth))
}
return collectionView.dequeueConfiguredReusableCell(using: fieldEditRegistration, for: indexPath, item: (field, titleWidth))
to

if !isEditing || !field.isEditable {
    return collectionView.dequeueConfiguredReusableCell(using: fieldRegistration, for: indexPath, item: (.field(field), titleWidth))
}
if multiline {
    return collectionView.dequeueConfiguredReusableCell(using: fieldMultilineEditRegistration, for: indexPath, item: (field, titleWidth))
}
return collectionView.dequeueConfiguredReusableCell(using: fieldEditRegistration, for: indexPath, item: (field, titleWidth))

and this

zotero-ios/Zotero/Scenes/Detail/ItemDetail/Views/ItemDetailCollectionViewHandler.swift

Line 914 in 757aa35

guard !viewModel.state.isEditing || (viewModel.state.data.type == ItemTypes.attachment), let field = viewModel.state.data.fields[fieldId], field.isTappable else { return }

to guard let field = viewModel.state.data.fields[fieldId], field.isTappable && (!viewModel.state.isEditing || !field.isEditable) else { return }

So that they are not only tied to isAttachment, but whatever comes up later.

}

return (fieldKeys, fields, (abstractIndex != nil))
return (fields, hasAbstract)
}

/// Returns all field keys for given item type, except those that should not appear as fields in item detail.
static func allFieldKeys(for itemType: String, schemaController: SchemaController) -> [String] {
static func allFieldKeys(for itemType: String, schemaController: SchemaController) -> OrderedSet<String> {
guard let fieldSchemas = schemaController.fields(for: itemType) else { return [] }
var fieldKeys = fieldSchemas.map({ $0.field })
// Remove title and abstract keys, those 2 are used separately in Data struct
if let index = fieldKeys.firstIndex(of: FieldKeys.Item.abstract) {
fieldKeys.remove(at: index)
}
if let key = schemaController.titleKey(for: itemType), let index = fieldKeys.firstIndex(of: key) {
fieldKeys.remove(at: index)
var fieldKeys: OrderedSet<String> = OrderedSet(fieldSchemas.map({ $0.field }))
// Remove title and abstract keys, those 2 are used separately in Data struct.
fieldKeys.remove(FieldKeys.Item.abstract)
if let titleKey = schemaController.titleKey(for: itemType) {
fieldKeys.remove(titleKey)
}
return fieldKeys
}

/// Returns filtered, sorted array of keys for fields that have non-empty values.
static func filteredFieldKeys(from fieldKeys: [String], fields: [String: ItemDetailState.Field]) -> [String] {
var newFieldKeys: [String] = []
fieldKeys.forEach { key in
if !(fields[key]?.value ?? "").isEmpty {
newFieldKeys.append(key)
}
}
return newFieldKeys
/// Returns filtered, ordered set of keys for fields that have non-empty values.
static func filteredFieldKeys(from fields: OrderedDictionary<String, ItemDetailState.Field>) -> OrderedSet<String> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth renaming to something better like nonEmptyFieldKeys or similar.

return fields.filter({ !$0.value.value.isEmpty }).keys
}

/// Checks whether field is tappable based on its key and value.
Expand Down
7 changes: 4 additions & 3 deletions Zotero/Scenes/Detail/ItemDetail/Models/ItemDetailState.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
//

import UIKit
import OrderedCollections

import CocoaLumberjackSwift
import RealmSwift
Expand Down Expand Up @@ -177,8 +178,7 @@ struct ItemDetailState: ViewModelState {
var localizedType: String
var creators: [String: Creator]
var creatorIds: [String]
var fields: [String: Field]
var fieldIds: [String]
var fields: OrderedDictionary<String, Field>
var abstract: String?

var dateModified: Date
Expand Down Expand Up @@ -222,7 +222,6 @@ struct ItemDetailState: ViewModelState {
creators: [:],
creatorIds: [],
fields: [:],
fieldIds: [],
abstract: nil,
dateModified: date,
dateAdded: date,
Expand All @@ -248,6 +247,7 @@ struct ItemDetailState: ViewModelState {
var data: Data
var snapshot: Data?
var promptSnapshot: Data?
var presentedFieldIds: OrderedSet<String>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably change to visibleFieldIds, presented on iOS feels like some view controller is going to be presented (presentViewController()).

var notes: [Note]
var attachments: [Attachment]
var tags: [Tag]
Expand Down Expand Up @@ -291,6 +291,7 @@ struct ItemDetailState: ViewModelState {
self.userId = userId
self.changes = []
self.data = .empty
self.presentedFieldIds = []
self.attachments = []
self.notes = []
self.tags = []
Expand Down
Loading