Differential D11466 Diff 37735 source/blender/blenlib/intern/storage_apple.mm

Changeset View

Standalone View

source/blender/blenlib/intern/storage_apple.mm

Context not available.
	*/	*/

	#import <Foundation/Foundation.h>	#import <Foundation/Foundation.h>
		#include <sys/xattr.h>

	#include "BLI_fileops.h"	#include "BLI_fileops.h"
	#include "BLI_path_util.h"	#include "BLI_path_util.h"
Context not available.
	return true;	return true;
	}	}

		bool listxattrResponseContainsAttribute(char attributes, size_t size, const char searchAttribute) {
		ankitmUnsubmitted Done Inline Actions See the `blender::StringRef::find` functions. ankitm:* See the `blender::StringRef::find*` functions.
		int i = 0;
		bool found = false;
		char *subString = attributes;

		while (i < size && !found) {
		if ('\0' == attributes[i]) {
		if (0 == strcmp(searchAttribute, subString)) {
		return true;
		} else {
		subString = &attributes[i+1];
		}
		}
		i++;
		}

		return false;
		}

		/**
		* Checks if the file is merely a placeholder for a OneDrive file that hasn't yet been downloaded.
		brechtUnsubmitted Done Inline Actions The `"\0"s` at the start seems wrong. From the `listxattr` man page it does not seem like it starts with a null terminator, so the first attribute would not be matched. brecht: The `"\0"s` at the start seems wrong. From the `listxattr` man page it does not seem like it…
		lzandmanAuthorUnsubmitted Done Inline Actions The `"\0"s` at the start seems wrong. From the `listxattr` man page it does not seem like it starts with a null terminator, so the first attribute would not be matched. The first attribute should be handled by if-statement above. By now I've done a dozen implementations of this function. The first one, which was C only, worked pretty well. But @Ankit Meel (ankitm) preferred a `std::string` solution and we've gone back and forth over a proper implementation. I hope this one suffices. lzandman: > The `"\0"s` at the start seems wrong. From the `listxattr` man page it does not seem like it…
		*
		* \param path: the path of the file.
		* \return 'true' when the file is a OneDrive placeholder, otherwise 'false'.
		*/
		bool oneDrive_checkIfFileIsPlaceholder(const char *path)
		{
		/* Note: Currently only checking for the "com.microsoft.OneDrive.RecallOnOpen" extended file attribute.
		* In theory this attribute can also be set on files that aren't located inside a OneDrive folder.
		* Maybe additional checks are required? */

		size_t size;
		ankitmUnsubmitted Done Inline Actions Use `ssize_t` and check for error (-1). https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/listxattr.2.html Also print corresponding errno to error message in that case. https://en.cppreference.com/w/cpp/string/byte/strerror Also see https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Variable_Scope ankitm: Use `ssize_t` and check for error (-1). https://developer.apple.
		lzandmanAuthorUnsubmitted Done Inline Actions and check for error (-1). Also print corresponding errno to error message in that case. https://en.cppreference.com/w/cpp/string/byte/strerror I did, but I didn't know what to do with the error. I opted for returning `false`, which is kind of weird. But you're saying I should print the error to the console? I didn't know that made sense for Blender. What should happen to the flow? I'm used to higher level programming languages that throw exceptions. Should I just print the error and be maintain my original flow? lzandman: > and check for error (-1). > Also print corresponding errno to error message in that case.
		ankitmUnsubmitted Done Inline Actions `size_t` is unsigned. So in case of error, its value would be a large positive number instead of -1. That case will not be caught by `size < 1`. Thus `ssize_t` should be used and it is what is returned by `listxattr` also. Returning false is fine. A debug statement to the console for the error is what I am suggesting: "Listing attributes failed: no such file or directory: /Users/t/a/b.jpeg" ankitm: `size_t` is unsigned. So in case of error, its value would be a large positive number instead…
		char *buffer = NULL;

		// Get extended file attributes
		size = listxattr(path, NULL, 0, XATTR_NOFOLLOW);
		if (size < 1) {
		return false;
		}

		buffer = (char )malloc(size sizeof(char));
		ankitmUnsubmitted Done Inline Actions Is `std::string` a better choice here ? It has methods to resize storage, cleans up after itself and gives access to underlying buffer by `.data()`. Later on, searching for substrings is easy with some extra care for the `\0`. https://godbolt.org/z/xf5xz3KnP ankitm: Is `std::string` a better choice here ? It has methods to resize storage, cleans up after…
		lzandmanAuthorUnsubmitted Done Inline Actions Funny, because I started out using NSString type. But I figured since most of Blender seems to consist of low-level C-code these "fancy" types should be avoided. I will look into it. lzandman: Funny, because I started out using NSString type. But I figured since most of Blender seems to…
		ankitmUnsubmitted Done Inline Actions That might have been an obj-C file, not an obj-C++ file. C++ containers/ STL (and their replacements like `blender::Map` are preferred over low level fiddling). ankitm: That might have been an obj-C file, not an obj-C++ file. C++ containers/ STL (and their…
		lzandmanAuthorUnsubmitted Done Inline Actions I've used the std::string find() method and threw away my custom code. lzandman: I've used the std::string find() method and threw away my custom code.
		if (NULL == buffer) {
		// Failed to allocate buffer
		brechtUnsubmitted Done Inline Actions Don't print any errors to the console, this message is not useful to users as something they can fix, it will just be an annoyance. brecht: Don't print any errors to the console, this message is not useful to users as something they…
		lzandmanAuthorUnsubmitted Done Inline Actions Getting conflicting review remarks, as @Ankit Meel (ankitm) asked me to include the error printing :-) I will remove it. lzandman: Getting conflicting review remarks, as @ankitm asked me to include the error printing :-) I…
		return false;
		}
		lzandmanAuthorUnsubmitted Done Inline Actions It just occurred to me that in the switch to using `std::string find()` method I've introduced a bug. Semantically we are no longer comparing each individual attribute, but doing a substring search on the complete list of attributes as a string. This produces wrong results when the attribute searched for isn't included itself, but a different attribute that happens to start with the same characters is present. So this result of `listxattr()` would generate a positive result, even though it doesn't contain the attribute: `"com.microsoft.OneDrive.RecallOnOpen.SomethingElse\0com.microsoft.OneDrive.RecallOnOpen.NothingElseMatters\0"` Guess I'll update the code. Or revert back to my "dumb", but perfect code :-) lzandman: It just occurred to me that in the switch to using `std::string find()` method I've introduced…

		size = listxattr(path, buffer, size, XATTR_NOFOLLOW);
		// In case listxattr() has failed the second time it's called...
		if (size < 1) {
		free(buffer);
		return false;
		}

		const char *ONEDRIVE_RECALLONOPEN_ATTRIBUTE = "com.microsoft.OneDrive.RecallOnOpen";
		ankitmUnsubmitted Done Inline Actions Can be a global variable in case others are added. ankitm: Can be a global variable in case others are added.
		lzandmanAuthorUnsubmitted Done Inline Actions As in: global to this file? Or should it be globally defined in some other (header) file? lzandman: As in: global to this file? Or should it be globally defined in some other (header) file?
		ankitmUnsubmitted Done Inline Actions this file ankitm: this file
		bool found = listxattrResponseContainsAttribute(buffer, size, ONEDRIVE_RECALLONOPEN_ATTRIBUTE);
		ankitmUnsubmitted Done Inline Actions `const` ankitm: `const`

		free(buffer);

		return found;
		}

		/**
		* Checks if the file is a placeholder for a cloud drive file that hasn't yet been downloaded.
		*
		* \param path: the path of the file.
		* \return 'true' when the file is a placeholder, otherwise 'false'.
		*/
		bool checkIfFileIsPlaceholder(const char *path)
		{
		// Logic for additional cloud storage providers could be added in future...
		ankitmUnsubmitted Done Inline Actions Comment style (// vs /) should be uniform in a file. Also remove the ellipses. ankitm:* Comment style (// vs /*) should be uniform in a file. Also remove the ellipses.
		return oneDrive_checkIfFileIsPlaceholder(path);
		}

	eFileAttributes BLI_file_attributes(const char *path)	eFileAttributes BLI_file_attributes(const char *path)
	{	{
	int ret = 0;	int ret = 0;
Context not available.
	const NSURL *fileURL = [[NSURL alloc] initFileURLWithFileSystemRepresentation:path	const NSURL *fileURL = [[NSURL alloc] initFileURLWithFileSystemRepresentation:path
	isDirectory:NO	isDirectory:NO
	relativeToURL:nil];	relativeToURL:nil];
	NSArray *resourceKeys =
	@[ NSURLIsAliasFileKey, NSURLIsHiddenKey, NSURLIsReadableKey, NSURLIsWritableKey ];	/* If the file is in a OneDrive folder and OneDrive's Files On-Demand feature is turned on
		* and the file is only a placeholder for an online file, we shouldn't query the file for
		* NSURLIsReadableKey and NSURLIsWritableKey, because that will trigger OneDrive to actually
		* download the file. */
		ankitmUnsubmitted Done Inline Actions Shorter, drive service agnostic comment would be fine too. Querying `NSURLIsReadableKey` and `NSURLIsWritableKey` keys for placeholder files in cloud services triggers their download. ankitm: Shorter, drive service agnostic comment would be fine too. Querying `NSURLIsReadableKey` and…
		lzandmanAuthorUnsubmitted Done Inline Actions But currently I only noticed this issue for OneDrive placeholder files. iCloud Drive doesn't seem affected by this. And I haven't tested other cloud storage services. I'm in favor of a shorter comment, but I do think it should be OneDrive-specific for now. lzandman: But currently I only noticed this issue for OneDrive placeholder files. iCloud Drive doesn't…
		NSArray *resourceKeys = NULL;
		bool fileIsPlaceholder = checkIfFileIsPlaceholder(path);
		ankitmUnsubmitted Done Inline Actions `const bool ...` ankitm: `const bool ...`

		if (fileIsPlaceholder) {
		resourceKeys =
		@[ NSURLIsAliasFileKey, NSURLIsHiddenKey ];
		ankitmUnsubmitted Done Inline Actions What are the values of `is_readable` and `is_writable` in the case of a placeholder file ? ankitm: What are the values of `is_readable` and `is_writable` in the case of a placeholder file ?
		lzandmanAuthorUnsubmitted Done Inline Actions I wondered what to do with those. They are `false` for placeholder files, as no corresponding attribute is found in the collection (because I didn't query for them). The result is the file is marked `FILE_ATTR_SYSTEM` which does seem kind of weird, but it seems to work out. I think currently the only consequence is that the file is displayed using `ICON_SYSTEM` (line 1240 in `source/blender/editors/space_file/filelist.c`). lzandman: I wondered what to do with those. They are `false` for placeholder files, as no corresponding…
		} else {
		resourceKeys =
		@[ NSURLIsAliasFileKey, NSURLIsHiddenKey, NSURLIsReadableKey, NSURLIsWritableKey ];
		}

	const NSDictionary *resourceKeyValues = [fileURL resourceValuesForKeys:resourceKeys error:nil];	const NSDictionary *resourceKeyValues = [fileURL resourceValuesForKeys:resourceKeys error:nil];

Context not available.
	if (!is_readable) {	if (!is_readable) {
	ret \|= FILE_ATTR_SYSTEM;	ret \|= FILE_ATTR_SYSTEM;
	}	}
		if (fileIsPlaceholder) {
		ret \|= FILE_ATTR_OFFLINE;
		}
	}	}

	return (eFileAttributes)ret;	return (eFileAttributes)ret;
Context not available.