ubuntu-touch-coreapps-reviewers team mailing list archive

[Niklas Wenzel <nikwen.developer@xxxxxxxxx>]

Review: Needs Fixing

Thanks. This is one of the features I always missed!

I have a few minor complaints though. ;)
There is an issue related to the removed code from couldAddDot() now. Typing in '4!.2' results in '4!0.2' while in my opinion it should be '4!*0.2'. The same applies to 'pi.2'.
Also try the following: Type in '23,4', place the cursor after the three and type in a plus. Shouldn't it insert a zero?

Check out my inline comment below as well. ;)

Thanks again!

Diff comments:

> === modified file 'app/engine/formula.js'
> --- app/engine/formula.js	2015-03-26 21:16:14 +0000
> +++ app/engine/formula.js	2015-06-17 21:13:40 +0000
> @@ -90,14 +90,21 @@
>          return couldAddOperator(formula, stringToAddToFormula[0]);
>      }
>  
> -    if (stringToAddToFormula === ".") {
> +    // After decimal separator only number is allowed
> +    if (isNaN(stringToAddToFormula)) {
> +        if (formula.slice(-1) === ".") {
> +            return false;
> +        }
> +    }
> +
> +    if (stringToAddToFormula.slice(-1) === ".") {

Would you mind adding a comment here explaining that the slice() is needed when a zero is inserted when the dot is pressed? Otherwise one might revert it back to the original version later without thinking. ;)

>          return couldAddDot(formula);
>      }
>  
>      if (stringToAddToFormula === ")") {
>          return couldAddCloseBracket(formula);
>      }
> -
> + 
>      // Validate complex numbers
>      if ((stringToAddToFormula === "i") || (!isNaN(stringToAddToFormula))){
>          if (formula.slice(-1) === "i") {
> @@ -226,11 +233,6 @@
>   * @return bool: true if the dot could be added, false otherwhise
>   */
>  function couldAddDot(formulaToCheck) {
> -    // A dot could be only after a number
> -    if ((isNaN(formulaToCheck.slice(-1))) || (formulaToCheck === "")) {
> -        return false;
> -    }
> -
>      // If is after a number and it's the first dot of the calc it could be added
>      if (formulaToCheck.indexOf('.') === -1) {
>          return true;
> 
> === modified file 'app/ubuntu-calculator-app.qml'
> --- app/ubuntu-calculator-app.qml	2015-06-11 15:40:14 +0000
> +++ app/ubuntu-calculator-app.qml	2015-06-17 21:13:40 +0000
> @@ -137,6 +137,10 @@
>          if (!isNaN(visual) && isLastCalculate) {
>              longFormula = displayedInputText = shortFormula = "";
>          }
> +        // Add zero when decimal separator is not after number
> +        if ((visual === ".") && ((isNaN(longFormula.slice(textInputField.cursorPosition - 1, textInputField.cursorPosition))) || (longFormula === ""))) {
> +            visual = "0.";
> +        }
>          isLastCalculate = false;
>  
>          if (visual === "()") {
> 
> === modified file 'tests/autopilot/ubuntu_calculator_app/tests/test_main.py'
> --- tests/autopilot/ubuntu_calculator_app/tests/test_main.py	2015-04-26 22:41:49 +0000
> +++ tests/autopilot/ubuntu_calculator_app/tests/test_main.py	2015-06-17 21:13:40 +0000
> @@ -192,6 +192,16 @@
>          self.app.main_view.insert('-1=')
>          self._assert_result_is(u'0.666666666667')
>  
> +    def test_comma_without_number_validation(self):
> +        # Validation of the decimal separator
> +        # We are trying to add several commas into one number
> +        # Only first comma in the number should be allowed
> +        self.app.main_view.insert('..1.3*.*5.=')
> +        self._assert_result_is(u'0.065')
> +        self._assert_history_contains(u'0.13×0.5=0.065')
> +        self.app.main_view.insert('.7')
> +        self._assert_result_is(u'0.0657')
> +
>      def test_square(self):
>          self.app.main_view.insert('4')
>          self.app.main_view.show_scientific_keyboard()
> 


-- 
https://code.launchpad.net/~gang65/ubuntu-calculator-app/ubuntu-calculator-app-comma-without-number/+merge/262273
Your team Ubuntu Calculator Developers is subscribed to branch lp:ubuntu-calculator-app.