-
Notifications
You must be signed in to change notification settings - Fork 6
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 expense and income lists to update automatically #168
Conversation
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
============================================
- Coverage 74.76% 74.75% -0.02%
- Complexity 701 702 +1
============================================
Files 124 124
Lines 2128 2139 +11
Branches 226 227 +1
============================================
+ Hits 1591 1599 +8
Misses 448 448
- Partials 89 92 +3
Continue to review full report at Codecov.
|
src/main/java/ay2021s1_cs2103_w16_3/finesse/model/ModelManager.java
Outdated
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.
LGTM!
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.
LGTM!
Fixes #128
The root cause of #128 is that a new
ObservableList
is constructed every timegetFilteredExpenseList
orgetFilteredIncomeList
is called. This makes it so that the UI must be responsible for calling these methods to update itself. However, this goes against the philosophy of anObservableList
in the first place.Instead, inside
ModelManager
we will maintain twoObservableList
s of the correct type parameter (Expense
andIncome
). Any call togetFilteredExpenseList
orgetFilteredIncomeList
will return a wrapper around the sameObservableList
every time, instead of constructing a new list.When changes are made to the filtered expense or income lists, the corresponding internal
ObservableList
s will be refreshed with the latest values. This unfortunately needs to be done 'manually' within theModelManager
, but at least it is contained within theModel
component and is not dependent on any other components.Pitest: https://ay2021s1-cs2103t-w16-3.github.io/reports/pitest/202010241722/