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

Edit window fix #2238

Merged
merged 3 commits into from
Aug 28, 2014
Merged

Edit window fix #2238

merged 3 commits into from
Aug 28, 2014

Conversation

Benglin
Copy link
Contributor

@Benglin Benglin commented Aug 27, 2014

Background

This pull request fixes a crash issue with node editing on Revit (see below for detailed test scenarios). The crash originated from the following statement on Revit:

public EditWindow(bool updateSourceOnTextChange = false)
{
    // In unmanaged host scenarios like Revit, 'Application.Current' will be 'null'.
    this.Owner = WPF.FindUpVisualTree<DynamoView>(Application.Current.MainWindow);
}

The philosophy of the fix is that if EditWindow needs an instance of DynamoViewModel, then it should always be supplied with one. And since EditWindow is on the view layer, it is completely fine for it to have a reference to DynamoViewModel.

Test Scenarios

The fix has been tested on the following scenarios:

  1. Click Edit... on context menu of a Note
  2. Click Rename Node... on context menu of a Node
  3. Click Edit... on context menu of a String node
  4. Click Edit... on Volume From String node
  5. Click Edit... on Phython node

Notes for Reviewers

Hi @sharadkjaiswal, @pboyer, please have a look. I have not been able to hit editWindowItem_Click method in String class (in BasicInteractive.cs), if you know it that'll be great. All other callers for EditWindow have been tested working.

@sharadkjaiswal
Copy link
Contributor

LGTM, thanks for the fix.

@Benglin
Copy link
Contributor Author

Benglin commented Aug 28, 2014

Thanks for checking this out. This crash seems pretty serious and if we could afford, I'll get @riteshchandawar to verify the fix (I have done to a certain degree, but Ritesh might have better coverage). Once that's verified we can then merge this in.

@riteshchandawar
Copy link
Contributor

I have test the fix with the scenarios mentioned by ben in the PR as well other scenarios like..

  1. Double click on edit (Note and Node Node Name)
  2. Edit Node and with/without changing anything click on Accept.
  3. Edit Note and make it Empty and click on Accept. (I am not sure whether we should allow this?)
  4. Python Node edit (Using Context menu and double click)
  5. Note and Node from Custom Node Workspace,
  6. Custom Node Renaming.
  7. Undo Redo of Node and Note editing. (it seems to have some problem but not a crash or a Stop-ship, it just takes more than one Undo command to undo the changes.)

All scenarios have been tested on Revit-2104, Revit -2015 and Vasari.

LGTM

Thanks,
Ritesh

@Benglin
Copy link
Contributor Author

Benglin commented Aug 28, 2014

Thanks @riteshchandawar, since this is affecting a number of our customers, given the test outcome I'm merging this in. If @pboyer has any concern we will then make changes on the fix. By the way, #7 is a known issue in the way Note requires more than one undo to get back the original text, I believe a defect has been written up for it before.

Benglin added a commit that referenced this pull request Aug 28, 2014
@Benglin Benglin merged commit f8d22da into DynamoDS:master Aug 28, 2014
@Benglin Benglin deleted the EditWindowFix branch August 28, 2014 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

daily: crash when editing node's name or note
3 participants