Desktop: Resolves #279: Add support for editable resources (#3305)

* Add support for editable resources

* Fixed handling of resource paths

* Add delay before saving resource

* Delete temp files and stop watching when switching notes

* Handle sync when resource has been edited

* Handle resource conflicts

* Refactoring and handle conflicts from edit watcher

* Added test unit for conflict handling when deleting resource

* Trying to fix Travis-ci test units

* Fixed typo

* Update TinyMCE.tsx

* Update AsyncActionQueue.ts

ReactNativeClient/lib/AsyncActionQueue.ts

@@ -7,6 +7,10 @@ export interface QueueItem {
 	context: any,
 }
 
+// The AsyncActionQueue can be used to debounce asynchronous actions, to make sure
+// they run in the right order, and also to ensure that if multiple actions are emitted
+// only the last one is executed. This is particularly useful to save data in the background.
+// Each queue should be associated with a specific entity (a note, resource, etc.)
 export default class AsyncActionQueue {
 
 	queue_:QueueItem[] = [];
@@ -64,6 +68,8 @@ export default class AsyncActionQueue {
 	}
 
 	waitForAllDone() {
+		if (!this.queue_.length) return Promise.resolve();
+
 		this.scheduleProcessing(1);
 
 		return new Promise((resolve) => {

ReactNativeClient/lib/BaseApplication.js

@@ -521,7 +521,7 @@ class BaseApplication {
 			DecryptionWorker.instance().scheduleStart();
 		}
 
-		if (this.hasGui() && action.type === 'SYNC_CREATED_RESOURCE') {
+		if (this.hasGui() && action.type === 'SYNC_CREATED_OR_UPDATED_RESOURCE') {
 			ResourceFetcher.instance().autoAddResources();
 		}
 

ReactNativeClient/lib/joplin-renderer/MdToHtml/rules/link_open.js

@@ -23,8 +23,9 @@ function installRule(markdownIt, mdOptions, ruleOptions) {
 		let icon = '';
 		let hrefAttr = '#';
 		let mime = '';
+		let resourceId = '';
 		if (isResourceUrl) {
-			const resourceId = resourceHrefInfo.itemId;
+			resourceId = resourceHrefInfo.itemId;
 
 			const result = ruleOptions.resources[resourceId];
 			const resourceStatus = utils.resourceStatus(ruleOptions.ResourceModel, result);
@@ -62,7 +63,7 @@ function installRule(markdownIt, mdOptions, ruleOptions) {
 		// https://github.com/laurent22/joplin/issues/2030
 		href = href.replace(/'/g, '%27');
 
-		let js = `${ruleOptions.postMessageSyntax}(${JSON.stringify(href)}); return false;`;
+		let js = `${ruleOptions.postMessageSyntax}(${JSON.stringify(href)}, { resourceId: ${JSON.stringify(resourceId)} }); return false;`;
 		if (hrefAttr.indexOf('#') === 0 && href.indexOf('#') === 0) js = ''; // If it's an internal anchor, don't add any JS since the webview is going to handle navigating to the right place
 
 		if (ruleOptions.plainResourceRendering || pluginOptions.linkRenderingType === 2) {

ReactNativeClient/lib/joplin-renderer/utils.js

@@ -141,6 +141,7 @@ utils.imageReplacement = function(ResourceModel, src, resources, resourceBaseUrl
 	if (ResourceModel.isSupportedImageMimeType(mime)) {
 		let newSrc = `./${ResourceModel.filename(resource)}`;
 		if (resourceBaseUrl) newSrc = resourceBaseUrl + newSrc;
+		newSrc += `?t=${resource.updated_time}`;
 		return {
 			'data-resource-id': resource.id,
 			src: newSrc,

ReactNativeClient/lib/models/Note.js

@@ -149,7 +149,7 @@ class Note extends BaseItem {
 			const id = resourceIds[i];
 			const resource = await Resource.load(id);
 			if (!resource) continue;
-			const resourcePath = options.useAbsolutePaths ? `file://${Resource.fullPath(resource)}` : Resource.relativePath(resource);
+			const resourcePath = options.useAbsolutePaths ? `${`file://${Resource.fullPath(resource)}` + '?t='}${resource.updated_time}` : Resource.relativePath(resource);
 			body = body.replace(new RegExp(`:/${id}`, 'gi'), markdownUtils.escapeLinkUrl(resourcePath));
 		}
 
@@ -174,12 +174,19 @@ class Note extends BaseItem {
 		this.logger().info('replaceResourceExternalToInternalLinks', 'options:', options, 'pathsToTry:', pathsToTry, 'body:', body);
 
 		for (const basePath of pathsToTry) {
-			const reString = `${pregQuote(`${basePath}/`)}[a-zA-Z0-9.]+`;
-			const re = new RegExp(reString, 'gi');
-			body = body.replace(re, match => {
-				const id = Resource.pathToId(match);
-				return `:/${id}`;
-			});
+			const reStrings = [
+				// Handles file://path/to/abcdefg.jpg?t=12345678
+				`${pregQuote(`${basePath}/`)}[a-zA-Z0-9.]+\\?t=[0-9]+`,
+				// Handles file://path/to/abcdefg.jpg
+				`${pregQuote(`${basePath}/`)}[a-zA-Z0-9.]+`,
+			];
+			for (const reString of reStrings) {
+				const re = new RegExp(reString, 'gi');
+				body = body.replace(re, match => {
+					const id = Resource.pathToId(match);
+					return `:/${id}`;
+				});
+			}
 		}
 
 		this.logger().info('replaceResourceExternalToInternalLinks result', body);

ReactNativeClient/lib/models/Resource.js

@@ -1,5 +1,6 @@
 const BaseModel = require('lib/BaseModel.js');
 const BaseItem = require('lib/models/BaseItem.js');
+const ItemChange = require('lib/models/ItemChange.js');
 const NoteResource = require('lib/models/NoteResource.js');
 const ResourceLocalState = require('lib/models/ResourceLocalState.js');
 const Setting = require('lib/models/Setting.js');
@@ -67,6 +68,7 @@ class Resource extends BaseItem {
 		return Resource.fsDriver_;
 	}
 
+	// DEPRECATED IN FAVOUR OF friendlySafeFilename()
 	static friendlyFilename(resource) {
 		let output = safeFilename(resource.title); // Make sure not to allow spaces or any special characters as it's not supported in HTTP headers
 		if (!output) output = resource.id;
@@ -91,6 +93,15 @@ class Resource extends BaseItem {
 		return resource.id + extension;
 	}
 
+	static friendlySafeFilename(resource) {
+		let ext = resource.extension;
+		if (!ext) ext = resource.mime ? mime.toFileExtension(resource.mime) : '';
+		const safeExt = ext ? pathUtils.safeFileExtension(ext).toLowerCase() : '';
+		let title = resource.title ? resource.title : resource.id;
+		if (safeExt && pathUtils.fileExtension(title).toLowerCase() === safeExt) title = pathUtils.filename(title);
+		return pathUtils.friendlySafeFilename(title) + (safeExt ? `.${safeExt}` : '');
+	}
+
 	static relativePath(resource, encryptedBlob = false) {
 		return `${Setting.value('resourceDirName')}/${this.filename(resource, encryptedBlob)}`;
 	}
@@ -100,8 +111,21 @@ class Resource extends BaseItem {
 	}
 
 	static async isReady(resource) {
+		const r = await this.readyStatus(resource);
+		return r === 'ok';
+	}
+
+	static async readyStatus(resource) {
 		const ls = await this.localState(resource);
-		return resource && ls.fetch_status === Resource.FETCH_STATUS_DONE && !resource.encryption_blob_encrypted;
+		if (!resource) return 'notFound';
+		if (ls.fetch_status !== Resource.FETCH_STATUS_DONE) return 'notDownloaded';
+		if (resource.encryption_blob_encrypted) return 'encrypted';
+		return 'ok';
+	}
+
+	static async requireIsReady(resource) {
+		const readyStatus = await Resource.readyStatus(resource);
+		if (readyStatus !== 'ok') throw new Error(`Resource is not ready. Status: ${readyStatus}`);
 	}
 
 	// For resources, we need to decrypt the item (metadata) and the resource binary blob.
@@ -218,7 +242,7 @@ class Resource extends BaseItem {
 		return url.substr(2);
 	}
 
-	static localState(resourceOrId) {
+	static async localState(resourceOrId) {
 		return ResourceLocalState.byResourceId(typeof resourceOrId === 'object' ? resourceOrId.id : resourceOrId);
 	}
 
@@ -297,6 +321,59 @@ class Resource extends BaseItem {
 		throw new Error(`Invalid status: ${status}`);
 	}
 
+	static async updateResourceBlobContent(resourceId, newBlobFilePath) {
+		const resource = await Resource.load(resourceId);
+		await this.requireIsReady(resource);
+
+		const fileStat = await this.fsDriver().stat(newBlobFilePath);
+		await this.fsDriver().copy(newBlobFilePath, Resource.fullPath(resource));
+
+		return await Resource.save({
+			id: resource.id,
+			size: fileStat.size,
+		});
+	}
+
+	static async resourceBlobContent(resourceId, encoding = 'Buffer') {
+		const resource = await Resource.load(resourceId);
+		await this.requireIsReady(resource);
+		return await this.fsDriver().readFile(Resource.fullPath(resource), encoding);
+	}
+
+	static async duplicateResource(resourceId) {
+		const resource = await Resource.load(resourceId);
+		const localState = await Resource.localState(resource);
+
+		let newResource = { ...resource };
+		delete newResource.id;
+		newResource = await Resource.save(newResource);
+
+		const newLocalState = { ...localState };
+		newLocalState.resource_id = newResource.id;
+		delete newLocalState.id;
+
+		await Resource.setLocalState(newResource, newLocalState);
+
+		const sourcePath = Resource.fullPath(resource);
+		if (await this.fsDriver().exists(sourcePath)) {
+			await this.fsDriver().copy(sourcePath, Resource.fullPath(newResource));
+		}
+
+		return newResource;
+	}
+
+	static async createConflictResourceNote(resource) {
+		const Note = this.getClass('Note');
+
+		const conflictResource = await Resource.duplicateResource(resource.id);
+
+		await Note.save({
+			title: _('Attachment conflict: "%s"', resource.title),
+			body: _('There was a [conflict](%s) on the attachment below.\n\n%s', 'https://joplinapp.org/conflict', Resource.markdownTag(conflictResource)),
+			is_conflict: 1,
+		}, { changeSource: ItemChange.SOURCE_SYNC });
+	}
+
 }
 
 Resource.IMAGE_MAX_DIMENSION = 1920;

ReactNativeClient/lib/services/ResourceEditWatcher.ts

@@ -0,0 +1,229 @@
+const { Logger } = require('lib/logger.js');
+const Setting = require('lib/models/Setting');
+const Resource = require('lib/models/Resource');
+const { shim } = require('lib/shim');
+const EventEmitter = require('events');
+const chokidar = require('chokidar');
+const { bridge } = require('electron').remote.require('./bridge');
+const { _ } = require('lib/locale');
+import AsyncActionQueue from '../AsyncActionQueue';
+
+interface WatchedItem {
+	resourceId: string,
+	lastFileUpdatedTime: number,
+	lastResourceUpdatedTime: number,
+	path:string,
+	asyncSaveQueue: AsyncActionQueue,
+}
+
+interface WatchedItems {
+	[key:string]: WatchedItem,
+}
+
+export default class ResourceEditWatcher {
+
+	private static instance_:ResourceEditWatcher;
+
+	private logger_:any;
+	// private dispatch:Function;
+	private watcher_:any;
+	private chokidar_:any;
+	private watchedItems_:WatchedItems = {};
+	private eventEmitter_:any;
+	private tempDir_:string = '';
+
+	constructor() {
+		this.logger_ = new Logger();
+		// this.dispatch = () => {};
+		this.watcher_ = null;
+		this.chokidar_ = chokidar;
+		this.eventEmitter_ = new EventEmitter();
+	}
+
+	initialize(logger:any/* , dispatch:Function*/) {
+		this.logger_ = logger;
+		// this.dispatch = dispatch;
+	}
+
+	static instance() {
+		if (this.instance_) return this.instance_;
+		this.instance_ = new ResourceEditWatcher();
+		return this.instance_;
+	}
+
+	private async tempDir() {
+		if (!this.tempDir_) {
+			this.tempDir_ = `${Setting.value('tempDir')}/edited_resources`;
+			await shim.fsDriver().mkdir(this.tempDir_);
+		}
+
+		return this.tempDir_;
+	}
+
+	logger() {
+		return this.logger_;
+	}
+
+	on(eventName:string, callback:Function) {
+		return this.eventEmitter_.on(eventName, callback);
+	}
+
+	off(eventName:string, callback:Function) {
+		return this.eventEmitter_.removeListener(eventName, callback);
+	}
+
+	private watch(fileToWatch:string) {
+		if (!this.chokidar_) return;
+
+		const makeSaveAction = (resourceId:string, path:string) => {
+			return async () => {
+				this.logger().info(`ResourceEditWatcher: Saving resource ${resourceId}`);
+				const resource = await Resource.load(resourceId);
+				const watchedItem = this.watchedItemByResourceId(resourceId);
+
+				if (resource.updated_time !== watchedItem.lastResourceUpdatedTime) {
+					this.logger().info(`ResourceEditWatcher: Conflict was detected (resource was modified from somewhere else, possibly via sync). Conflict note will be created: ${resourceId}`);
+					// The resource has been modified from elsewhere, for example via sync
+					// so copy the current version to the Conflict notebook, and overwrite
+					// the resource content.
+					await Resource.createConflictResourceNote(resource);
+				}
+
+				const savedResource = await Resource.updateResourceBlobContent(resourceId, path);
+				watchedItem.lastResourceUpdatedTime = savedResource.updated_time;
+				this.eventEmitter_.emit('resourceChange', { id: resourceId });
+			};
+		};
+
+		if (!this.watcher_) {
+			this.watcher_ = this.chokidar_.watch(fileToWatch);
+			this.watcher_.on('all', async (event:any, path:string) => {
+				this.logger().info(`ResourceEditWatcher: Event: ${event}: ${path}`);
+
+				if (event === 'unlink') {
+					// File are unwatched in the stopWatching functions below. When we receive an unlink event
+					// here it might be that the file is quickly moved to a different location and replaced by
+					// another file with the same name, as it happens with emacs. So because of this
+					// we keep watching anyway.
+					// See: https://github.com/laurent22/joplin/issues/710#issuecomment-420997167
+					// this.watcher_.unwatch(path);
+				} else if (event === 'change') {
+					const watchedItem = this.watchedItemByPath(path);
+					const resourceId = watchedItem.resourceId;
+
+					if (!watchedItem) {
+						this.logger().error(`ResourceEditWatcher: could not find resource ID from path: ${path}`);
+						return;
+					}
+
+					const stat = await shim.fsDriver().stat(path);
+					const editedFileUpdatedTime = stat.mtime.getTime();
+
+					if (watchedItem.lastFileUpdatedTime === editedFileUpdatedTime) {
+						// chokidar is buggy and emits "change" events even when nothing has changed
+						// so double-check the modified time and skip processing if there's no change.
+						// In particular it emits two such events just after the file has been copied
+						// in openAndWatch().
+						this.logger().debug(`ResourceEditWatcher: No timestamp change - skip: ${resourceId}`);
+						return;
+					}
+
+					this.logger().debug(`ResourceEditWatcher: Queuing save action: ${resourceId}`);
+
+					watchedItem.asyncSaveQueue.push(makeSaveAction(resourceId, path));
+					watchedItem.lastFileUpdatedTime = editedFileUpdatedTime;
+				} else if (event === 'error') {
+					this.logger().error('ResourceEditWatcher: error');
+				}
+			});
+			// Hack to support external watcher on some linux applications (gedit, gvim, etc)
+			// taken from https://github.com/paulmillr/chokidar/issues/591
+			// @ts-ignore Leave unused path variable
+			this.watcher_.on('raw', async (event:string, path:string, options:any) => {
+				if (event === 'rename') {
+					this.watcher_.unwatch(options.watchedPath);
+					this.watcher_.add(options.watchedPath);
+				}
+			});
+		} else {
+			this.watcher_.add(fileToWatch);
+		}
+
+		return this.watcher_;
+	}
+
+	public async openAndWatch(resourceId:string) {
+		let watchedItem = this.watchedItemByResourceId(resourceId);
+
+		if (!watchedItem) {
+			// Immediately create and push the item to prevent race conditions
+
+			watchedItem = {
+				resourceId: resourceId,
+				lastFileUpdatedTime: 0,
+				lastResourceUpdatedTime: 0,
+				asyncSaveQueue: new AsyncActionQueue(1000),
+				path: '',
+			};
+
+			this.watchedItems_[resourceId] = watchedItem;
+
+			const resource = await Resource.load(resourceId);
+			if (!(await Resource.isReady(resource))) throw new Error(_('This attachment is not downloaded or not decrypted yet'));
+			const sourceFilePath = Resource.fullPath(resource);
+			const tempDir = await this.tempDir();
+			const editFilePath = await shim.fsDriver().findUniqueFilename(`${tempDir}/${Resource.friendlySafeFilename(resource)}`);
+			await shim.fsDriver().copy(sourceFilePath, editFilePath);
+			const stat = await shim.fsDriver().stat(editFilePath);
+
+			watchedItem.path = editFilePath;
+			watchedItem.lastFileUpdatedTime = stat.mtime.getTime();
+			watchedItem.lastResourceUpdatedTime = resource.updated_time;
+
+			this.watch(editFilePath);
+		}
+
+		bridge().openItem(watchedItem.path);
+
+		this.logger().info(`ResourceEditWatcher: Started watching ${watchedItem.path}`);
+	}
+
+	async stopWatching(resourceId:string) {
+		if (!resourceId) return;
+
+		const item = this.watchedItemByResourceId(resourceId);
+		if (!item) {
+			this.logger().error(`ResourceEditWatcher: Trying to stop watching non-watched resource ${resourceId}`);
+			return;
+		}
+
+		await item.asyncSaveQueue.waitForAllDone();
+
+		if (this.watcher_) this.watcher_.unwatch(item.path);
+		await shim.fsDriver().remove(item.path);
+		delete this.watchedItems_[resourceId];
+		this.logger().info(`ResourceEditWatcher: Stopped watching ${item.path}`);
+	}
+
+	public async stopWatchingAll() {
+		const promises = [];
+		for (const resourceId in this.watchedItems_) {
+			const item = this.watchedItems_[resourceId];
+			promises.push(this.stopWatching(item.resourceId));
+		}
+		return Promise.all(promises);
+	}
+
+	private watchedItemByResourceId(resourceId:string):WatchedItem {
+		return this.watchedItems_[resourceId];
+	}
+
+	private watchedItemByPath(path:string):WatchedItem {
+		for (const resourceId in this.watchedItems_) {
+			const item = this.watchedItems_[resourceId];
+			if (item.path === path) return item;
+		}
+		return null;
+	}
+
+}

ReactNativeClient/lib/shim-init-node.js

@@ -144,7 +144,7 @@ function shimInit() {
 
 	shim.createResourceFromPath = async function(filePath, defaultProps = null, options = null) {
 		options = Object.assign({
-			resizeLargeImages: 'always', // 'always' or 'ask'
+			resizeLargeImages: 'always', // 'always', 'ask' or 'never'
 		}, options);
 
 		const readChunk = require('read-chunk');
@@ -182,7 +182,7 @@ function shimInit() {
 
 		const targetPath = Resource.fullPath(resource);
 
-		if (['image/jpeg', 'image/jpg', 'image/png'].includes(resource.mime)) {
+		if (options.resizeLargeImages !== 'never' && ['image/jpeg', 'image/jpg', 'image/png'].includes(resource.mime)) {
 			const ok = await handleResizeImage_(filePath, targetPath, resource.mime, options.resizeLargeImages);
 			if (!ok) return null;
 		} else {

ReactNativeClient/lib/synchronizer.js

@@ -372,6 +372,12 @@ class Synchronizer {
 						let reason = '';
 						let remoteContent = null;
 
+						const getConflictType = (conflictedItem) => {
+							if (conflictedItem.type_ === BaseModel.TYPE_NOTE) return 'noteConflict';
+							if (conflictedItem.type_ === BaseModel.TYPE_RESOURCE) return 'resourceConflict';
+							return 'itemConflict';
+						};
+
 						if (!remote) {
 							if (!local.sync_time) {
 								action = 'createRemote';
@@ -379,7 +385,7 @@ class Synchronizer {
 							} else {
 								// Note or item was modified after having been deleted remotely
 								// "itemConflict" is for all the items except the notes, which are dealt with in a special way
-								action = local.type_ == BaseModel.TYPE_NOTE ? 'noteConflict' : 'itemConflict';
+								action = getConflictType(local);
 								reason = 'remote has been deleted, but local has changes';
 							}
 						} else {
@@ -416,7 +422,7 @@ class Synchronizer {
 								// Since, in this loop, we are only dealing with items that require sync, if the
 								// remote has been modified after the sync time, it means both items have been
 								// modified and so there's a conflict.
-								action = local.type_ == BaseModel.TYPE_NOTE ? 'noteConflict' : 'itemConflict';
+								action = getConflictType(local);
 								reason = 'both remote and local have changes';
 							} else {
 								action = 'updateRemote';
@@ -528,8 +534,25 @@ class Synchronizer {
 								conflictedNote.is_conflict = 1;
 								await Note.save(conflictedNote, { autoTimestamp: false, changeSource: ItemChange.SOURCE_SYNC });
 							}
-
+						} else if (action == 'resourceConflict') {
 							// ------------------------------------------------------------------------------
+							// Unlike notes we always handle the conflict for resources
+							// ------------------------------------------------------------------------------
+
+							await Resource.createConflictResourceNote(local);
+
+							if (remote) {
+								// The local content we have is no longer valid and should be re-downloaded
+								await Resource.setLocalState(local.id, {
+									fetch_status: Resource.FETCH_STATUS_IDLE,
+								});
+							}
+						}
+
+						if (['noteConflict', 'resourceConflict'].includes(action)) {
+							// ------------------------------------------------------------------------------
+							// For note and resource conflicts, the creation of the conflict item is done
+							// differently. However the way the local content is handled is the same.
 							// Either copy the remote content to local or, if the remote content has
 							// been deleted, delete the local content.
 							// ------------------------------------------------------------------------------
@@ -718,9 +741,9 @@ class Synchronizer {
 							if (action == 'createLocal') options.isNew = true;
 							if (action == 'updateLocal') options.oldItem = local;
 
-							const creatingNewResource = content.type_ == BaseModel.TYPE_RESOURCE && action == 'createLocal';
+							const creatingOrUpdatingResource = content.type_ == BaseModel.TYPE_RESOURCE && (action == 'createLocal' || action == 'updateLocal');
 
-							if (creatingNewResource) {
+							if (creatingOrUpdatingResource) {
 								if (content.size >= this.maxResourceSize()) {
 									await handleCannotSyncItem(ItemClass, syncTargetId, content, `File "${content.title}" is larger than allowed ${this.maxResourceSize()} bytes. Beyond this limit, the mobile app would crash.`, BaseItem.SYNC_ITEM_LOCATION_REMOTE);
 									continue;
@@ -731,7 +754,7 @@ class Synchronizer {
 
 							await ItemClass.save(content, options);
 
-							if (creatingNewResource) this.dispatch({ type: 'SYNC_CREATED_RESOURCE', id: content.id });
+							if (creatingOrUpdatingResource) this.dispatch({ type: 'SYNC_CREATED_OR_UPDATED_RESOURCE', id: content.id });
 
 							if (!hasAutoEnabledEncryption && content.type_ === BaseModel.TYPE_MASTER_KEY && !masterKeysBefore) {
 								hasAutoEnabledEncryption = true;

ReactNativeClient/root.js

@@ -151,7 +151,7 @@ const generalMiddleware = store => next => async (action) => {
 		DecryptionWorker.instance().scheduleStart();
 	}
 
-	if (action.type === 'SYNC_CREATED_RESOURCE') {
+	if (action.type === 'SYNC_CREATED_OR_UPDATED_RESOURCE') {
 		ResourceFetcher.instance().autoAddResources();
 	}