BHoM / Excel_UI

GNU Lesser General Public License v3.0
5 stars 4 forks source link

Fix internalise bug on spill arrays #339

Closed adecler closed 2 years ago

adecler commented 2 years ago

Issues addressed by this PR

Closes #315

The internalise function was replacing the formula of each cell with its string representation in order. This means that, as soon as the first cell of an expanded array was replaced, all the other cells would loose their value (since they don't contain a formula themselves). To fix this, we are now using the cached value of each cell.

Test files

https://burohappold.sharepoint.com/:f:/s/BHoM/EoFSYhhfB7VIo83fGEiQZksBx-GMQJ2upK1nbYfLvpYHIQ?e=Nui359

adecler commented 2 years ago

@BHoMBot check compliance

bhombot-ci[bot] commented 2 years ago
@adecler to confirm, the following checks are now queued: - `code-compliance` - `documentation-compliance` - `project-compliance` - `branch-compliance` - `dataset-compliance` - `copyright-compliance`
adecler commented 2 years ago

@BHoMBot check required

bhombot-ci[bot] commented 2 years ago
@adecler to confirm, the following checks are now queued: - `code-compliance` - `documentation-compliance` - `project-compliance` - `core` - `null-handling` - `serialisation` - `versioning` - `installer`
bhombot-ci[bot] commented 2 years ago
The check `code-compliance` has already been run previously and recorded as a successful check. This check has not been run again at this time.
bhombot-ci[bot] commented 2 years ago
The check `documentation-compliance` has already been run previously and recorded as a successful check. This check has not been run again at this time.
kayleighhoude commented 2 years ago

Getting to this testing now

adecler commented 2 years ago

@kayleighhoude ,

The behaviour you are complaining about is exactly the same you would get by using the standard copying by value in both cases. I would stick to my policy of replicating behaviours that an expert Excel user would expect from native Excel functionality.

@FraserGreenroyd , I will leave the final decision to you but I am personally happy to merge this PR despite Kayleigh's comment.

FraserGreenroyd commented 2 years ago

I have retested based on @kayleighhoude comments, and while I can experience the same concern, I have also confirmed this occurs with native Excel components. As such, I am in agreement with @adecler that this is default Excel behaviour and should not be addressed at this time, certainly not within the scope of this PR.

This PR brings in a benefit to the codebase, in fixing the issue of internalising a selection of cells which have previously been expanded, and as such I see benefit to including this in the 5.1 beta later this week. Owing to the approving review from myself, and the confirmation from @kayleighhoude in their review that this PR does resolve the originally reported issue, I am merging this PR for distribution in the final beta test artefacts ahead of the final beta release at the end of this week.

If there are more items to discuss, or questions, as part of this work, I recommend raising them as new issues for appropriate discussion 😄