-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Check Saved
on ProjectItem before calling Save
#46393
Check Saved
on ProjectItem before calling Save
#46393
Conversation
Why not check the |
That's hypothetical on my end. I'm digging in to get a better stack and find out. |
9f17262
to
2d186de
Compare
if (!projectItemForDocument.Saved) | ||
{ | ||
projectItemForDocument.Save(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonmalinowski are you familiar with this space at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I filed https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1163405 to track it being fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do believe we can do this as a workaround for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So looking at the internal bug I think it's still not clear (at least to me) why this works because we don't quite understand what's throwing in the first place. If the project system folks think this is good enough then I'm OK with it.
I would however like it if we can update this code here to say it's a workaround and include the link to the bug so we can at least have some understanding of why it's here, and can perhaps remove it some day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment and link to tracking bug
if (!projectItemForDocument.Saved) | ||
{ | ||
projectItemForDocument.Save(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So looking at the internal bug I think it's still not clear (at least to me) why this works because we don't quite understand what's throwing in the first place. If the project system folks think this is good enough then I'm OK with it.
I would however like it if we can update this code here to say it's a workaround and include the link to the bug so we can at least have some understanding of why it's here, and can perhaps remove it some day.
Hello @ryzngard! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-approval
…name Check `Saved` on ProjectItem before calling Save
Merge pull request #46393 from ryzngard/issue/46192_nre_save_rename
If a ProjectItem is retrieved but has no
Document
property, callingSave
throws a NRE. Add a check inApplyDocumentInfoChanged
if the item needs to be saved before doing so.Fixes #46192