[apps/calculation] The heights (common and expanded) of calculation cells are

computed when the calculation is added to the store and don't change afterwards.
Otherwise, if their heights change when scrolling (due to a modification of the
display output type - ExactOnly, ApproximateOnly...), it generates crashes.

apps/calculation/additional_outputs/complex_list_controller.cpp

@@ -26,10 +26,10 @@ void ComplexListController::setExpression(Poincare::Expression e) {
   }
   Poincare::Context * context = App::app()->localContext();
   // Fill Calculation Store
-  m_calculationStore.push("im(z)", context);
-  m_calculationStore.push("re(z)", context);
-  m_calculationStore.push("arg(z)", context);
-  m_calculationStore.push("abs(z)", context);
+  m_calculationStore.push("im(z)", context, CalculationHeight);
+  m_calculationStore.push("re(z)", context, CalculationHeight);
+  m_calculationStore.push("arg(z)", context, CalculationHeight);
+  m_calculationStore.push("abs(z)", context, CalculationHeight);
 
   // Set Complex illustration
   // Compute a and b as in Expression::hasDefinedComplexApproximation to ensure the same defined result

apps/calculation/additional_outputs/illustrated_list_controller.cpp

@@ -80,16 +80,7 @@ KDCoordinate IllustratedListController::rowHeight(int j) {
   }
   Shared::ExpiringPointer<Calculation> calculation = m_calculationStore.calculationAtIndex(calculationIndex);
   constexpr bool expanded = true;
-  KDCoordinate result = calculation->memoizedHeight(expanded);
-  if (result < 0) {
-    result = ScrollableThreeExpressionsCell::Height(calculation.pointer());
-    if (result < 0) {
-      // Raise, because Height modified the calculation and failed.
-      Poincare::ExceptionCheckpoint::Raise();
-    }
-    calculation->setMemoizedHeight(expanded, result);
-  }
-  return result + Metric::CellSeparatorThickness;
+  return calculation->height(expanded) + Metric::CellSeparatorThickness;
 }
 
 int IllustratedListController::typeAtLocation(int i, int j) {

apps/calculation/additional_outputs/illustrated_list_controller.h

@@ -10,8 +10,6 @@
 namespace Calculation {
 
 class IllustratedListController : public ListController, public SelectableTableViewDelegate {
-/* TODO There is factorizable code between this and
- * Calculation::HistoryController (at least rowHeight). */
 public:
   IllustratedListController(EditExpressionController * editExpressionController);
 
@@ -35,6 +33,7 @@ public:
 
   constexpr static KDCoordinate k_illustrationHeight = 120;
 protected:
+  static KDCoordinate CalculationHeight(Calculation * c, bool expanded) { return ScrollableThreeExpressionsCell::Height(c); }
   Poincare::Expression m_savedExpression;
   CalculationStore m_calculationStore;
 private:

apps/calculation/additional_outputs/scrollable_three_expressions_cell.cpp

@@ -8,8 +8,8 @@ void ScrollableThreeExpressionsView::resetMemoization() {
   setLayouts(Poincare::Layout(), Poincare::Layout(), Poincare::Layout());
 }
 
-void ScrollableThreeExpressionsView::setCalculation(Calculation * calculation, bool * didForceOutput) {
-  assert(!didForceOutput || *didForceOutput == false);
+// TODO: factorize with HistoryViewCell!
+void ScrollableThreeExpressionsView::setCalculation(Calculation * calculation, bool canChangeDisplayOutput) {
   Poincare::Context * context = App::app()->localContext();
 
   // Clean the layouts to make room in the pool
@@ -24,13 +24,10 @@ void ScrollableThreeExpressionsView::setCalculation(Calculation * calculation, b
     bool couldNotCreateExactLayout = false;
     exactOutputLayout = calculation->createExactOutputLayout(&couldNotCreateExactLayout);
     if (couldNotCreateExactLayout) {
-      if (calculation->displayOutput(context) == ::Calculation::Calculation::DisplayOutput::ExactOnly) {
-        Poincare::ExceptionCheckpoint::Raise();
-      } else {
+      if (canChangeDisplayOutput && calculation->displayOutput(context) != ::Calculation::Calculation::DisplayOutput::ExactOnly) {
         calculation->forceDisplayOutput(::Calculation::Calculation::DisplayOutput::ApproximateOnly);
-        if (didForceOutput) {
-          *didForceOutput = true;
-        }
+      } else {
+        Poincare::ExceptionCheckpoint::Raise();
       }
     }
   }
@@ -44,21 +41,18 @@ void ScrollableThreeExpressionsView::setCalculation(Calculation * calculation, b
     bool couldNotCreateApproximateLayout = false;
     approximateOutputLayout = calculation->createApproximateOutputLayout(context, &couldNotCreateApproximateLayout);
     if (couldNotCreateApproximateLayout) {
-      if (calculation->displayOutput(context) == ::Calculation::Calculation::DisplayOutput::ApproximateOnly) {
-        Poincare::ExceptionCheckpoint::Raise();
-      } else {
+      if (canChangeDisplayOutput && calculation->displayOutput(context) != ::Calculation::Calculation::DisplayOutput::ApproximateOnly) {
         /* Set the display output to ApproximateOnly, make room in the pool by
          * erasing the exact layout, and retry to create the approximate layout */
         calculation->forceDisplayOutput(::Calculation::Calculation::DisplayOutput::ApproximateOnly);
-        if (didForceOutput) {
-          *didForceOutput = true;
-        }
         exactOutputLayout = Poincare::Layout();
         couldNotCreateApproximateLayout = false;
         approximateOutputLayout = calculation->createApproximateOutputLayout(context, &couldNotCreateApproximateLayout);
         if (couldNotCreateApproximateLayout) {
           Poincare::ExceptionCheckpoint::Raise();
         }
+      } else {
+        Poincare::ExceptionCheckpoint::Raise();
       }
     }
 
@@ -74,16 +68,7 @@ void ScrollableThreeExpressionsView::setCalculation(Calculation * calculation, b
 
 KDCoordinate ScrollableThreeExpressionsCell::Height(Calculation * calculation) {
   ScrollableThreeExpressionsCell cell;
-  bool didForceOutput = false;
-  cell.setCalculation(calculation, &didForceOutput);
-  if (didForceOutput) {
-    /* We could not compute the height of the calculation as it is (the display
-     * output was forced to another value during the height computation).
-     * Warning: the display output of calculation was actually changed, so it
-     * will cause problems if we already did some computations with another
-     * display value. */
-    return -1;
-  }
+  cell.setCalculation(calculation, true);
   KDRect leftFrame = KDRectZero;
   KDRect centerFrame = KDRectZero;
   KDRect approximateSignFrame = KDRectZero;
@@ -103,8 +88,8 @@ void ScrollableThreeExpressionsCell::reinitSelection() {
   m_view.reloadScroll();
 }
 
-void ScrollableThreeExpressionsCell::setCalculation(Calculation * calculation, bool * didForceOutput) {
-  m_view.setCalculation(calculation, didForceOutput);
+void ScrollableThreeExpressionsCell::setCalculation(Calculation * calculation, bool canChangeDisplayOutput) {
+  m_view.setCalculation(calculation, canChangeDisplayOutput);
   layoutSubviews();
 }
 

apps/calculation/additional_outputs/scrollable_three_expressions_cell.h

@@ -19,7 +19,7 @@ public:
     setBackgroundColor(KDColorWhite);
   }
   void resetMemoization();
-  void setCalculation(Calculation * calculation, bool * didForceOutput = nullptr);
+  void setCalculation(Calculation * calculation, bool canChangeDisplayOutput);
   void subviewFrames(KDRect * leftFrame, KDRect * centerFrame, KDRect * approximateSignFrame, KDRect * rightFrame) {
     return m_contentCell.subviewFrames(leftFrame, centerFrame, approximateSignFrame, rightFrame);
   }
@@ -60,7 +60,7 @@ public:
 
   void setHighlighted(bool highlight) override { m_view.evenOddCell()->setHighlighted(highlight); }
   void resetMemoization() { m_view.resetMemoization(); }
-  void setCalculation(Calculation * calculation, bool * didForceOutput = nullptr);
+  void setCalculation(Calculation * calculation, bool canChangeDisplayOutput = false);
   void setDisplayCenter(bool display);
   ScrollableThreeExpressionsView::SubviewPosition selectedSubviewPosition() { return m_view.selectedSubviewPosition(); }
   void setSelectedSubviewPosition(ScrollableThreeExpressionsView::SubviewPosition subviewPosition) { m_view.setSelectedSubviewPosition(subviewPosition); }

apps/calculation/additional_outputs/trigonometry_list_controller.cpp

@@ -11,9 +11,9 @@ void TrigonometryListController::setExpression(Poincare::Expression e) {
 
   // Fill calculation store
   Poincare::Context * context = App::app()->localContext();
-  m_calculationStore.push("sin(θ)", context);
-  m_calculationStore.push("cos(θ)", context);
-  m_calculationStore.push("θ", context);
+  m_calculationStore.push("sin(θ)", context, CalculationHeight);
+  m_calculationStore.push("cos(θ)", context, CalculationHeight);
+  m_calculationStore.push("θ", context, CalculationHeight);
 
   // Set trigonometry illustration
   float angle = Shared::PoincareHelpers::ApproximateToScalar<float>(m_calculationStore.calculationAtIndex(0)->approximateOutput(context, Calculation::NumberOfSignificantDigits::Maximal), context);

apps/calculation/calculation.cpp

@@ -120,12 +120,15 @@ Layout Calculation::createApproximateOutputLayout(Context * context, bool * coul
   }
 }
 
-void Calculation::setMemoizedHeight(bool expanded, KDCoordinate height) {
-  if (expanded) {
-    m_expandedHeight = height;
-  } else {
-    m_height = height;
-  }
+KDCoordinate Calculation::height(bool expanded) {
+  KDCoordinate h = expanded ? m_expandedHeight : m_height;
+  assert(h >= 0);
+  return h;
+}
+
+void Calculation::setHeights(KDCoordinate height, KDCoordinate expandedHeight) {
+  m_height = height;
+  m_expandedHeight = expandedHeight;
 }
 
 Calculation::DisplayOutput Calculation::displayOutput(Context * context) {
@@ -184,9 +187,9 @@ Calculation::DisplayOutput Calculation::displayOutput(Context * context) {
 }
 
 void Calculation::forceDisplayOutput(DisplayOutput d) {
+  // Heights haven't been computed yet
+  assert(m_height == -1 && m_expandedHeight == -1);
   m_displayOutput = d;
-  // Reset heights memoization as it might have changed when we modify the display output
-  resetHeightMemoization();
 }
 
 bool Calculation::shouldOnlyDisplayExactOutput() {
@@ -284,9 +287,4 @@ Calculation::AdditionalInformationType Calculation::additionalInformationType(Co
   return AdditionalInformationType::None;
 }
 
-void  Calculation::resetHeightMemoization() {
-  m_height = -1;
-  m_expandedHeight = -1;
-}
-
 }

apps/calculation/calculation.h

@@ -81,9 +81,8 @@ public:
   Poincare::Layout createExactOutputLayout(bool * couldNotCreateExactLayout);
   Poincare::Layout createApproximateOutputLayout(Poincare::Context * context, bool * couldNotCreateApproximateLayout);
 
-  // Memoization of height
-  KDCoordinate memoizedHeight(bool expanded) { return expanded ? m_expandedHeight : m_height; }
-  void setMemoizedHeight(bool expanded, KDCoordinate height);
+  // Heights
+  KDCoordinate height(bool expanded);
 
   // Displayed output
   DisplayOutput displayOutput(Poincare::Context * context);
@@ -97,7 +96,9 @@ private:
   static constexpr int k_numberOfExpressions = 4;
   static constexpr KDCoordinate k_heightComputationFailureHeight = 50;
   static constexpr const char * k_maximalIntegerWithAdditionalInformation = "10000000000000000";
-  void resetHeightMemoization();
+
+  void setHeights(KDCoordinate height, KDCoordinate expandedHeight);
+
   /* Buffers holding text expressions have to be longer than the text written
    * by user (of maximum length TextField::maxBufferSize()) because when we
    * print an expression we add omitted signs (multiplications, parenthesis...) */

apps/calculation/calculation_store.cpp

@@ -50,7 +50,7 @@ ExpiringPointer<Calculation> CalculationStore::calculationAtIndex(int i) {
   return calculationAtIndex(i);
 }
 
-ExpiringPointer<Calculation> CalculationStore::push(const char * text, Context * context) {
+ExpiringPointer<Calculation> CalculationStore::push(const char * text, Context * context, CalculationHeight height) {
   /* Compute ans now, before the buffer is slided and before the calculation
    * might be deleted */
   Expression ans = ansExpression(context);
@@ -84,7 +84,7 @@ ExpiringPointer<Calculation> CalculationStore::push(const char * text, Context *
       /* If the input does not fit in the store (event if the current
        * calculation is the only calculation), just replace the calculation with
        * undef. */
-      return emptyStoreAndPushUndef(context);
+      return emptyStoreAndPushUndef(context, height);
     }
     nextSerializationLocation += strlen(nextSerializationLocation) + 1;
   }
@@ -115,7 +115,7 @@ ExpiringPointer<Calculation> CalculationStore::push(const char * text, Context *
          * undef if it fits, else replace the whole calcualtion with undef. */
         Expression undef = Undefined::Builder();
         if (!pushSerializeExpression(undef, nextSerializationLocation, &newCalculationsLocation)) {
-          return emptyStoreAndPushUndef(context);
+          return emptyStoreAndPushUndef(context, height);
         }
       }
       nextSerializationLocation += strlen(nextSerializationLocation) + 1;
@@ -132,7 +132,15 @@ ExpiringPointer<Calculation> CalculationStore::push(const char * text, Context *
   // Clean the memoization
   resetMemoizedModelsAfterCalculationIndex(-1);
 
-  return ExpiringPointer<Calculation>(reinterpret_cast<Calculation *>(m_buffer));
+  ExpiringPointer<Calculation> calculation = ExpiringPointer<Calculation>(reinterpret_cast<Calculation *>(m_buffer));
+  /* Heights are computed now to make sure that the display output is decided
+   * accordingly to the remaining size in the Poincare pool. Once it is, it
+   * can't change anymore: the calculation heights are fixed which ensures that
+   * scrolling computation is right. */
+  calculation->setHeights(
+      height(calculation.pointer(), false),
+      height(calculation.pointer(), true));
+  return calculation;
 }
 
 void CalculationStore::deleteCalculationAtIndex(int i) {
@@ -251,12 +259,12 @@ const char * CalculationStore::lastCalculationPosition(const char * calculations
   return reinterpret_cast<const char *>(c);
 }
 
-Shared::ExpiringPointer<Calculation> CalculationStore::emptyStoreAndPushUndef(Context * context) {
+Shared::ExpiringPointer<Calculation> CalculationStore::emptyStoreAndPushUndef(Context * context, CalculationHeight height) {
   /* We end up here as a result of a failed calculation push. The store
    * attributes are not necessarily clean, so we need to reset them. */
   m_slidedBuffer = false;
   deleteAll();
-  return push(Undefined::Name(), context);
+  return push(Undefined::Name(), context, height);
 }
 
 void CalculationStore::resetMemoizedModelsAfterCalculationIndex(int index) {

apps/calculation/calculation_store.h

@@ -28,7 +28,8 @@ class CalculationStore {
 public:
   CalculationStore();
   Shared::ExpiringPointer<Calculation> calculationAtIndex(int i);
-  Shared::ExpiringPointer<Calculation> push(const char * text, Poincare::Context * context);
+  typedef KDCoordinate (*CalculationHeight)(Calculation * c, bool expanded);
+  Shared::ExpiringPointer<Calculation> push(const char * text, Poincare::Context * context, CalculationHeight height);
   void deleteCalculationAtIndex(int i);
   void deleteAll();
   int numberOfCalculations() const { return m_numberOfCalculations; }
@@ -60,7 +61,7 @@ private:
   char * slideCalculationsToEndOfBuffer(); // returns the new position of the calculations
   size_t deleteLastCalculation(const char * calculationsStart = nullptr);
   const char * lastCalculationPosition(const char * calculationsStart) const;
-  Shared::ExpiringPointer<Calculation> emptyStoreAndPushUndef(Poincare::Context * context);
+  Shared::ExpiringPointer<Calculation> emptyStoreAndPushUndef(Poincare::Context * context, CalculationHeight height);
 
   char m_buffer[k_bufferSize];
   const char * m_bufferEnd;

apps/calculation/edit_expression_controller.cpp

@@ -122,7 +122,7 @@ bool EditExpressionController::inputViewDidReceiveEvent(Ion::Events::Event event
     if (!myApp->isAcceptableText(m_cacheBuffer)) {
       return true;
     }
-    m_calculationStore->push(m_cacheBuffer, myApp->localContext());
+    m_calculationStore->push(m_cacheBuffer, myApp->localContext(), HistoryViewCell::Height);
     m_historyController->reload();
     return true;
   }
@@ -145,7 +145,7 @@ bool EditExpressionController::inputViewDidFinishEditing(const char * text, Layo
   } else {
     layoutR.serializeParsedExpression(m_cacheBuffer, k_cacheBufferSize, context);
   }
-  m_calculationStore->push(m_cacheBuffer, context);
+  m_calculationStore->push(m_cacheBuffer, context, HistoryViewCell::Height);
   m_historyController->reload();
   m_contentView.expressionField()->setEditing(true, true);
   telemetryReportEvent("Input", m_cacheBuffer);

apps/calculation/history_controller.cpp

@@ -206,20 +206,7 @@ KDCoordinate HistoryController::rowHeight(int j) {
   }
   Shared::ExpiringPointer<Calculation> calculation = calculationAtIndex(j);
   bool expanded = j == selectedRow() && selectedSubviewType() == SubviewType::Output;
-  KDCoordinate result = calculation->memoizedHeight(expanded);
-  if (result < 0) {
-    result = HistoryViewCell::Height(calculation.pointer(), expanded);
-    if (result < 0) {
-      // Raise, because Height modified the calculation and failed.
-      Poincare::ExceptionCheckpoint::Raise();
-    }
-    calculation->setMemoizedHeight(expanded, result);
-  }
-  /* We might want to put an assertion here to check the memoization:
-   * assert(result == HistoryViewCell::Height(calculation.pointer(), expanded));
-   * However, Height might fail due to pool memory exhaustion, in which case the
-   * assertion fails even if "result" had the right value. */
-  return result;
+  return calculation->height(expanded);
 }
 
 int HistoryController::typeAtLocation(int i, int j) {

apps/calculation/history_view_cell.cpp

@@ -36,16 +36,7 @@ void HistoryViewCellDataSource::setSelectedSubviewType(SubviewType subviewType,
 
 KDCoordinate HistoryViewCell::Height(Calculation * calculation, bool expanded) {
   HistoryViewCell cell(nullptr);
-  bool didForceOutput = false;
-  cell.setCalculation(calculation, expanded, &didForceOutput);
-  if (didForceOutput) {
-    /* We could not compute the height of the calculation as it is (the display
-     * output was forced to another value during the height computation).
-     * Warning: the display output of calculation was actually changed, so it
-     * will cause problems if we already did some computations with another
-     * display value. */
-    return -1;
-  }
+  cell.setCalculation(calculation, expanded, true);
   KDRect ellipsisFrame = KDRectZero;
   KDRect inputFrame = KDRectZero;
   KDRect outputFrame = KDRectZero;
@@ -246,8 +237,7 @@ void HistoryViewCell::resetMemoization() {
   m_calculationCRC32 = 0;
 }
 
-void HistoryViewCell::setCalculation(Calculation * calculation, bool expanded, bool * didForceOutput) {
-  assert(!didForceOutput || *didForceOutput == false);
+void HistoryViewCell::setCalculation(Calculation * calculation, bool expanded, bool canChangeDisplayOutput) {
   uint32_t newCalculationCRC = Ion::crc32Byte((const uint8_t *)calculation, ((char *)calculation->next()) - ((char *) calculation));
   if (newCalculationCRC == m_calculationCRC32 && m_calculationExpanded == expanded) {
     return;
@@ -273,11 +263,8 @@ void HistoryViewCell::setCalculation(Calculation * calculation, bool expanded, b
     bool couldNotCreateExactLayout = false;
     exactOutputLayout = calculation->createExactOutputLayout(&couldNotCreateExactLayout);
     if (couldNotCreateExactLayout) {
-      if (calculation->displayOutput(context) != ::Calculation::Calculation::DisplayOutput::ExactOnly) {
+      if (canChangeDisplayOutput && calculation->displayOutput(context) != ::Calculation::Calculation::DisplayOutput::ExactOnly) {
         calculation->forceDisplayOutput(::Calculation::Calculation::DisplayOutput::ApproximateOnly);
-        if (didForceOutput) {
-          *didForceOutput = true;
-        }
       } else {
         /* We should only display the exact result, but we cannot create it
          * -> raise an exception. */
@@ -294,21 +281,18 @@ void HistoryViewCell::setCalculation(Calculation * calculation, bool expanded, b
     bool couldNotCreateApproximateLayout = false;
     approximateOutputLayout = calculation->createApproximateOutputLayout(context, &couldNotCreateApproximateLayout);
     if (couldNotCreateApproximateLayout) {
-      if (calculation->displayOutput(context) == ::Calculation::Calculation::DisplayOutput::ApproximateOnly) {
-        Poincare::ExceptionCheckpoint::Raise();
-      } else {
+      if (canChangeDisplayOutput && calculation->displayOutput(context) != ::Calculation::Calculation::DisplayOutput::ApproximateOnly) {
         /* Set the display output to ApproximateOnly, make room in the pool by
          * erasing the exact layout, and retry to create the approximate layout */
         calculation->forceDisplayOutput(::Calculation::Calculation::DisplayOutput::ApproximateOnly);
-        if (didForceOutput) {
-          *didForceOutput = true;
-        }
         exactOutputLayout = Poincare::Layout();
         couldNotCreateApproximateLayout = false;
         approximateOutputLayout = calculation->createApproximateOutputLayout(context, &couldNotCreateApproximateLayout);
         if (couldNotCreateApproximateLayout) {
           Poincare::ExceptionCheckpoint::Raise();
         }
+      } else {
+        Poincare::ExceptionCheckpoint::Raise();
       }
     }
   }

apps/calculation/history_view_cell.h

@@ -51,7 +51,7 @@ public:
   Poincare::Layout layout() const override;
   KDColor backgroundColor() const override { return m_even ? KDColorWhite : Palette::WallScreen; }
   void resetMemoization();
-  void setCalculation(Calculation * calculation, bool expanded, bool * didForceOutput = nullptr);
+  void setCalculation(Calculation * calculation, bool expanded, bool canChangeDisplayOutput = false);
   int numberOfSubviews() const override { return 2 + displayedEllipsis(); }
   View * subviewAtIndex(int index) override;
   void layoutSubviews(bool force = false) override;