[MBL-19695][S/T/P] Fix issue of non-updating calendars filter screen when calendar options changes#3892
Conversation
…ions changes refs: MBL-19695 affects: Student, Teacher, Parent builds: Student, Teacher, Parent release note: Fixed issue of non-updating calendars filter screen when calendar options changes
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR addresses a SwiftUI view update issue by adding stable identifiers to MultiSelectionView components using a custom hashing mechanism. The approach is reasonable, but there are some concerns about the hash implementation.
Issues Found
- Hash inconsistency with OptionItem conformance - The custom
hashValueForViewIDincludes properties (title, subtitle, color, headerTitle) that aren't part of OptionItem's Equatable/Hashable implementation, which only usesid. This could lead to unexpected behavior (CalendarFilterScreen.swift:129-138) - Non-deterministic hash values -
Hasher.finalize()doesn't guarantee stable hash values across app launches, which could cause issues if view identity needs to persist (CalendarFilterScreen.swift:129-130)
Positive Feedback
- Good use of the
.id()modifier to force SwiftUI view updates when the underlying data changes - The extension is properly scoped as
privateto avoid namespace pollution - Clear documentation explaining the purpose of the workaround
- Follows Swift naming conventions and code style
Suggestions
Consider whether you need to detect changes in properties beyond id. If id changes are sufficient to trigger updates, simplify the hash to only use id. If you need to detect other property changes, consider using a more explicit approach like concatenating property values into a deterministic string or using a structure that better represents your intent.
Overall, the solution addresses the immediate problem but could benefit from refinement in the hashing logic to avoid potential edge cases.
BuildsCommit: Merge branch 'master' into bugfix/MBL-19695-NonUpdaing-Calendars-Filter-Screen (960fe03)
|
vargaat
left a comment
vargaat
left a comment
There was a problem hiding this comment.
This solution seems like a code smell for me, maybe with MultiSelectionView itself that would be nice to have fixed, to fix all such issues in the future. With the id of the selectionview being generated by all of its items will result in re-creating the whole view instead of just updating the one cell that actually changed. This might not be a problem here and fixes the issue but I feel something fundamental is broken here.
Could you check again the update logic of this?
Yes, that was like a kind of hacky solution.
That's correct. Upon digging deep into
Fixed the code to have like it that. |
| self.hasAllSelectionButton = hasAllSelectionButton | ||
|
|
||
| self._viewModel = StateObject(wrappedValue: .init( | ||
| self.viewModel = .init( |
There was a problem hiding this comment.
So this works because the Observed object is re-created on each render and that re-initializes it with the updated allOptions array, right?
As I see, the core issue with the view and the view model is that it was simply not created for changing allOptions or title properties. In the original implementation the MultiSelectionViewModel was a @StateObject that was only created once on the first view init with a set of title and allOptionsproperties. Even if these properties did change in some upper level, theMultiSelectionViewModel` was never re-created, thus the changes never reflected on the UI.
- One fix would be to keep
MultiSelectionViewModelas anObservedObjectand store it in the hosting view, make itstitleandallOptions@Published. - Another idea is to keep the
@StateObjectbut movetitleandallObjectsas view properties ofMultiSelectionView. Maybe this is simpler than the above as it requires less refactoring.
@rh12 What do you think?
There was a problem hiding this comment.
Honestly, what made my choice to use the ID synthesizing technique I used before is to avoid missing around with StateObject, as it can get us into few problems. But I forgot about that getting busy with other bugs out of urgency. So, I am favor now of getting back to my previous solution 🙃.
Problems with 2 suggestions above:
1- Using @ObservedObject will force view creation on each body calling of the container view. Even when putting title and allOptions as @Published. Which might not be efficient in a lot of cases.
Upon speaking with Attila about it, I think this point is not valid. The new understanding is we can have some way to move the viewModel of MultiSelectionView to the original viewModel of container view, then have it passed to MultiSelectionView as an ObservedObject. This way the mere updating of @Published allOptions property will do the job without the effect of view re-rendering on each view initialization.
2- The second is a good alternative, but still we need to pass allOptions to the viewModel as it is needed there. it can feel redundant to do so, but at least it is more efficient than having to turn it to @ObservedObject
I would leave the last call for @rh12 on that, of course.
petkybenedek
deleted the
bugfix/MBL-19695-NonUpdaing-Calendars-Filter-Screen
branch
petkybenedek
restored the
bugfix/MBL-19695-NonUpdaing-Calendars-Filter-Screen
branch
|
To be revisited later. |
4 participants
refs: MBL-19695
affects: Student, Teacher, Parent
builds: Student, Teacher, Parent
release note: Fixed issue of non-updating calendars filter screen when calendar options changes
Fixes an issue where the calendar filter screen's selection lists weren't reflecting updates to the filter options. Added .id() modifiers to force view recreation when the underlying calendar options change, ensuring the UI stays in sync with the data.
Changes:
Test Plan
Prerequisites
Navigate to Calendar → Filter screen
Test Cases
1. Course list updates
2. Group list updates
3. Selection state persists
4. Metadata updates
Checklist