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

AttributedString content in ChatEntity #10

Merged
merged 11 commits into from
Feb 19, 2024

Conversation

philippzagar
Copy link
Member

@philippzagar philippzagar commented Feb 17, 2024

AttributedString content in ChatEntity

♻️ Current situation & Problem

At the moment, SpeziChat is not able to store and display Markdown-based content, which is typically returned from LLM providers like OpenAI.

⚙️ Release Notes

  • The content property of the ChatEntity can now be accessed as a computed property that is an AttributedString, providing the ability to render Markdown-formatted text in the MessageView.

📚 Documentation

In-line DocC comments written and adjusted

✅ Testing

--

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@philippzagar philippzagar added the enhancement New feature or request label Feb 17, 2024
@philippzagar philippzagar self-assigned this Feb 17, 2024
Copy link

codecov bot commented Feb 17, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (ea5e21b) 83.84% compared to head (dafb10a) 83.50%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #10      +/-   ##
==========================================
- Coverage   83.84%   83.50%   -0.33%     
==========================================
  Files          11       11              
  Lines         495      503       +8     
==========================================
+ Hits          415      420       +5     
- Misses         80       83       +3     
Files Coverage Δ
Sources/SpeziChat/ChatView.swift 83.61% <100.00%> (ø)
Sources/SpeziChat/MessageStyleViewModifier.swift 100.00% <ø> (ø)
Sources/SpeziChat/MessageView.swift 100.00% <100.00%> (ø)
Sources/SpeziChat/Models/ChatEntity.swift 88.00% <78.58%> (-12.00%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea5e21b...dafb10a. Read the comment docs.

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for the improvements! I only had a few minor comments that should be easily addressable before merging the PR. Thank you @philippzagar!

Sources/SpeziChat/Models/ChatEntity.swift Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Sources/SpeziChat/Models/ChatEntity.swift Show resolved Hide resolved
Copy link
Member Author

@philippzagar philippzagar left a comment

Choose a reason for hiding this comment

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

Thanks for the review @PSchmiedmayer! 🚀

I reverted back to String-based content, after I initially drafted the content as an AttributedString.
The reason for that is the following: An AttributedString can be initialized with Markdown text which is then parsed to an internal representation within the AttributedString. However, this representation doesn't enable us to actually get the Markdown text back from the AttributedString, as all Markdown-specific syntax will be gone by then. This is a major dealbreaker for SpeziLLM, in which we need to get the existing AttributedString content, get the raw String out of it, concatenate the new output piece to the raw String, and then parse it as an AttributedString Markdown text again.
Of course, one could store both the raw String (containing Markdown) as well as the parsed AttributedString. However, this is overly complex and requires to keep both of these data points in sync. Therefore, I opted for the easier route by just offering a computed property that parses the raw stored String to an AttributedString.

Sources/SpeziChat/Models/ChatEntity.swift Show resolved Hide resolved
@PSchmiedmayer
Copy link
Member

@philippzagar Nice; I like the approach. Makes the internal logic a bit cleaner 🎉

@philippzagar
Copy link
Member Author

philippzagar commented Feb 19, 2024

@PSchmiedmayer The downside is that one cannot initialize with ChatEntry with an arbitrarily styled AttributedString anymore (AttributedString offers lots of possibilities). However, as the ChatView isn't crazily capable as of now, I guess it's fine to just offer Markdown

@philippzagar
Copy link
Member Author

In addition, this PR tackles issues with scrolling to the latest rendered text piece within a message bubble as well as multiline text alingment

@philippzagar philippzagar merged commit 8233230 into main Feb 19, 2024
9 checks passed
@philippzagar philippzagar deleted the feat/attributed-string-content branch February 19, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants