Bug 161338 - Goal seek corrupts the data in Calc cell
Summary: Goal seek corrupts the data in Calc cell
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
24.2.3.2 release
Hardware: All All
: medium normal
Assignee: Rafael Lima
URL:
Whiteboard: target:25.2.0 target:24.8.0.0.beta2
Keywords:
Depends on:
Blocks: GoalSeek
  Show dependency treegraph
 
Reported: 2024-05-30 08:39 UTC by grofaty
Modified: 2024-06-17 13:42 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
goal_seek.ods (9.49 KB, application/octet-stream)
2024-05-30 08:40 UTC, grofaty
Details

Note You need to log in before you can comment on or make changes to this bug.
Description grofaty 2024-05-30 08:39:06 UTC
Description:
When using Tools | Goal Seek and question appears to fill the number, clicking on No button I expect to cancel all following operations, but instead #N/A is inserted and so corrupts value in cell.

Steps to Reproduce:
1. Type in:
A1: 250
B1: 0.25
C1: 200
D1 = A1 * B1 / C1

2. Click on D1 and Tools | Goal Seek
3. Formula Cell is already filled with $D$1. Leave this field as is.
4. Click on Variable cell and type in: $C$1 (or click on appropriate Calc cell).
5. Click on Target value and type in 100 and click OK button.
6. Dialog is displayed: Insert the closes value (200) into the variable cell anyway? I don't want 200, because I already have 200, so I click on No button. Now problem appears. Click on C1 cell and you will see int "=" row that #N/A was inserted despite in cell C1 itself it is still value 200 displayed. Now click on A1 and type in 300 and press Enter and now error appears in C1 and D1 with #N/A value. This is strange, because I have clicked on No button and I expect no action is followed.

Actual Results:
C1 gets corrupted with #N/A

Expected Results:
1. If I click on No, I expect no action to follow, so not corrupting C1 cell.

2. It is actually unclear what Yes/No buttons actually do. This is not the best UX. I think buttons should never be Yes/No, but instead "action" and "cancel" like "Insert" and "Cancel" in this case, so user can be absolutely sure what individual button action really does.



Reproducible: Always


User Profile Reset: No

Additional Info:
Version: 24.2.3.2 (X86_64) / LibreOffice Community
Build ID: 433d9c2ded56988e8a90e6b2e771ee4e6a5ab2ba
CPU threads: 12; OS: Windows 10.0 Build 22631; UI render: Skia/Vulkan; VCL: win
Locale: sl-SI (en_SI); UI: en-US
Calc: CL threaded
Comment 1 grofaty 2024-05-30 08:40:22 UTC
Created attachment 194447 [details]
goal_seek.ods
Comment 2 m_a_riosv 2024-05-30 11:35:19 UTC
Reproducible
Version: 24.8.0.0.alpha1+ (X86_64) / LibreOffice Community
Build ID: 6084962f93efc005b6827edceae12d3170f17ccd
CPU threads: 16; OS: Windows 11 X86_64 (10.0 build 22631); UI render: Skia/Raster; VCL: win
Locale: es-ES (es_ES); UI: en-US
Calc: CL threaded
Comment 3 ady 2024-05-30 12:08:51 UTC
So, 2 items to resolve:
* What should had happened.
* Improve UX so it is (more) clear to users what to expect in each case/selection/button.
Comment 4 Heiko Tietze 2024-06-07 07:38:55 UTC
Shouldn't the help tells you how to use the dialog? Maybe with some examples. 
https://help.libreoffice.org/24.2/en-US/text/scalc/01/06040000.html

And if the calculation fails we should not alter the cell content.
Comment 5 Rafael Lima 2024-06-07 17:46:24 UTC
(In reply to Heiko Tietze from comment #4)
> Shouldn't the help tells you how to use the dialog? Maybe with some
> examples. 

Indeed the help page can be improved in this regard.

> And if the calculation fails we should not alter the cell content.

If the user clicks "No", we should not alter the original value (note that Goal Seek doesn't respond to Undo - Ctrl+Z, so the original value is permanently lost). 

When Goal Seek succeeds and we click "No", the variable cell is left unchanged, which is the expected behavior.

Also, I think it's ok to keep Yes/No as the labels here, provided that they behave correctly.

Yes = Insert the calculated value
No = Do not insert the calculated value

One weird thing is that this calculation should not fail... it has a solution of 0.625. If the value in C1 is 200, Calc will fail to find the solution, but if the value in C1 is changed to 1, Calc will find the solution. This seems to be is a separate bug.
Comment 6 Rafael Lima 2024-06-07 18:27:25 UTC
Proposed fix here:
https://gerrit.libreoffice.org/c/core/+/168537
Comment 7 grofaty 2024-06-08 11:37:54 UTC
@Rafael Lima, actually according to the instruction from the original post if in C1 we set 100, then Goal Seek should not produce an error in the first place it should offer solution for B1 with value 0.125.

There are actually multiple problems:
1. UI is confusing Yes/No buttons should ever never be used. It should always be used "action" and "cancel", in our case action = Insert. See for example the following article: https://uxplanet.org/7-basic-rules-for-button-design-63dcdf5676b4 paragraph "3. Label buttons with what they do" where it is explained how should interface look like to bi user friendly.
2. If error appears it should not corrupt cells with #N/A
3. If something unexpected happens, user should have ability to undo, but Ctrl+Z does not undo the Goal Seek action.
4. In this particular case Goal Seek should not even fail. Following the steps Goal Seek should bind a solution for B1 cell with value 0.125.
Comment 8 Heiko Tietze 2024-06-10 07:04:22 UTC
(In reply to grofaty from comment #7)
> 1. UI is confusing Yes/No buttons should ever never be used.
"Insert the result into the variable cell?" Yes/No
"Insert the closest value (200) into the variable cell anyway?" Yes/No

Sounds clear to me. And while the positive answer could be replaced I see no good explanation what No means. "Insert the..." Insert/Cancel -> Yes/No sounds better to me.

In addition to your list of other bugs (please file extra tickets) I struggle with the confirmation box on top of the GS dialog. I understand the No as cancel and expect to get back to the GS dialog. Either we just proceed without any confirmation if a result has been found and inform but do nothing if not, or we keep the GS dialog behind and go back on No/Cancel.
Comment 9 Commit Notification 2024-06-12 14:29:45 UTC
Rafael Lima committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/d05f1ec2e20eb86ceb99e9ff5d4efb9ff9356dbb

tdf#161338 Do not change variable cell when Goal Seek fails

It will be available in 25.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 10 Rafael Lima 2024-06-12 14:34:49 UTC
(In reply to grofaty from comment #7)
> There are actually multiple problems:

As for issue [1], I agree with Heiko that "Yes/No" here is good already.

Issue [2] is fixed by the patch

Issue [3] is no longer relevant since issue [2] is fixed. When an error happens, nothing will change in the document, so no Undo necessary.

Issue [4] should be filed separately. However, I'd say bug 149333 already complains about the Goal Seek solver failing when it should succeed.
Comment 11 Commit Notification 2024-06-13 08:38:27 UTC
Rafael Lima committed a patch related to this issue.
It has been pushed to "libreoffice-24-8":

https://git.libreoffice.org/core/commit/01f92e1ed22cb23b60052964a7203801393e5acf

tdf#161338 Do not change variable cell when Goal Seek fails

It will be available in 24.8.0.0.beta2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 12 Commit Notification 2024-06-14 04:07:29 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/ffa5c8e13970403319a6761811fd79fb51dbaff7

tdf#161338: Pass a flag to ScDocument::Solver to avoid #NA! on error

It will be available in 25.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 13 Mike Kaganski 2024-06-14 04:18:19 UTC
(In reply to Rafael Lima from comment #10)
> Issue [3] is no longer relevant since issue [2] is fixed. When an error
> happens, nothing will change in the document, so no Undo necessary.

Since commit above, the Undo should also be OK in case of Yes here (and as expected, no change / no Undo corruption in case of No).
Comment 14 Commit Notification 2024-06-14 09:59:28 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "libreoffice-24-8":

https://git.libreoffice.org/core/commit/2b3e57c7d5726bf49dba224a337650bbc4395eaa

tdf#161338: Pass a flag to ScDocument::Solver to avoid #NA! on error

It will be available in 24.8.0.0.beta2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.