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.

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

Forms Consume new attachments api #434

Merged
merged 37 commits into from
May 22, 2024

Conversation

kaushikrw
Copy link
Collaborator

@kaushikrw kaushikrw commented May 16, 2024

Summary of changes

  • Uses the latest daily with new api changes.
  • Uses the attachmentChanged event to handle deletion and renaming of attachments.
  • Changed the type of state holder for the AttachmentElementState.attachments from a Map to a List. This is because the Map does not preserve insertion order without an additional overhead. Using a list preserves the insertion order but increases the time complexity for deletion and rename to O(n).
  • The AttachmentElementState.Saver does to store load statuses of its FormAttachmentState. Instead, this can be inferred by looking at the FormAttachment objects when building the state list.
  • FormAttachmentState specifies a smaller size for the thumbnail to save on memory, which increased the UI responsiveness quite a bit.

@kaushikrw kaushikrw self-assigned this May 16, 2024
@kaushikrw kaushikrw marked this pull request as ready for review May 20, 2024 22:22
@kaushikrw kaushikrw requested a review from sorenoid May 20, 2024 22:23
Copy link
Collaborator

@sorenoid sorenoid left a comment

Choose a reason for hiding this comment

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

I have a question. Looks good, but I don't understand why the add operation is done differently than the delete and rename operation.

// scroll to the new attachment after a delay to allow the recomposition to complete
delay(100)
lazyListState.scrollToItem(attachments.count())
formElement.addAttachment(name, contentType, data).onSuccess { formAttachment ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is the addition to the state list done here and not in response to the added change event like rename and delete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sorenoid fixed!

@kaushikrw kaushikrw requested a review from sorenoid May 22, 2024 22:29
@kaushikrw kaushikrw merged commit 368be1a into feature-branches/forms May 22, 2024
@kaushikrw kaushikrw deleted the kaushik/forms/consume-new-api branch May 22, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants