fix marker scaling in frequency display #1425
Conversation
| val extraWeekdays = YearMonth.of(year, month).lengthOfMonth() - 28 | ||
|
|
||
| val freq = Array(7) { 4 } | ||
| for (day in weekday until weekday + extraWeekdays) { | ||
| freq[day % 7] = 5 | ||
| } |
There was a problem hiding this comment.
I think it might more readable if you just iterate through all the days of the month and add +1 to the corresponding entry in the array at every iteration.
There was a problem hiding this comment.
Yeah I see what you mean, I went with this approach to reduce the time complexity of the algorithm, but I can see how it may be confusing to read.
There was a problem hiding this comment.
Changed it to the method you described.
uhabits-android/src/main/java/org/isoron/uhabits/activities/common/views/FrequencyChart.kt
Show resolved
Hide resolved
|
I fixed the test, I incorrectly assumed that Sunday was 0, but looking at the Timestamp class I noticed that Saturday is mapped to 0 instead. I couldn't run the tests on my machine because of a mockito error, probably a misconfiguration on my end. But I ran the build & test workflow on my fork and the core tests seem to work fine now. Also, the view test for the frequency display probably fails. I tried to follow the |
| */ | ||
| @JvmStatic | ||
| fun getWeekdaysInMonth(startOfMonth: Timestamp): Array<Int> { | ||
| val month = startOfMonth.toCalendar()[Calendar.MONTH] + 1 |
There was a problem hiding this comment.
The Calendar class encodes the months with numbers from 0 to 11, but the YearMonth.of(year, month) method used below to get the length of the month expects an int from 1 to 12 for the month, so I just add 1 to use that method correctly.
|
Thank you for the PR, @eduebernal. I fixed the test screenshots and merged it. For the record, the commands were renamed to |
fix marker scaling in frequency display
|
Hi @eduebernal, I just noticed that this PR breaks the frequency chart for numerical habits (see screenshot attached below). If you think you could fix this in the next few days please let me know, and I will wait for your fix before we release 2.1.0. Otherwise, I will revert the commits, and we can re-apply them later.
|
|
Sure, I submitted a new PR #1489, however if you're not satisfied with that fix, I cannot promise that I will have time to work on it outside of weekends. |
Fixes #1355
The marker scaling is now based on the ratio of (number of [Weekday] with habit observed in the month/number of [Weekday]), as proposed in the issue.
This screenshot shows weekdays with 100% completion:
This screenshot shows weekdays with varying completion percentages:
I have some concerns with the scaling percentages. On weekdays that appear 4 times in a month, the possible percentages are (0,25,50,75,100%), and on weekdays that appear 5 times, the percentages are (0,20,40,60,80,100%). So, for example if you missed the habit 1 time for each weekday, the percentage for some markers would be 75% and for others would be 80%. The difference is barely noticeable but I just wanted to point it out.
Another thing is that this frequency chart may be only meaningful with habits that are done everyday or on specific weekdays.
Tested with a samsung galaxy A71.
Let me know what you think.