microsoft / calculator

Windows Calculator: A simple yet powerful calculator that ships with Windows
MIT License
29.53k stars 5.35k forks source link

Repeated enter key presses only applies previous binary operator (ignores unary operators) #264

Open baktrius opened 5 years ago

baktrius commented 5 years ago

Describe the bug Mysterious "+ 1" appears in the history panel after reproducing following steps which leads to an incorrect result.

Click on buttons in a following order to reproduce a. 1 b. + c. 1 d. n! e. = (at this point the calculator shows correct result which is 2) f. √ g. x2 (at this point the calculator still shows correct result which is 2) h. = (at this point the calculator shows incorrect result 3 insted of 2 and "+ 1" appears at the end of the expression in the history panel)

Screenshots obraz

Device and Application Information (please complete the following information):

Additional context Ones can be replaced with any other natural numbers and issue will still occur. In that case mysterious "+ 1" after expression will be replaced with "+ whatever the result of previously counted factorial was".

MicrosoftIssueBot commented 5 years ago

This is your friendly Microsoft Issue Bot. I've seen this issue come in and have gone to tell a human about it.

grochocki commented 5 years ago

Thanks for the report! I appreciate the thoroughness in your filing. Definitely something odd going on here. I would expect that hitting the equals button without any modification to repeat the last action, but not after transforming with other operators:

Szigerd commented 5 years ago

This "bug" has imho nothing to do with factorial, as you can reproduce the exact same behavior when omitting step d. in the first post (using ';' as separator it is the sequence 1; +; 1; =; √; x^2; =).

Imho the crucial thing is that √ and x^2 are inverse operations; i.e (√(x)^2) = x for all x. So these operations combined do nothing, they are the trivial function id(x) = x. Or "no operation" as I think a programmer would say. (Mathematician here)

And when hitting the "="-sign, the calculator repeats the last NONtrivial operation (adding 1 in the example in the first post).

Tested this hypothesis replacing the functions √ and ^2 with the inverse pairs of functions

e^x, ln 10^x, log sin^-1, sin cos^-1, cos tan^-1, tan 1/x, 1/x (1/x is self-inverse)

and all showed the same behavior.

Example: 1; ; 3; =, ln, e^x; = yields e^(ln(3)) 3 and any subsequent '=' yields previous result *3, the last nontrivial operation.

another example: 2; ; 5; =; 1/x; 1/x; = yields (1/(1/10) 5 and any subsequent '=' yields previous result *5.

I am not sure if I should consider this behavior as a bug or a feature, as I don't like the '='-sign to repeat trivial operations. AND you see the result (which is always the trivial one) before you hit the '='-sign.

To wrap up: For me, it's not a bug, it's a feature!

grochocki commented 5 years ago

And when hitting the "="-sign, the calculator repeats the last NONtrivial operation (adding 1 in the example in the first post).

I think we need to more crisply define non-trivial operation. For example, I can still reproduce the reported issue without applying inverse pairs:

In that last step, even though I do not apply inverse (sqrt), you might expect that the square operation to apply again. On the other hand, "+1" was the last operation performed before hitting the '=' previously, so there could be an argument there too.

Retriaging to investigate to give time for community weight in on expectations here.

I-Campbell commented 5 years ago

The expectation is: If you use an operation (binary or unary), hitting equals stores that result to the history. If you hit equals again, it will repeat the last operation.

I think the problem is in calculator/src/CalcManager/CEngine/scicomm.cpp there are two problems:

  1. binary operators correctly set a flag m_bNoPrevEqu on line 290 https://github.com/Microsoft/calculator/blob/e40791d7ad94ff17dc1f9e672e0fd32097ecbdc4/src/CalcManager/CEngine/scicomm.cpp#L290, but unary operators do not set this flag.

  2. if 1. was fixed and unary operators did set that flag, hitting a second equals will repeat the last BINARY operator. https://github.com/Microsoft/calculator/blob/e40791d7ad94ff17dc1f9e672e0fd32097ecbdc4/src/CalcManager/CEngine/scicomm.cpp#L481

[Background] Binary operators are ones with two operands eg + is a binary operator as it has two operands. Unary only have one operand, eg sqr() only has one operand.

grochocki commented 5 years ago

if 1. was fixed and unary operators did set that flag, hitting a second equals will repeat the last BINARY operator.

@I-Campbell Would you expect that all binary and unary operators are repeated when pressing equals?

I am not sure why, but it feels right that x^2 would be repeated, but does not feel as right if log, n! or sqrt did.

rudyhuyn commented 5 years ago

Working on it!

I-Campbell commented 5 years ago

Sorry for late reply to https://github.com/microsoft/calculator/issues/264#issuecomment-476069388 I do think all binary unary operators should be repeated. It makes the double equals make more sense: "Will repeat last operator" Rather than "Will repeat last operator unless you used one of the following..."

I am now looking forward to taking the log(log(10 000 000 000)) when this bug is fixed.

grochocki commented 5 years ago

Based on the conversation here and after looking at what other calculators do for unary operators, I am marking this as approved and unlocking #513 so that hitting "=" will always repeat the last operation.