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

Set m_OriginalMethod to null when selecting a type #285

Merged

Conversation

IsakNaslundBh
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh commented Jun 29, 2020

Issues addressed by this PR

Closes #284

as stated in issue:

When SetItem is called for the general case that a method is being picked, m_OriginalMethod is being handled and set to the corresponding method or nulled out if not used in the SetItem on the method called.
On the CreateCaller on the other hand, when a type is selected, m_OriginalMethod is not touched.

For Grasshopper and dynamo this is not an issue, as they have a one-to-one relation between a component and one caller. For excel on the other hand this becomes an issue, as Excel_UI only owns one of each caller type, which is being used for all the calls that goes through that particular caller.

This means that if a create method is called, followed by a property assignment method, basically method followed by type, m_OriginalMethod is still set to the method.

This in turn causes issues with the serialisation of the component, as selected item prioritises m_OriginalMethod over SelectedItem when turned to JSON.

Test files

This is all happening on adding methods, hence no test file can be provided. In Excel: Try Creating an object with a create method, then creating an obejct with a property initialiser. Then either look at how the methods have been stored, and/or save and reopen the sheet.

Alsno need to make sure this does not cause any issues elsewhere in the other UIs.

Changelog

Additional comments

@IsakNaslundBh IsakNaslundBh added the type:bug Error or unexpected behaviour label Jun 29, 2020
@IsakNaslundBh IsakNaslundBh self-assigned this Jun 29, 2020
Copy link
Member

@rwemay rwemay left a comment

Choose a reason for hiding this comment

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

Tested Excel and this resolves the issue raised there.

Copy link
Member

@adecler adecler left a comment

Choose a reason for hiding this comment

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

I have no problem with this change as it doesn't break anything for any UI.
Based on @rwemay confirming that it fixes the problem in Excel, I am happy to approve.

@alelom alelom self-requested a review June 30, 2020 08:26
Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

This solves the issue for me in Excel.
This change does not cause any issue for me in Grasshopper. Have not tested Dynamo.

@pawelbaran
Copy link
Member

Seems to be fine with Dynamo too.

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.

Testing in Grasshopper and Excel - also working for me

@al-fisher
Copy link
Member

/azp run BHoM_UI.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@al-fisher al-fisher merged commit 7f4228b into master Jun 30, 2020
@al-fisher al-fisher deleted the BHoM_UI-#284-SetOriginalMethodToNullWhenSelectingType branch June 30, 2020 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BHoM_UI: m_OriginalMethod is not nulled out when type is selected for CreateObject
6 participants