GNOME Bugzilla – Bug 694863
Repeating previous calculation doesn't show the right calculation
Last modified: 2014-06-15 22:12:45 UTC
gnome-calculator 3.7.90 on Ubuntu 13.04 1. Enter 2 2. Enter "x 2" 3. Press "=" or hit Enter repeatedly to basically repeat step 2 What happens Each time Enter is pressed, the calculator alternates between showing "2 x 2" or the result of multiplying the previous answer by 2. The problem is that it's no longer "2 x 2" but actually "4 x 2" then "8 x 2", etc. What should happen Each time Enter is pressed, the calculator alternates between showing "(value of the previous answer) x 2" or the result of multiplying the previous answer by 2.
Jeremy, thanks for the bug report. The Calculator is meant to switch between the input equation and answer when you press "=" or enter multiple times. I guess, what you are expecting is a classic calculator behaviour, which is due to 2 register limitation in calculators. This program doesn't emulate a classic calculator, hence this behaviour is normal IMO.
No, gnome-calculator 3.7 does not work right. Type This What Shows in the Display 2 2 Enter 2 x 2 2x2 Enter 4 Enter 2x2 Enter 8 Enter 2x2 Enter 16 ... Proposal 1 (this is what Windows 8 and OS X does) Type This What Shows in the Display 2 2 Enter 2 x 2 2x2 Enter 4 Enter 8 Enter 16 ... Proposal 2 Type This What Shows in the Display 2 2 Enter 2 x 2 2x2 Enter 4 Enter (ans)x2 or something like that (My memory is a bit fuzzy but I think the Windows XP PowerToy calculator did this) Enter 8 Enter (ans)x2 Enter 16 ...
Jeremy, I guess you are missing the point here. gnome-calculator is not supposed to do recursive calculations. The idea behind it's behaviour is, user should be able to edit the input equation easily after calculating the result. The easiest way to do is by pressing enter key again to enter edit mode. Therefore it is intentionally designed to switch between input and answer. Physical calculators were able to perform such task because there were only 2 registers for storing the numbers, which caused it to overwrite one of digit with answer, and when you press calculate again, it would simply perform the operation with overwritten value (previous answer) again and again. e.g. when you enter 2x2 in a calculator, the registers A and B would both contain 2. Now when you press equal, the operation which would be performed is A = A * B. Hence every time you press calculate, it will update the value as shown below. Key pressed A-register(display) B-register 2 x 2 2 2 = 4 2 (A = A * B) = 8 2 (A = A * B) ... for someone who would like to perform similar operation in gnome-calculator, the simplest way is to enter it in following sequence.. Key sequence Display 2 2 <Enter> 2 (In bold letters, which means it's the answer) * 2 2 x 2 (first 2 in bold) <Enter> 4 <Enter><Enter> 8 <Enter><Enter> 16 <Enter><Enter> 32 ... I hope, this will help you..
Hello I have to agree with Jeremy the following workflow does not seam right at all: Key sequence Display 2 2 <Enter> 2 (In bold letters, which means it's the answer) * 2 2 x 2 (first 2 in bold) <Enter> 4 <Enter> 2 x 2 (first 2 in bold) <Enter> 8 <Enter> 2 x 2 (first 2 in bold) <Enter> 16 <Enter> 2 x 2 (first 2 in bold) <Enter> 32 when you hit enter to switch to the 'edit' mode: - ether 'ans' should be restore to the previous value - or the equation should be updated with the udpated 'ans' value or the 'ans' string the actual implementation is just doing 'undo' if the current display is equal to 'ans' this as nothing to do with switching to 'edit' mode see the followings workflow with the current implem. Key sequence Display ############ Workflow 1 ################ 2 2 <Enter> 2 (In bold letters, which means it's the answer) * 2 2 x 2 (first 2 in bold) <Backspace>x2 2 (2 in bold) <enter> 2 x (2 in bold) (undo is called) <enter> Malformed expression ############ Workflow 2 ################ ans ans <Enter> an
Sorry, PioneerAxon, I also agree with Jeremy. gnome-calculator is doing recursive calculations *now* even though it's not supposed to. This is a regression from 3.6 where pressing Enter multiple times would toggle between the answer and its expression.
Elliott Sales de Andrade, It's not a regression from 3.6. The code is the same from the day it was implemented. It still toggles between the answer and it's expression. What happens in back-end is, when you evaluate any expression, the variable "ans" is assigned the evaluated value. Now, bold letters in any expression is internally evaluated as "ans" variable. So no matter what the bold letters represent, it's internally "ans". Therefore when you calculate anything, the value of "ans" is updated. So, when you keep on pressing enter, it's basically evaluated as ans = 2 x 2 (4) ans = ans x 2 (8) ans = ans x 2 (16) ... However, there is a strong need to fix this issue. The only way to do so is by either update the value (in bold letters) on the display according to the value in "ans" variable or replace the value in "ans" variable with the displayed value, whenever the display is restored to any previous state. My best guess on this is, we go with updating the value in "ans" variable with the one on display.. Still it's open for discussion..
(In reply to comment #6) > It's not a regression from 3.6. The code is the same from the day it was > implemented. > The fact remains, in 3.6, pressing Enter would toggle between "2x2" and "4". Now it doesn't; that's a regression and I don't know how you can say it isn't. It certainly wasn't a design choice.
(In reply to comment #6) > It still toggles between the answer and it's expression. What It does is 'undo' when there is only 'ans' on the screen. In basic use case, undo just update the input with the previous one. (so you think it toggles between the answer and it's expression.) I copy-paste 2 use cases from my previous comment. Key sequence Display ############ Workflow 1 ################ 2 2 <Enter> 2 (In bold letters, which means it's the answer) * 2 2 x 2 (first 2 in bold) <Backspace>x2 2 (2 in bold) <enter> 2 x (2 in bold) (undo is called) <enter> Malformed expression ############ Workflow 2 ################ ans ans <Enter> an
*** Bug 701951 has been marked as a duplicate of this bug. ***
*** Bug 705666 has been marked as a duplicate of this bug. ***
The problem is due to the "ans" variable in undo stack referring to same Number instance as current ans. So, whenever a value is changed in global "ans" variable, the same value is represented by the undo stack's "ans" variable. I've tried to implement different solutions (copy constructor, new constructor from_number (), etc.), but none of them have any effect on the results. The generated C code seems to unref the older value and then refer the newer value, but the "ans" in stack always gets modified along with the global "ans" when anything is calculated. I am not sure whether it's a bug in Vala / gobject or some unidentified logical mistake.
(In reply to comment #3) > Key sequence Display > 2 2 > <Enter> 2 (In bold letters, which means it's the answer) > * 2 2 x 2 (first 2 in bold) > <Enter> 4 > <Enter><Enter> 8 > <Enter><Enter> 16 > <Enter><Enter> 32 > ... > > I hope, this will help you.. Hi, I understand the explanation and your reasons why you think this is a correct behavior. BUT: As you write > <Enter><Enter> 8 > <Enter><Enter> 16 > <Enter><Enter> 32 in fact what the user sees is this: Input: Display: <Enter> 2 * 2 <Enter> 8 <Enter> 2 * 2 <Enter> 16 <Enter> 2 * 2 <Enter> 32 I'm sorry but I just don't want a calculator which tells me 2 * 2 is anything else than 4 ! I understand there are people wanting this behavior with <Enter><Enter>. But I think in this case the calculator must UPDATE the answer variable to show this: <Enter> 4 * 2 <Enter> 8 <Enter> 8 * 2 <Enter> 16 <Enter> 16 * 2 <Enter> 32 so the user will be aware of this behavior. Another thing is with the Ctrl-Z. The users expect to REPAIR their previous input when pressing Ctrl-Z. Not to use the new answer. Imagine you would like to compute 5 * 8 and then you want 99 % of that result. So you type: Input: Display: 5 * 8 <Enter> 40 * 0.98 <Enter> 39.2 (that was a user's mistake, so the user presses Ctrl-Z) <Ctrl-Z> 40 * 0.98 (the users repairs a value 0.98 to 0.99) <Backspace> 9 40 * 0.99 <Enter> 38.808 The correct answer is 39.6. Nobody expects 38.808 !
I have gnome-calculator 3.8.2 and there is still the same issue exactly as vochomurka says.
*** Bug 706848 has been marked as a duplicate of this bug. ***
*** Bug 722122 has been marked as a duplicate of this bug. ***
*** Bug 720452 has been marked as a duplicate of this bug. ***
gnome-calculator 3.8.2 has still the same issue. I would definitely prefer the behaviour of the version in Ubuntu 12.04 LTS which feels like updating the ans variable with the value shown in the display.
*** Bug 723363 has been marked as a duplicate of this bug. ***
*** Bug 724849 has been marked as a duplicate of this bug. ***
Created attachment 271507 [details] [review] fixes the bug " repeating previous calculation doesn't show the right calculation" in a primitive way. This patch would display the following results: 2 = 2 2*2= 4 = ans*2 = 8 = ans*2 = 16 ..... Hoping that this patch could be of some help in fixing this issue. I am working on a new patch which would replace ans by the real ans but as this bug is quite complicated , it will take a while to figure it out.
Created attachment 271577 [details] [review] Replaces "ans" in previous calculations by real ans This patch will now display the following results: 2 = 2 2*3= 6 = 6*3 = 18 = 18*3 = 54 However whenever I first restart the calculator I get the following error message in the terminal for the first few displays: gnome-calculator:6077 ** CRITICAL string contains assertion "NEEDLE!= NULL" failed I am unable to figure out what exactly is causing this causing this error though its not affecting any of the calculator -operations. I will try to fix it if required. If anyone has any idea on what causes this error message please let me know so that I can make the required changes in the patch. Thankyou.
Review of attachment 271577 [details] [review]: Elita asked me to review this patch, but I'm not very familiar with this code, so I'll just mention a few issues that I see. First off: this patch fixes the bug as reported, but it introduces a new issue when using undo after multiple calculations. For example, type 2x2 and hit enter. 4 is displayed. Now type x2 and hit enter. 8 is displayed. Now hit Undo. 8x2 is displayed, but I ought to see 4x2 again. Also, you should try to match the style of the surrounding code. For example, Calculator uses four-space indentations, always has spaces around operations like = and &&, never leaves spaces after function argument names, always leaves a space before the opening parenthesis in a function call or an if statement, and always places braces on their own line. There should also not be a blank line before an else or else if, and omit empty else blocks. ::: src/math-equation.vala @@ +17,3 @@ } +public static int b; +public static string prev_ans=null; Please don't use global variables. Instead, make these members of MathEquation. Also, b needs to have a more descriptive name. It's often OK for local variables to have short names, but rarely does that make sense for global variables or member variables. @@ +689,3 @@ if (ans_start_mark != null) get_ans_offsets (out ans_start, out ans_end); + if(prev_ans in text){ The critical warnings are probably caused because prev_ans could be null here. You probably want to write something like `if (prev_ans != null && prev_ans in text)`
Created attachment 272227 [details] [review] fixes bug "repeating previous calculations doesn't show right calculations" I have ensured that whatever expression is pushed into the stack , the same is pushed.Earlier the intermediate expressions were pushed in the undo stack and immediately popped out, however this patch checks that the intermediate expressions are not popped out from the stack unless undo is clicked. I might have missed out some cases. :/ So kindly review the patch and let me know if It can be a possible way to fix the bug and if it requires any modification. Thankyou
Review of attachment 272227 [details] [review]: Thanks for the update! I'm still getting issues with Undo. For example, type 2x2 and hit enter. 4 is displayed. Now type x2 and hit enter. 8 is displayed. Now hit Undo. 4x2 is displayed, but if I hit enter, I get 4x2 = 16. (So it's the opposite of the problem with the original patch.) ::: src/math-equation.vala @@ +33,3 @@ public uint error_token_end; /* End offset of error token */ } +public static int flag; Again, I don't think we need should be using global variables here. A global with a generic name like "flag" is especially confusing. Can you give this a longer name and move it into the SolveData class? Also, I think this should probably be a bool. @@ +429,3 @@ state.ans = s.ans; state.ans_base = s.ans_base; + var ans_text = serializer.to_string(state.ans); Style nit: please leave a space before opening parenthesis in a function call. You forget to do this in several other places, as well. @@ +431,3 @@ + var ans_text = serializer.to_string(state.ans); + var text = s.expression; + if(flag == 0) Style nit: please leave a space before opening parenthesis @@ +433,3 @@ + if(flag == 0) + { + text=text.splice( 0, s.ans_end, ans_text ); Another style nit: I suspect the surrounding code places a space on either side of the = operator. You should do that as well (here and below where you write flag=0). Also, no extra space on the inside of the parentheses in the function call. @@ +498,3 @@ } + + public void undo1 () Can we give this a more meaningful name?
Created attachment 274224 [details] [review] Possible fix for bug 694683 Respected Sir, I have made the changes as you suggested. Kindly let me know if I have left out any changes in the patch or if i have missed out some cases. Also I apologize for the delay in submitting this new new patch. Thankyou.
This patch seems to work well -- at least, I haven't been able to break it.
Review of attachment 274224 [details] [review]: The patch seems to work. However it fails in certain conditions (as said by Elita on mail). Also, it's not a good idea to store the answer as string. It looses the precesion of the Number. There are quite a few coding-style issues. I've pointed them out at their first appearance. ::: src/math-equation.vala @@ +413,2 @@ if (do_remove_tag) + { Change must not have trailing whitespaces. @@ +429,3 @@ /* Disable undo detection */ in_undo_operation = true; + Whitespace changes + trailing whitespace. @@ +432,3 @@ state.ans = s.ans; state.ans_base = s.ans_base; + var ans_text = serializer.to_string(state.ans); Function calls must have whitespace before parentheses. @@ +436,3 @@ + int k1=0; + var index=0; + int k=0; Whitespace surrounding assignments. e.g. k<whitespace>=<whitespace>0; @@ +438,3 @@ + int k=0; + unichar c; + const unichar superscript_digits[] = {'⁰', '¹', '²', '³', '⁴', '⁵', '⁶', '⁷', '⁸', '⁹'}; Shoule have whitespace before square brackets. e.g. x<whitespace>[10]; @@ +443,3 @@ + var i= index; + unichar next_char; + if ((ans_text.get_next_char (ref i, out next_char))&&(next_char in superscript_digits)) Whitespace surrounding binary operations. e.g. x<whitespace>&&<whitespace>y; @@ +445,3 @@ + if ((ans_text.get_next_char (ref i, out next_char))&&(next_char in superscript_digits)) + { + length=length+1; Whitespace surrounding "=" and "+". @@ +495,2 @@ { + Do not add empty lines in non-modified code. @@ +503,3 @@ get_iter_at_offset (out end, s.ans_end); ans_end_mark = create_mark (null, end, true); + apply_tag (ans_tag, start, end); Trailing whitespace. @@ +742,3 @@ text = text.splice (text.index_of_nth_char (ans_start), text.index_of_nth_char (ans_end), "ans"); + } + if((ans_start >= 0)&&(undoflag == false)&&(undo_equal == true)) Must have whitespace between function-call-like identifiers (if, while, for). e.g. if<whitespace>(x == 0);
Created attachment 277076 [details] [review] patch for showing previous calculations on clicking undo This patch works for all cases. Kindly let me know if it requires any modifications to be made.
Created attachment 278391 [details] [review] Restore ans to previous value when undo Here my (single-line) patch for this issue. IMO this behavior (2 <enter> ×2 <enter> <enter> will display 2×2 ) is more useful and simpler to understand for users.
Created attachment 278442 [details] [review] patch for showing previous calculations
Review of attachment 278391 [details] [review]: Rodolphe, Thanks for the patch.. :) It works just fine. Committed.
Review of attachment 278442 [details] [review]: Sorry, the issue has already been fixed. The patch seems a bit too complicated to fix the issue.
Thanks for the bug report, discussions and patches. The issue has been fixed in development version, and it'll be available with next major release of Calculator. Thanks once again.. :)