Fix external document loading #4952
Conversation
|
Thank you @realthunder!! I'll see if I can find some time to review and verfiy this fix. |
luzpaz
added
the
Core
| objNames.swap(it->second); | ||
| } | ||
| int pass = 0; | ||
| do { |
There was a problem hiding this comment.
Why use a do... here? Given the exit condition !_pendingDocs.empty(), you actively don't want this to guarantee the first iteration given your access to _pendingDocs.front(). In addition, the Core Guidelines advise against its use (see http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es75-avoid-do-statements). I suggest switching to a while...:
There was a problem hiding this comment.
Well, I think this would be a 'genuine examples where a do-statement is a clear statement of a solution'. _pendingDocs may get pushed with new entry at the very end of this very long loop statement. And there is a big block of comments explaining the reason, about 10 lines above the exiting while(). Although this is not the only place where _pendingDocs is refilled, but I just want emphasize that push which is the solution to the problem that this PR is trying to solve.
There was a problem hiding this comment.
My concern here is that when someone reads this do...while loop, it calls their attention to the fact that you are trying to guarantee at least one execution, regardless of loop end condition. That is contrary to the code itself, however: if _pendingDocs were indeed empty upon entering this loop, you very much do NOT want to execute it once, the statement _pendingDocs.front() is almost sure to crash.
I argue that it's much more comfortable for a programmer reading this code to keep the loop end condition in mind as they read, despite the length of the loop. As written the only way to read it is to first scroll to the end, note the end condition, then scroll back to the top. Then you have to think "OK, why do we want to guarantee one execution of this loop, even if the end condition is satisfied?" And then you see line 704... it's very confusing. I don't think you accomplish your stated goal of emphasizing the final push: your large comment block emphasizes that without trouble: using this very non-idiomatic loop construction only muddies the waters.
There was a problem hiding this comment.
My concern here is that when someone reads this do...while loop, it calls their attention to the fact that you are trying to guarantee at least one execution, regardless of loop end condition.
I think that's where our difference is. When I see a do {} while loop, I think of it as not worth it to check for loop entry condition because it is already guaranteed. And it is, as you can see with the few lines before the loop. Maybe it would be more obvious if I move the code to populate _pendingDocs closer together?
I see do {} while loop more for the emphasis on exit condition rather than entry, and the second half of this loop (which is newly added by this PR) is dedicated for checking the exit condition, so it would be logical to put the final check at the end.
There was a problem hiding this comment.
We can agree to disagree on this point: every developer is a little different, after all! I don't think this is a deal-breaker for the merge, it's not worth either of us spending more time considering it, IMO.
src/Mod/Part/Gui/ViewProviderExt.cpp
Outdated
| @@ -472,6 +473,12 @@ void ViewProviderPartExt::onChanged(const App::Property* prop) | |||
| // if the object was invisible and has been changed, recreate the visual | |||
| if (prop == &Visibility && (isUpdateForced() || Visibility.getValue()) && VisualTouched) { | |||
| updateVisual(); | |||
| // updateVisual() may not be triggered by any change (e.g. | |||
| // triggered by an external object through forceUpdate()). And | |||
| // since DiffuseColor is not changed here either, do not falsly set | |||
There was a problem hiding this comment.
| // since DiffuseColor is not changed here either, do not falsly set | |
| // since DiffuseColor is not changed here either, do not falsely set |
|
@realthunder when you have a free moment, can you respond to the reviewers? thx! |
|
Yeah, I've already conversed with @chennes about it. I'd like to make some modifications according to the comments (this and a few other PR), but it takes quite some time to checkout a branch and compile and test. I am busy with a pending release at the moment. Will get back to it soon. |
|
No rush on my end, obviously, I'm just slowly working my way through these old PRs in the hopes that we can get them resurrected and merged. |
|
Following a link to the branch on the CI-repository: https://gitlab.com/freecad/FreeCAD-CI/-/commits/PR_4952 The CI-status is available on the latest commit of the branch. https://gitlab.com/freecad/FreeCAD-CI/-/pipelines?scope=branches |
|
a rebase on master would be great to see if CI runs fine ... |
The problem happens when partial loading is enabled. If document A contains a link to some object in document B, it will load B as partial document with only that object and its necessary dependencies. But if document A contains another link to some object in document C which also has a link to some object in document B, the link in document C may not be restored, because document B is partially loaded without the linked object. This patch will check for this case and reload document B for more objects. See an example reported in https://forum.freecadweb.org/viewtopic.php?p=495078#p495078
realthunder
force-pushed
the
FixDocumentRestore
branch
from
c785f25
to
9885dad
Compare
|
I notice that when I load the original example file from that forums discussion (which now works fine), if I then immediately close the file, it asks if I want to save my changes, even though I haven't knowingly made any. Is that just a consequence of the file structure? Is it expected? |
|
The author probably didn't save the external files in the correct order, that is, save the external documents first before the depending document, which FreeCAD will automatically do when saving the main document. As mentioned in the 'big block of comments' I mentioned earlier, the Link will mark itself as 'touched' if it detects the linked document has changed its |
Forum thread https://forum.freecadweb.org/viewtopic.php?p=495078#p495078
The problem happens when partial loading is enabled. If document A contains a link to some object in document B, it will load B as partial document with only that object and its necessary dependencies. But if document A contains another link to some object in document C which also has a link to some object in document B, the link in document C may not be restored, because document B is partially loaded without the linked object. This patch will check for this case and reload document B for more objects.