Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI Refactoring #243

Merged
merged 44 commits into from
Sep 10, 2020
Merged

UI Refactoring #243

merged 44 commits into from
Sep 10, 2020

Conversation

adecler
Copy link
Member

@adecler adecler commented Aug 1, 2020

NOTE: Depends on

BHoM/BHoM_UI#297

Issues addressed by this PR

See BHoM/BHoM_UI#297 for details

See comment below: https://github.com/BHoM/Excel_Toolkit/pull/243#issuecomment-678620148

Also fixes a series of pre-existing issues:
Fixes #242
Fixes #233
Fixes #218
Fixes #187
Fixes #149
Fixes #240
Fixes #244

Test files

Changelog

Additional comments

@adecler adecler self-assigned this Aug 1, 2020
@adecler adecler added size:XL Measured in weeks status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge status:WIP PR in progress and still in draft, not ready for formal review type:bug Error or unexpected behaviour type:compliance Non-conforming to code guidelines type:feature New capability or enhancement labels Aug 1, 2020
@adecler adecler added this to the BHoM 3.3 β MVP milestone Aug 1, 2020
@adecler
Copy link
Member Author

adecler commented Aug 27, 2020

@IsakNaslundBh , I think I have fixed the issues you mentioned above. Let me know if you still have problems

@IsakNaslundBh
Copy link
Contributor

IsakNaslundBh commented Aug 27, 2020

@IsakNaslundBh , I think I have fixed the issues you mentioned above. Let me know if you still have problems

  • Explode to not return '#VALUE'
  • Explode go deep to explode inner objects
  • Excel to not crash on open file
  • Excel to load up formulas saved. Had to re-enable the method and save again in my sheet, but after doing that, all worked just fine!

So think all first issues found fixed 👍

@adecler
Copy link
Member Author

adecler commented Sep 4, 2020

OK, I think I might have finally fixed the bug with Excel crashing when closing. I say might because this thing is very elusive but I haven't had the crash for the last 10+ times I have pen Excel so that's a good sign.

The weird thing is that the bug was related to registering to the WorkbookOpened event. After a lot of trial ad errors, it sees that this version is not crashing anymore 🤞 . For the record, every detail of the change is important. Even keeping App_WorkbookOpen private as it was before would trigger the crash again.

Let me know if you still notice the problem after this commit.

Edit: Trying to access the hidden sheet for internalised data still causes the bug (regardless of the sheet existing or not). It doesn't make sense since it is the exact same process as accessing the hidden sheet for caller but i'll just disable that for now until I found a solution to it. At least the bug doesn't occur on WorkbookOpened event anymore.

Edit2: Using NetOffice to try to access a sheet seems to be causing the same bug at closing time. Conclusion -> Let's get rid of that package.

@IsakNaslundBh
Copy link
Contributor

have opened and closed excel ~20 times in a row now with no crash. Given the nature of the bug, that ofc does not guarantee anything, but if not gone, it at least seem to have improved it significantly!

@adecler adecler removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Sep 5, 2020
@adecler
Copy link
Member Author

adecler commented Sep 8, 2020

For those that have already created new Excel spreadsheet with this PR:

In order to add support to multiple workbooks being opened at the same time, I had to slightly modify the structure of the sheet used to save the list of used components. This mean that, while your Excel file will still open, it will not be able to execute the existing BHoM formulas correctly.

Here's how to fix this:

  • Unhide the BHoM_CallersHidden (right click on any sheet name and select unhide. A menu will pop up to select that sheet)
  • Insert a new row at the top and write a random string
  • Hide the sheet again
  • Save, close and reopen the file.

image

Copy link
Member

@al-fisher al-fisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This so LGTM!
Great stuff @adecler
As discussed ready for wider alpha testing on master!

@al-fisher
Copy link
Member

/azp run Excel_Toolkit.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XL Measured in weeks type:bug Error or unexpected behaviour type:compliance Non-conforming to code guidelines type:feature New capability or enhancement
Projects
None yet
3 participants