This is a continuation of patch T21836 provided by Rob McKay
Included some cleanup and bringing it up-to date with latest code from myself.
Details
Details
Diff Detail
Diff Detail
- Branch
- elubie-uncpaths
Event Timeline
Comment Actions
I didn't test this patches but spotted some issues reading the code.
| source/blender/blenlib/BLI_path_util.h | ||
|---|---|---|
| 169–172 | I would make these static functions and not put them in the API, best to keep the API platform independent when possible. | |
| source/blender/blenlib/intern/path_util.c | ||
| 527–528 | This { position doesn't follow code style. | |
| 804 | I don't think this works on the path a\some.blend, in that case it should still switch the second character. | |
| 1528 | strlen(dir) >= 2 is unnecessary | |
| source/blender/blenlib/intern/storage.c | ||
| 493–503 | A comment about what the purpose of this code is would be good, it's not clear to me. | |
| 497 | I think this should be (*share) ? share+1 : share;. Otherwise the next line reads past the end of the array if no backslash is found. | |
| 501–502 | This seems like one increment too much, the folder++ line can be left out I think? | |
| source/blender/editors/space_file/file_ops.c | ||
| 1181 | Use PATH_MAX instead of 4096 here. | |
| 1184 | I don't think this works right, sfile->params->dir will be in utf8 but _fullpath will not assume that. I think you need to convert and use _wfullpath instead. I'm not quite sure what the purpose of this function call is though, if it is necessary it may be good to clarify with a comment. | |
| 1192 | This does not check if it writes past the end of the array. | |
| 1192–1197 | This code does not seem so reliable, I don't think it handles the \\?\ syntax but I also don't really understand what is supposed to be doing. | |
Comment Actions
- Basic support for UNC paths
Updated patch with changes: - created functions to make purpose of code clearer
- hopefully addressed all weak points, previous patch contained some blunders
Comment Actions
Looking good, some picky remarks.
| source/blender/blenlib/intern/path_util.c | ||
|---|---|---|
| 397 | *picky* spaces around operators. | |
| 478 | again, think strlen() can be removed here. | |
| 480 | *picky* prefer name BLI_path_*** as prefix for new functions. BLI_path_unc_prefix_len ? | |
| 515 | *picky* - odd, spaces after (, in other places too. | |
| 522 | again BLI_long_unc_to_short -> BLI_path_unc_to_short / BLI_path_unc_long_to_short / BLI_path_unc_shorten ? | |
| 524 | *picky* should use braces for multiline if's | |
| 527 | *picky* convention is to only have brace on newline for multiline if's | |
| 535 | *picky* spaces around operators. | |
| 809–816 | *picky* in this case no space after * | |
| 1528 | dont think strlen() is needed here can't BLI_path_is_unc() do this? | |
| source/blender/editors/space_file/file_ops.c | ||
| 1223 | *picky* }\nelse{ - use newline (code style) | |
Comment Actions
Hi,
would be nice if this could be reviewed, I hopefully fixed now all concerns.
Thanks,
Andrea
Comment Actions
Looks good (though I didn't test), suggested minor change.
| source/blender/blenlib/intern/path_util.c | ||
|---|---|---|
| 522 | Can't this be simplified to: /* Ensure paths are both UNC paths or are both drives */
const bool is_unc = BLI_path_is_unc(file);
if (BLI_path_is_unc(temp) != is_unc) {
....And then... /* Ensure both UNC paths are on the same share */
if (is_unc) {
.... | |
Comment Actions
Looks good to me, two minor suggestions. But I think this is ok to commit.
| source/blender/blenlib/intern/path_util.c | ||
|---|---|---|
| 397 | Maybe add a comment here that +2 is to skip the UNC case. | |
| source/blender/editors/space_file/file_ops.c | ||
| 1252–1257 | I would leave out this #if defined(..) since can_create_dir already handles it. | |
Comment Actions
Thanks Brecht and Campbell.
I'm preparing for commit now with small changes from Campbell. We decided to keep the whole can_create_dir windows only for now, removing the case handling inside the function.
For the second comment, changed to using the BLI_path_unc_prefix_len there as well to avoid magic numbers and also added comment.