-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix data saving and display in stat screen #298
Conversation
…into fix/stat-screen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR fixes the stat screen bug and brings solid improvements to both the data model and UI. Great work overall, but I still have a few suggestions for improvement:
- You could introduce a helper function to generate the graphs and their corresponding text dynamically, instead of repeating similar code multiple times.
- The "Talk Time Percentage" graph can be removed since it always displays 100% (based on testing). It simplifies the screen and improves clarity
- We already have a test for
documentToUserProfile
so adding checks to ensure analysis data is correctly retrieved and mapped from Firestore would not be too hard and it will help reach 80% coverage. - Consider adding a helper function for converting Firestore data to
AnalysisData
. This would keepdocumentToUserProfile
cleaner (check out themapToSpeechBattle
function)
Otherwise, great job on addressing the serialization issues, fixing the UI and the display of the stats. Thanks, Marc!
app/src/main/java/com/github/se/orator/model/profile/UserProfileRepositoryFirestore.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/github/se/orator/ui/profile/StatScreen.kt
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
app/src/main/java/com/github/se/orator/model/profile/UserProfileRepositoryFirestore.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on achieving 97% line coverage and incorporating my suggestions! For the graph, it might be best to check with the rest of the team to get their input. I'll go ahead and approve it for now. Thanks again, Marc!
Regarding the talkTimePercentage, I will do a new PR for replacing it by a more relevant metric as I as well previously found it to be useless. |
This Pull Request refactors how recent speech analysis data is stored and displayed, going from an ArrayDeque to a List for UserStatistics.recentData to make serialization from firestore easier. Alongside this data model change, the problem in the display of the metrics extracted from the data is now fixed and the UI in StatScreen is updated to be scrollable, ensuring that graphs and statistics are more accessible and visually clear.
Data Model Improvements:
UI Enhancements:
Preview:
Related issue:
#273
#297
In this PR , the main bugs of saving and displaying recentData on the statScreen are finally fixed.