Crostini: Use nice round numbers for ticks on the disk size slider

Bug: chromium:1100726
Test: Unit tests, manual

Change-Id: I58057a29ae2a1cecab14f82630d1cb90fb0fcc34
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2308437Reviewed-by: Fergus Dall <sidereal@google.com>
Commit-Queue: Fergus Dall <sidereal@google.com>
Auto-Submit: David Munro <davidmunro@google.com>
Cr-Commit-Position: refs/heads/master@{#790728}

chrome/browser/chromeos/crostini/crostini_disk.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/crostini/crostini_disk.h"
 
 #include <algorithm>
+#include <cmath>
 #include <utility>
 
 #include "base/bind.h"
@@ -37,6 +38,14 @@ void EmitResizeResultMetric(vm_tools::concierge::DiskImageStatus status) {
       static_cast<vm_tools::concierge::DiskImageStatus>(
           vm_tools::concierge::DiskImageStatus_MAX + 1));
 }
+
+int64_t round_up(int64_t n, double increment) {
+  return std::ceil(n / increment) * increment;
+}
+
+int64_t round_down(int64_t n, double increment) {
+  return std::floor(n / increment) * increment;
+}
 }  // namespace
 
 namespace crostini {
@@ -320,17 +329,40 @@ void OnResize(
 }
 
 std::vector<int64_t> GetTicksForDiskSize(int64_t min_size,
-                                         int64_t available_space) {
+                                         int64_t available_space,
+                                         int num_ticks) {
   if (min_size < 0 || available_space < 0 || min_size > available_space) {
     return {};
   }
   std::vector<int64_t> ticks;
-  int64_t range = available_space - min_size;
-  for (int n = 0; n <= 100; n++) {
-    // These are disk sizes so we're not worried about overflow, 2^60 == 1024
-    // PiB.
-    ticks.emplace_back(min_size + n * range / 100);
+
+  int64_t delta = (available_space - min_size) / num_ticks;
+  double increments[] = {1 * kGiB, 0.5 * kGiB, 0.2 * kGiB, 0.1 * kGiB};
+  double increment;
+  if (delta > increments[0]) {
+    increment = increments[0];
+  } else if (delta > increments[1]) {
+    increment = increments[1];
+  } else if (delta > increments[2]) {
+    increment = increments[2];
+  } else {
+    increment = increments[3];
+  }
+
+  int64_t start = round_up(min_size, increment);
+  int64_t end = round_down(available_space, increment);
+
+  if (end <= start) {
+    // We have less than 1 tick between min_size and available space, so the
+    // only option is to give all the space.
+    return std::vector<int64_t>{min_size};
+  }
+
+  ticks.emplace_back(start);
+  for (int n = 1; std::ceil(n * increment) < (end - start); n++) {
+    ticks.emplace_back(start + std::round(n * increment));
   }
+  ticks.emplace_back(end);
   return ticks;
 }
 }  // namespace disk

chrome/browser/chromeos/crostini/crostini_disk.h

@@ -38,6 +38,12 @@ constexpr int64_t kDiskHeadroomBytes = 1 * kGiB;
 constexpr int64_t kMinimumDiskSizeBytes = 2 * kGiB;
 constexpr int64_t kRecommendedDiskSizeBytes = 5 * kGiB;
 
+// A number which influences the interval size and number of ticks selected for
+// a given range. At 400 >400 GiB gets 1 GiB ticks, smaller sizes get smaller
+// intervals. 400 is arbitrary, chosen because it keeps ticks at least 1px each
+// on sliders and feels nice.
+constexpr int kGranularityFactor = 400;
+
 // The size of the download for the VM image.
 // As of 2020-01-10 the Termina files.zip is ~90MiB and the squashfs container
 // is ~330MiB.
@@ -116,9 +122,10 @@ void OnResize(
 // Splits the range between |min_size| and |available_space| into enough
 // evenly-spaced intervals you can use them as ticks on a slider. Will return an
 // empty set if the range is invalid (e.g. any numbers are negative).
-// The number of ticks will fit in a signed integer.
-std::vector<int64_t> GetTicksForDiskSize(int64_t min_size,
-                                         int64_t available_space);
+std::vector<int64_t> GetTicksForDiskSize(
+    int64_t min_size,
+    int64_t available_space,
+    int granularity_factor_for_testing = kGranularityFactor);
 
 }  // namespace disk
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_disk_unittest.cc

@@ -20,18 +20,6 @@
 #include "testing/gmock/include/gmock/gmock-matchers.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-namespace {
-
-// end is inclusive
-std::vector<int64_t> range(int64_t start, int64_t end, int64_t step) {
-  std::vector<int64_t> ticks;
-  for (int64_t n = start; n <= end; n += step) {
-    ticks.emplace_back(n);
-  }
-  return ticks;
-}
-}  // namespace
-
 namespace crostini {
 namespace disk {
 
@@ -182,9 +170,6 @@ TEST_F(CrostiniDiskTest, AreTicksCalculated) {
 
   ASSERT_TRUE(disk_info);
   EXPECT_EQ(disk_info->ticks.front()->value, kMinimumDiskSizeBytes);
-
-  // Available space is current + free.
-  EXPECT_EQ(disk_info->ticks.back()->value, kMinimumDiskSizeBytes + 100);
 }
 
 TEST_F(CrostiniDiskTest, DefaultIsCurrentValue) {
@@ -292,14 +277,6 @@ TEST_F(CrostiniDiskTestDbus, DiskResizeNegativeHeadroom) {
   ASSERT_EQ(disk_info->ticks.at(disk_info->ticks.size() - 1)->value, 3 * kGiB);
 }
 
-TEST_F(CrostiniDiskTest, GetTicksForDiskSizeSizeZeroRange) {
-  // When min_size == available_space the only possible option is
-  // available_space.
-  std::vector<int64_t> expected(101);
-  std::fill(expected.begin(), expected.end(), 10);
-  EXPECT_THAT(GetTicksForDiskSize(10, 10), testing::ContainerEq(expected));
-}
-
 TEST_F(CrostiniDiskTest, GetTicksForDiskSizeInvalidInputsNoTicks) {
   const std::vector<int64_t> empty = {};
   // If min_size > available_space there's no solution, so we expect an empty
@@ -312,15 +289,73 @@ TEST_F(CrostiniDiskTest, GetTicksForDiskSizeInvalidInputsNoTicks) {
   EXPECT_THAT(GetTicksForDiskSize(-100, -10), testing::ContainerEq(empty));
 }
 
-TEST_F(CrostiniDiskTest, GetTicksForDiskSizeHappyPath) {
-  auto expected = range(100, 1100, 10);
-  EXPECT_THAT(GetTicksForDiskSize(100, 1100), testing::ContainerEq(expected));
+TEST_F(CrostiniDiskTest, GetTicksForDiskSizeRoundEnds) {
+  // With 1000 GiB - epsilon of free space we should round to 1 GiB increments.
+  // Our top should be rounded down, and bottom rounded up (since they aren't on
+  // 1GiB).
+  auto ticks = GetTicksForDiskSize(1, 1000 * kGiB - 1);
+  EXPECT_EQ(ticks.front(), 1 * kGiB);
+  EXPECT_EQ(ticks.back(), 999 * kGiB);
+}
+
+TEST_F(CrostiniDiskTest, GetTicksForDiskSizeExactEnds) {
+  // With 1000 GiB of free space we should round to 1 GiB increments. Since our
+  // max and min are on 1GIB increments already, they should not be rounded.
+  auto ticks = GetTicksForDiskSize(0, 1000 * kGiB);
+  EXPECT_EQ(ticks.front(), 0 * kGiB);
+  EXPECT_EQ(ticks.back(), 1000 * kGiB);
+}
+
+TEST_F(CrostiniDiskTest, GetTicksForDiskSizeIncrements) {
+  // We target 400'ish ticks on the slider (implementation detail). With that
+  // granularity we should have increments of:
+  //  1.0 GiB for >= 400kGiB
+  //  0.5 GiB for >= 200kGiB && < 400kGiB
+  //  0.2 GiB for >= 80GiB && < 200GiB
+  //  0.1 GiB for < 80 GiB
+  auto ticks10 = GetTicksForDiskSize(0, 401 * kGiB);
+  auto ticks05 = GetTicksForDiskSize(0, 399 * kGiB);
+  auto ticks02 = GetTicksForDiskSize(0, 81 * kGiB);
+  auto ticks01 = GetTicksForDiskSize(0, 79 * kGiB);
+
+  EXPECT_FLOAT_EQ(ticks10[0] + 1.0 * kGiB, double(ticks10[1]));
+  EXPECT_FLOAT_EQ(ticks05[0] + 0.5 * kGiB, double(ticks05[1]));
+  EXPECT_FLOAT_EQ(ticks02[0] + 0.2 * kGiB, double(ticks02[1]));
+  EXPECT_FLOAT_EQ(ticks01[0] + 0.1 * kGiB, double(ticks01[1]));
+}
+
+TEST_F(CrostiniDiskTest, GetTicksForDiskSizeMinimalSpace) {
+  // Currently our minimum increment is 0.1 GiB. This means that if they have
+  // <(min + 0.1) GiB available their only option is min.
+  auto expected = std::vector<int64_t>{2 * kGiB};
+  EXPECT_THAT(GetTicksForDiskSize(2 * kGiB, 2 * kGiB + 0.09 * kGiB),
+              testing::ContainerEq(expected));
 }
 
-TEST_F(CrostiniDiskTest, GetTicksForDiskSizeNotDivisible) {
-  // Ticks are rounded down to integer values.
-  auto expected = std::vector<int64_t>{313, 316, 319, 323, 326, 329, 333};
-  EXPECT_THAT(GetTicksForDiskSize(0, 333), testing::IsSupersetOf(expected));
+TEST_F(CrostiniDiskTest, GetTicksForDiskSizeSmallRangeNonZeroStart) {
+  // 20 ticks for 1 GiB, smallest interval is 0.1GiB so we should end up with
+  // only 11 0.1 GiB ticks. For bonus coverage, start at non-zero/.
+  auto ticks = GetTicksForDiskSize(2 * kGiB, 3 * kGiB, 20);
+  std::vector<int64_t> expected = {
+      int64_t(2.0 * kGiB), int64_t(2.1 * kGiB), int64_t(2.2 * kGiB),
+      int64_t(2.3 * kGiB), int64_t(2.4 * kGiB), int64_t(2.5 * kGiB),
+      int64_t(2.6 * kGiB), int64_t(2.7 * kGiB), int64_t(2.8 * kGiB),
+      int64_t(2.9 * kGiB), int64_t(3.0 * kGiB)};
+  ASSERT_EQ(ticks.size(), expected.size());
+  for (size_t n = 0; n < expected.size(); n++) {
+    EXPECT_FLOAT_EQ(ticks[n], expected[n]);
+  }
+}
+TEST_F(CrostiniDiskTest, GetTicksForDiskSizeLargeRange) {
+  // 5 ticks for 7 GiB, largest interval is 1GiB so we should end up with 8
+  // 0.1 GiB ticks. For bonus coverage, start at non-zero non-round.
+  auto ticks = GetTicksForDiskSize(13 * kGiB - 5678, 20 * kGiB + 1234, 5);
+  std::vector<int64_t> expected = {13 * kGiB, 14 * kGiB, 15 * kGiB, 16 * kGiB,
+                                   17 * kGiB, 18 * kGiB, 19 * kGiB, 20 * kGiB};
+  ASSERT_EQ(ticks.size(), expected.size());
+  for (size_t n = 0; n < expected.size(); n++) {
+    EXPECT_FLOAT_EQ(ticks[n], expected[n]);
+  }
 }
 
 }  // namespace disk