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

Added exportFormat parameter to LLMChatView #48

Merged

Conversation

nriedman
Copy link
Contributor

@nriedman nriedman commented Feb 28, 2024

LLMChatView exportFormat parameter

♻️ Current situation & Problem

Currently, the LLMChatView creates an instance of the ChatView by passing .pdf as the exportFormat. The result is that there is no way to change the export format or disable exporting entirely.

⚙️ Release Notes

Instead of passing .pdf to the ChatView, LLMChatView now has an optional parameter exportFormat that is passed to the ChatView. By default, it is .pdf (so the .init signature need not change), but can now take any of .pdf, .json, .text, or .none. If exportFormat is .none, no export button will appear in the toolbar.

📚 Documentation

The LLMChatView/init now has an optional new signature where the user passes in a value for exportFormat.

struct LLMChatTestView: View {
      // Use the convenience property wrapper to instantiate the `LLMMockSession`
     @LLMSessionProvider(schema: LLMMockSchema()) var llm: LLMMockSession
     @State var muted = true

     var body: some View {
         LLMChatView(session: $llm, exportFormat: .none)
             .speak(llm.context, muted: muted)
             .speechToolbarButton(muted: $muted)
     }
 }

Alternatively, the exportFormat parameter may be omitted, in which case the LLMChatView will default to .pdf.

✅ Testing

Export functionality is rigorously tested as part of the UI testing for the Spezi ChatView.

📝 Code of Conduct & Contributing Guidelines

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

Instead of passing .pdf to the ChatView, LLMChatView now has an optional parameter exportFormat that is passed to the ChatView. By default, it is .pdf (so the .init signature need not change), but can now take any ot .pdf, .json, .text, or .none. If exportFormat is .none, no export button will appear in the toolbar.
@PSchmiedmayer
Copy link
Member

Thank you for the PR @nriedman; amazing!

Feel free to request a review from @philippzagar once you are happy with the result of the PR and would like to get a review.

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.19%. Comparing base (d6819a1) to head (e3f756d).

❗ Current head e3f756d differs from pull request most recent head c6d5e5b. Consider uploading reports for the commit c6d5e5b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   30.17%   30.19%   +0.03%     
==========================================
  Files          66       66              
  Lines        2891     2892       +1     
==========================================
+ Hits          872      873       +1     
  Misses       2019     2019              
Files Coverage Δ
Sources/SpeziLLM/Views/LLMChatView.swift 91.67% <100.00%> (+0.24%) ⬆️

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 d6819a1...c6d5e5b. Read the comment docs.

@philippzagar philippzagar added the enhancement New feature or request label Mar 5, 2024
Copy link
Member

@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 so much for the PR @nriedman, greatly appreciated! 🚀
I had some minor change requests that I persisted as in-line comments. Feel free to actually build the documentation within Xcode and check out the result via Product > Build Documentation, that helps to resolve lots of issues I mentioned in the in-line comments 👍

Sources/SpeziLLM/SpeziLLM.docc/SpeziLLM.md Outdated Show resolved Hide resolved
Sources/SpeziLLM/SpeziLLM.docc/SpeziLLM.md Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Copy link
Member

@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 @nriedman, just minor fixes left

Sources/SpeziLLM/SpeziLLM.docc/SpeziLLM.md Outdated Show resolved Hide resolved
Sources/SpeziLLM/SpeziLLM.docc/SpeziLLM.md Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Outdated Show resolved Hide resolved
@PSchmiedmayer
Copy link
Member

@nriedman How are we doing with this PR? It would be great to get it merged so we can tag a release and use this in the main application 🚀

Copy link
Member

@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.

@nriedman Only one small adjustment necessary, then you should be good to go! 🚀

Sources/SpeziLLM/SpeziLLM.docc/SpeziLLM.md Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Show resolved Hide resolved
Sources/SpeziLLM/Views/LLMChatView.swift Show resolved Hide resolved
Copy link
Member

@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 @PSchmiedmayer for the final changes!

@philippzagar philippzagar enabled auto-merge (squash) March 21, 2024 23:51
@PSchmiedmayer PSchmiedmayer merged commit dc37b91 into StanfordSpezi:main Mar 22, 2024
8 checks passed
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.

3 participants