Page MenuHome

Basic support for UNC paths on Windows
ClosedPublic

Authored by Andrea Weikert (elubie) on Feb 8 2014, 6:18 PM.

Details

Summary

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.

Diff Detail

Branch
arcpatch-D298

Event Timeline

Brecht Van Lommel (brecht) requested changes to this revision.Feb 9 2014, 2:54 PM

I didn't test this patches but spotted some issues reading the code.

source/blender/blenlib/BLI_path_util.h
175–178

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
607–608

This { position doesn't follow code style.

884–888

I don't think this works on the path a\some.blend, in that case it should still switch the second character.

1612

strlen(dir) >= 2 is unnecessary

source/blender/blenlib/intern/storage.c
492–502

A comment about what the purpose of this code is would be good, it's not clear to me.

496

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.

500–501

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.

Andrea Weikert (elubie) updated this revision to Unknown Object (????).Feb 15 2014, 5:17 PM
  • 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

Added Windows users as reviewers

Andrea Weikert (elubie) updated this revision to Unknown Object (????).Mar 11 2014, 9:17 PM

Removed some strlen occurrences and made code a bit more understandable in one place

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 ?

535

*picky* spaces around operators.

604

*picky* should use braces for multiline if's

607

*picky* convention is to only have brace on newline for multiline if's

893–900

*picky* in this case no space after *

1612

dont think strlen() is needed here can't BLI_path_is_unc() do this?

source/blender/editors/space_file/file_ops.c
1202

*picky* }\nelse{ - use newline (code style)

Andrea Weikert (elubie) updated this revision to Unknown Object (????).Apr 13 2014, 12:08 PM
  • Fixed some code style issues (picky remarks)
Andrea Weikert (elubie) updated this revision to Unknown Object (????).Apr 13 2014, 12:37 PM
  • Fix: parent operator still allowed navigating upwards when no parent directory

Hi,

would be nice if this could be reviewed, I hopefully fixed now all concerns.

Thanks,
Andrea

Looks good (though I didn't test), suggested minor change.

source/blender/blenlib/intern/path_util.c
602

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) {
....
Andrea Weikert (elubie) updated this revision to Unknown Object (????).Apr 21 2014, 3:19 PM
  • Further work on UNC paths:

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
1249–1254

I would leave out this #if defined(..) since can_create_dir already handles it.

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.