Skip to content

Commit

Permalink
Fix "Rename File from Parent Metadata" if target filename exists
Browse files Browse the repository at this point in the history 
Add a unique numeric suffix to the filename, before any extension
  • Loading branch information
@dstillman
dstillman committed Nov 1, 2017
1 parent c784db8 commit 0f743e5
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 14 deletions.
66 changes: 53 additions & 13 deletions chrome/content/zotero/xpcom/data/item.js
View file
Expand Up @@ -2416,14 +2416,18 @@ Zotero.Item.prototype.fileExistsCached = function () {



/*
/**
* Rename file associated with an attachment
*
* -1 Destination file exists -- use _force_ to overwrite
* -2 Error renaming
* false Attachment file not found
* @param {String} newName
* @param {Boolean} [overwrite=false] - Overwrite file if one exists
* @param {Boolean} [unique=false] - Add suffix to create unique filename if necessary
* @return {Number|false} -- true - Rename successful
* -1 - Destination file exists; use _force_ to overwrite
* -2 - Error renaming
* false - Attachment file not found
*/
Zotero.Item.prototype.renameAttachmentFile = Zotero.Promise.coroutine(function* (newName, overwrite) {
Zotero.Item.prototype.renameAttachmentFile = Zotero.Promise.coroutine(function* (newName, overwrite=false, unique=false) {
var origPath = yield this.getFilePathAsync();
if (!origPath) {
Zotero.debug("Attachment file not found in renameAttachmentFile()", 2);
Expand All @@ -2442,21 +2446,57 @@ Zotero.Item.prototype.renameAttachmentFile = Zotero.Promise.coroutine(function*
return true;
}

var destPath = OS.Path.join(OS.Path.dirname(origPath), newName);
var parentDir = OS.Path.dirname(origPath);
var destPath = OS.Path.join(parentDir, newName);
var destName = OS.Path.basename(destPath);
// Get root + extension, if there is one
var pos = destName.lastIndexOf('.');
if (pos > 0) {
var root = destName.substr(0, pos);
var ext = destName.substr(pos + 1);
}
else {
var root = destName;
}

// Update mod time and clear hash so the file syncs
// TODO: use an integer counter instead of mod time for change detection
// Update mod time first, because it may fail for read-only files on Windows
yield OS.File.setDates(origPath, null, null);
var result = yield OS.File.move(origPath, destPath, { noOverwrite: !overwrite })
// If no overwriting and file exists, return -1
.catch(OS.File.Error, function (e) {
if (e.becauseExists) {
return -1;
var result;
var incr = 0;
while (true) {
// If filename already exists, add a numeric suffix to the end of the root, before
// the extension if there is one
if (incr) {
if (ext) {
destName = root + ' ' + (incr + 1) + '.' + ext;
}
else {
destName = root + ' ' + (incr + 1);
}
destPath = OS.Path.join(parentDir, destName);
}
throw e;
});

try {
result = yield OS.File.move(origPath, destPath, { noOverwrite: !overwrite })
}
catch (e) {
if (e instanceof OS.File.Error) {
if (e.becauseExists) {
// Increment number to create unique suffix
if (unique) {
incr++;
continue;
}
// If no overwriting or making unique and file exists, return -1
return -1;
}
}
throw e;
}
break;
}
if (result) {
return result;
}
Expand Down
2 changes: 1 addition & 1 deletion chrome/content/zotero/zoteroPane.js
View file
Expand Up @@ -4541,7 +4541,7 @@ var ZoteroPane = new function()
newName = newName + ext;
}

var renamed = yield item.renameAttachmentFile(newName);
var renamed = yield item.renameAttachmentFile(newName, false, true);
if (renamed !== true) {
Zotero.debug("Could not rename file (" + renamed + ")");
continue;
Expand Down
87 changes: 87 additions & 0 deletions test/tests/zoteroPaneTest.js
View file
Expand Up @@ -244,6 +244,93 @@ describe("ZoteroPane", function() {
})


describe("#renameSelectedAttachmentsFromParents()", function () {
it("should rename a linked file", async function () {
var oldFilename = 'old.png';
var newFilename = 'Test.png';
var file = getTestDataDirectory();
file.append('test.png');
var tmpDir = await getTempDirectory();
var oldFile = OS.Path.join(tmpDir, oldFilename);
await OS.File.copy(file.path, oldFile);

var item = createUnsavedDataObject('item');
item.setField('title', 'Test');
await item.saveTx();

var attachment = await Zotero.Attachments.linkFromFile({
file: oldFile,
parentItemID: item.id
});
await zp.selectItem(attachment.id);

await assert.eventually.isTrue(zp.renameSelectedAttachmentsFromParents());
assert.equal(attachment.attachmentFilename, newFilename);
var path = await attachment.getFilePathAsync();
assert.equal(OS.Path.basename(path), newFilename)
await OS.File.exists(path);
});

it("should use unique name for linked file if target name is taken", async function () {
var oldFilename = 'old.png';
var newFilename = 'Test.png';
var uniqueFilename = 'Test 2.png';
var file = getTestDataDirectory();
file.append('test.png');
var tmpDir = await getTempDirectory();
var oldFile = OS.Path.join(tmpDir, oldFilename);
await OS.File.copy(file.path, oldFile);
// Create file with target filename
await Zotero.File.putContentsAsync(OS.Path.join(tmpDir, newFilename), '');

var item = createUnsavedDataObject('item');
item.setField('title', 'Test');
await item.saveTx();

var attachment = await Zotero.Attachments.linkFromFile({
file: oldFile,
parentItemID: item.id
});
await zp.selectItem(attachment.id);

await assert.eventually.isTrue(zp.renameSelectedAttachmentsFromParents());
assert.equal(attachment.attachmentFilename, uniqueFilename);
var path = await attachment.getFilePathAsync();
assert.equal(OS.Path.basename(path), uniqueFilename)
await OS.File.exists(path);
});

it("should use unique name for linked file without extension if target name is taken", async function () {
var oldFilename = 'old';
var newFilename = 'Test';
var uniqueFilename = 'Test 2';
var file = getTestDataDirectory();
file.append('test.png');
var tmpDir = await getTempDirectory();
var oldFile = OS.Path.join(tmpDir, oldFilename);
await OS.File.copy(file.path, oldFile);
// Create file with target filename
await Zotero.File.putContentsAsync(OS.Path.join(tmpDir, newFilename), '');

var item = createUnsavedDataObject('item');
item.setField('title', 'Test');
await item.saveTx();

var attachment = await Zotero.Attachments.linkFromFile({
file: oldFile,
parentItemID: item.id
});
await zp.selectItem(attachment.id);

await assert.eventually.isTrue(zp.renameSelectedAttachmentsFromParents());
assert.equal(attachment.attachmentFilename, uniqueFilename);
var path = await attachment.getFilePathAsync();
assert.equal(OS.Path.basename(path), uniqueFilename)
await OS.File.exists(path);
});
});


describe("#duplicateSelectedItem()", function () {
it("should add reverse relations", async function () {
await selectLibrary(win);
Expand Down

0 comments on commit 0f743e5

Please sign in to comment.