movescu's --bit-preserving option is not respecting the --output-directory option (issue #1122) #98
Conversation
Simplest attempt at correcting issue #1122. When movescu is called with +B (--bit-preserving) option, the output file ends up in the current working directory as opposed to the desired output directory even if you add the -od (--output-directory) option in the command line. Note that a better approach could be to have a singular writing function and adjust the function signatures to pass whether other processing logic should be skipped. However, that could cause a lot of headaches ensuring all logic paths are checked against the current bit-preserving behavior of outputting the data as-is. I leave the option to the maintainer if to request further work to address this issue more comprehensively or if the change proposed here is good enough for the project. The changes here appears to work as intended in my test environment.
Stylistic correction to comments
Unlike movescu, storescp does not recompute the file path in the storeSCP callback. It assumes that imageFileName already includes the directory name. However, I separated the imageFileName generation from the full path generation, so I realized I needed to correct the deleteFile logic as well. Will need to do something like this in movescu to ensure consistency across both codebases.
Keeping consistency between movescu and storescp so they both look at the file path in a similar manner.
Attempting to resolve merge conflict that arose in storescp due to new commits in the upstream mirror. It looks like it is addressing the path construction with a slightly different approach. A second commit will be added after I check if the directory path is getting added twice.
dcmnet/apps/movescu.cc
Outdated
| @@ -1468,11 +1472,11 @@ static OFCondition storeSCP( | |||
| /* remove file */ | |||
| if (!opt_ignore) | |||
| { | |||
| if (strcmp(imageFileName, NULL_DEVICE_NAME) != 0) OFStandard::deleteFile(imageFileName); | |||
| if (strcmp(imageFileName, NULL_DEVICE_NAME) != 0) OFStandard::deleteFile(ofname.c_str()); | |||
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
We observed that in these codes. Multiline blocks are missing. It is considered best practice to enclose all multiline blocks including if else blocks, loops, and function bodies within curly braces. To mitigate this issue, it serves to clearly define the scope of the block and avoid logic errors by grouping all related statements together visually.
dcmnet/apps/movescu.cc
Outdated
| } | ||
| #ifdef _WIN32 | ||
| } else if (opt_ignore) { | ||
| if (strcmp(imageFileName, NULL_DEVICE_NAME) != 0) OFStandard::deleteFile(imageFileName); // delete the temporary file | ||
| if (strcmp(imageFileName, NULL_DEVICE_NAME) != 0) OFStandard::deleteFile(ofname.c_str()); // delete the temporary file |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
We observed that in these codes. Multiline blocks are missing. It is considered best practice to enclose all multiline blocks including if else blocks, loops, and function bodies within curly braces. To mitigate this issue, it serves to clearly define the scope of the block and avoid logic errors by grouping all related statements together visually.
There was a problem hiding this comment.
Good morning. I was preserving the style as it was before the changes. Per your request/suggestions, I went ahead and added the scope braces.
Now that master is using snprintf(), I decided to remove my previously proposed solution in favor of the snprintf() suggestion. Also, adding bracket to if statement per comments in PR.
|
Hello @michaelonken ! I just wanted to check in with you if the current changes meet acceptance or if I need to make any further changes. I will be happy to do what I can to help out. Thank you! |
|
Luis, I didnt have to to check them in detail. I try to do this until end of the week. |
|
Thanks for your work Luis, I finally merged it with little changes into DCMTK (commit 6a6301). The change will be visible on master within the next days. |
|
@michaelonken Thank you for letting me know. Does that mean it is safe to remove my forked feature branch? I want to make sure I understand this project's workflow. Again, thank you! This is great news for my Friday! Haha! |
|
Correct, the code is now in an internal branch that undergoes automated testing on various platforms over night. Once we are happy with the outcome (i.e. there are no warnings or errors from the new code and it works as expected), we publish it to the master branch and push it public. Enjoy your weekend 😄 |
Simplest attempt at correcting issue #1122.
When movescu is called with +B (--bit-preserving) option, the output file ends up in the current working directory as opposed to the desired output directory even if you add the -od (--output-directory) option in the command line.
Note that a better approach could be to have a singular writing function and adjust the function signatures to pass whether other processing logic should be skipped. However, that could cause a lot of headaches ensuring all logic paths are checked against the current bit-preserving behavior of outputting the data as-is.
I leave the option to the maintainer if to request further work to address this issue more comprehensively or if the change proposed here is good enough for the project.
The changes here work as intended in my test environment.
https://support.dcmtk.org/redmine/issues/1122