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.

Sign up for GitHub

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

Jump to bottom

View Skipped Days In The Bar Chart #1736

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

View Skipped Days In The Bar Chart #1736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e107a1c
View Skipped Days In The Bar Chart
Mahmoud-Ibrahim-750 Jun 11, 2023
42c493e
Merge branch 'iSoron:dev' into dev
Mahmoud-Ibrahim-750 Jun 12, 2023
e551649
Update BarChartTest.kt
Mahmoud-Ibrahim-750 Jul 11, 2023
0f1fc23
Update the expected image
Mahmoud-Ibrahim-750 Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions uhabits-android/src/main/java/org/isoron/uhabits/activities/habits/show/views/BarCardView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class BarCardView(context: Context, attrs: AttributeSet) : LinearLayout(context,
val androidColor = state.theme.color(state.color).toInt()
binding.chart.view = BarChart(state.theme, JavaLocalDateFormatter(Locale.US)).apply {
series = mutableListOf(state.entries.map { it.value / 1000.0 })
skippedSeries = mutableListOf(state.skippedEntries.map { it.value / 1.0 })
colors = mutableListOf(theme.color(state.color.paletteIndex))
axis = state.entries.map { it.timestamp.toLocalDate() }
}
Expand Down
Binary file modified uhabits-core/assets/test/views/BarChart/base.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
10 changes: 10 additions & 0 deletions ...its-core/src/jvmMain/java/org/isoron/uhabits/core/ui/screens/habits/show/views/BarCard.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package org.isoron.uhabits.core.ui.screens.habits.show.views
import org.isoron.uhabits.core.models.Entry
import org.isoron.uhabits.core.models.Habit
import org.isoron.uhabits.core.models.PaletteColor
import org.isoron.uhabits.core.models.countSkippedDays
import org.isoron.uhabits.core.models.groupedSum
import org.isoron.uhabits.core.preferences.Preferences
import org.isoron.uhabits.core.ui.views.Theme
Expand All @@ -33,6 +34,7 @@ data class BarCardState(
val bucketSize: Int,
val color: PaletteColor,
val entries: List<Entry>,
val skippedEntries: List<Entry>,
val isNumerical: Boolean,
val numericalSpinnerPosition: Int,
)
Expand Down Expand Up @@ -64,9 +66,17 @@ class BarCardPresenter(
firstWeekday = firstWeekday,
isNumerical = habit.isNumerical,
)
// get all data from the oldest date till today, then count skipped days, and
// finally group days according to the truncate value (by weeks for example)
val skippedEntries = habit.computedEntries.getByInterval(oldest, today).countSkippedDays(
truncateField = ScoreCardPresenter.getTruncateField(bucketSize),
firstWeekday = firstWeekday
)

return BarCardState(
theme = theme,
entries = entries,
skippedEntries = skippedEntries,
bucketSize = bucketSize,
color = habit.color,
isNumerical = habit.isNumerical,
Expand Down
49 changes: 43 additions & 6 deletions uhabits-core/src/jvmMain/java/org/isoron/uhabits/core/ui/views/BarChart.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class BarChart(

// Data
var series = mutableListOf<List<Double>>()
var skippedSeries = mutableListOf<List<Double>>()
var colors = mutableListOf<Color>()
var axis = listOf<LocalDate>()
override var dataOffset = 0
Expand Down Expand Up @@ -83,28 +84,64 @@ class BarChart(
dataColumn < 0 || dataColumn >= series[s].size -> 0.0
else -> series[s][dataColumn]
}
if (value <= 0) return
val skippedValue = when {
dataColumn < 0 || dataColumn >= series[s].size -> 0.0
else -> skippedSeries[s][dataColumn]
}

if (value <= 0 && skippedValue <= 0.0) return // no value to be drawn
val perc = value / maxValue
val skipPerc = skippedValue / maxValue
val barHeight = round(maxBarHeight * perc)
val skipBarHeight = round(maxBarHeight * skipPerc)
val x = barOffset(c, s)
val y = height - footerHeight - barHeight
val skipY = y - skipBarHeight
canvas.setColor(colors[s])
val r = round(barWidth * 0.15)
if (2 * r < barHeight) {
// draw the main column
canvas.fillRect(x, y + r, barWidth, barHeight - r)
canvas.fillRect(x + r, y, barWidth - 2 * r, r + 1)
canvas.fillCircle(x + r, y + r, r)
canvas.fillCircle(x + barWidth - r, y + r, r)
} else {
canvas.fillRect(x, y, barWidth, barHeight)
}

// draw the skipped part
if (skippedValue > 0) {
// make the skipped day square color lighter by the half
canvas.setColor(colors[s].blendWith(theme.cardBackgroundColor, 0.5))
// if only skipped days are drawn, no separation (r) space is needed anymore
val h = if (value <= 0) skipBarHeight - r
else skipBarHeight
canvas.fillRect(x, skipY + r, barWidth, h)
}

// draw a small (rect) on top of the column
// draw this before the skipped part to draw the grid above
val rectY: Double
if (skippedValue > 0) {
rectY = skipY
canvas.setColor(colors[s].blendWith(theme.cardBackgroundColor, 0.5))
} else {
rectY = y
canvas.setColor(colors[s])
}
canvas.fillRect(x + r, rectY, barWidth - 2 * r, r + 1)
// draw two circles to make the column top rounded on both sides
canvas.fillCircle(x + r, rectY + r, r)
canvas.fillCircle(x + barWidth - r, rectY + r, r)

// draw the number above the column
canvas.setFontSize(theme.smallTextSize)
canvas.setTextAlign(TextAlign.CENTER)
canvas.setColor(colors[s])
val textY: Double = if (skippedValue > 0) skipY
else y
val textValue = value + skippedValue
canvas.drawText(
value.toShortString(),
textValue.toShortString(),
x + barWidth / 2,
y - theme.smallTextSize * 0.80
textY - theme.smallTextSize * 0.80
)
}

Expand Down
4 changes: 3 additions & 1 deletion uhabits-core/src/jvmTest/java/org/isoron/uhabits/core/ui/views/BarChartTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,13 @@ class BarChartTest {
val theme = LightTheme()
val component = BarChart(theme, fmt)
private val axis = (0..100).map { today.minus(it) }
private val series1 = listOf(200.0, 0.0, 150.0, 137.0, 0.0, 0.0, 500.0, 30.0, 100.0, 0.0, 300.0)
private val series1 = listOf(200.0, 0.0, 150.0, 137.0, 0.0, 0.0, 500.0, 30.0, 100.0, 0.0, 300.0)
private val series2 = listOf(10.0, 50.0, 50.0, 0.0, 0.0, 0.0, 0.0, 0.0, 60.0, 0.0, 110.0)

init {
component.axis = axis
component.series.add(series1)
component.skippedSeries.add(series2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come the test setup seems to change, but the assertions (the .png) do not? Are they just close enough such that the tests pass anyway, and should they be updated instead? What am I missing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After running the test multiple times with different cases, here is a final conclusion:
First, the test mechanism seems to ignore little differences between the expected image and the actual one, so yes, they are close enough such that the test pass as long as the difference value is less than 1.
Second, I think the test mechanism should be updated to take care of little difference, then according to that update, the expected image should be updated too. We may update it for now until we take care of the test in a different PR.

Here are some of the results I got from the test, feel free to try them on your own (each test case is provided with the difference value and the output image too):

  1. the current expected image (the one we may update later):
    [Imgur]

  2. the current test data (with skipped days):
    [Imgur](current test data with skipped days)
    [Imgur](output)
    [Imgur](difference)

  3. the current test data (without skipped days):
    [Imgur](current test data without skipped days)
    [Imgur](output)
    [Imgur](difference)

  4. a modified test data (accepted):
    [Imgur](accepted modified test data)
    [Imgur](output)
    [Imgur](difference)

  5. a modified test data (rejected):
    [Imgur](rejected modified test data)
    [Imgur](output)
    [Imgur](difference)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to update the expected images in the same PR, following https://github.com/iSoron/uhabits/blob/dev/docs/TEST.md#running-instrumented-tests, but you can also wait for @iSoron to chime in if you prefer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to wait. Updating the image takes no time anyway, So there's no need to rush.

Copy link
Author

@Mahmoud-Ibrahim-750 Mahmoud-Ibrahim-750 Dec 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iSoron, Would you check this PR so we can proceed and finally merge it? especially before you merge #1884?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hiqua Hey there! how are you doing? I hope you're fine. I just wanted to tell you that I will update the test image and request you to review this PR so we can finally merge it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hiqua Test image is up-to-date and this PR is ready for a final review.

component.colors.add(theme.color(8))
}

Expand Down