Skip to content

Fix console --force overwrite with --split option #212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
willibrandon wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Fix console --force overwrite with --split option #212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c7a2cca
Add a test that demonstrates the issue
willibrandon Oct 23, 2023
fb1a110
Utilize the force overwrite option during placeholder substitution
willibrandon Oct 23, 2023
5b8c6b5
Add the NoOverwrite test
willibrandon Oct 23, 2023
4d65b4b
Remove confirm overwrite from PDF save
willibrandon Oct 24, 2023
72ab085
Add WithPlaceholder tests
willibrandon Oct 24, 2023
29ab522
Add incrementPlaceholderIfExists substitution parameter
willibrandon Oct 24, 2023
5344dc5
Include the original todo message
willibrandon Oct 24, 2023
2b08aae
Fix original spacing
willibrandon Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 144 additions & 0 deletions NAPS2.Lib.Tests/Automation/CommandLineIntegrationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,150 @@ await _automationHelper.RunCommand(
AssertRecoveryCleanedUp();
}

[Fact]
public async Task ExistingFile_NoOverwrite_SplitWithPlaceholder()
{
var path = $"{FolderPath}/test$(n).pdf";
await _automationHelper.RunCommand(
new AutomatedScanningOptions
{
OutputPath = path,
ProfileName = string.Empty,
Split = true,
Verbose = true
},
new[] { Image1, Image2, Image3 });
await _automationHelper.WithContainer(container =>
{
var profileManager = container.Resolve<IProfileManager>();
profileManager.Profiles.Remove(profileManager.Profiles.Where(p => p.IsDefault).First());
}).RunCommand(
new AutomatedScanningOptions
{
OutputPath = path,
ProfileName = string.Empty,
Split = true,
Verbose = true
},
new[] { Image1, Image2, Image3 });
PdfAsserts.AssertImages($"{FolderPath}/test1.pdf", Image1);
PdfAsserts.AssertImages($"{FolderPath}/test2.pdf", Image2);
PdfAsserts.AssertImages($"{FolderPath}/test3.pdf", Image3);
Assert.True(File.Exists($"{FolderPath}/test4.pdf"));
Assert.True(File.Exists($"{FolderPath}/test5.pdf"));
Assert.True(File.Exists($"{FolderPath}/test6.pdf"));
AssertRecoveryCleanedUp();
}

[Fact]
public async Task ExistingFile_ForceOverwrite_SplitWithPlaceholder()
{
var path = $"{FolderPath}/test$(n).pdf";
await _automationHelper.RunCommand(
new AutomatedScanningOptions
{
OutputPath = path,
ForceOverwrite = true,
ProfileName = string.Empty,
Split = true,
Verbose = true
},
new[] { Image1, Image2, Image3 });
await _automationHelper.WithContainer(container =>
{
var profileManager = container.Resolve<IProfileManager>();
profileManager.Profiles.Remove(profileManager.Profiles.Where(p => p.IsDefault).First());
}).RunCommand(
new AutomatedScanningOptions
{
OutputPath = path,
ForceOverwrite = true,
ProfileName = string.Empty,
Split = true,
Verbose = true
},
new[] { Image1, Image2, Image3 });
PdfAsserts.AssertImages($"{FolderPath}/test1.pdf", Image1);
PdfAsserts.AssertImages($"{FolderPath}/test2.pdf", Image2);
PdfAsserts.AssertImages($"{FolderPath}/test3.pdf", Image3);
Assert.False(File.Exists($"{FolderPath}/test4.pdf"));
Assert.False(File.Exists($"{FolderPath}/test5.pdf"));
Assert.False(File.Exists($"{FolderPath}/test6.pdf"));
AssertRecoveryCleanedUp();
}

[Fact]
public async Task ExistingFile_NoOverwrite_SplitWithNoPlaceholder()
{
var path = $"{FolderPath}/test.pdf";
await _automationHelper.RunCommand(
new AutomatedScanningOptions
{
OutputPath = path,
ProfileName = string.Empty,
Split = true,
Verbose = true
},
new[] { Image1, Image2, Image3 });
await _automationHelper.WithContainer(container =>
{
var profileManager = container.Resolve<IProfileManager>();
profileManager.Profiles.Remove(profileManager.Profiles.Where(p => p.IsDefault).First());
}).RunCommand(
new AutomatedScanningOptions
{
OutputPath = path,
ProfileName = string.Empty,
Split = true,
Verbose = true
},
new[] { Image1, Image2, Image3 });
PdfAsserts.AssertPageCount(1, $"{FolderPath}/test.1.pdf");
PdfAsserts.AssertPageCount(1, $"{FolderPath}/test.2.pdf");
PdfAsserts.AssertPageCount(1, $"{FolderPath}/test.3.pdf");
Assert.True(File.Exists($"{FolderPath}/test.4.pdf"));
Assert.True(File.Exists($"{FolderPath}/test.5.pdf"));
Assert.True(File.Exists($"{FolderPath}/test.6.pdf"));
AssertRecoveryCleanedUp();
}

[Fact]
public async Task ExistingFile_ForceOverwrite_SplitWithNoPlaceholder()
{
var path = $"{FolderPath}/test.pdf";
await _automationHelper.RunCommand(
new AutomatedScanningOptions
{
OutputPath = path,
ForceOverwrite = true,
ProfileName = string.Empty,
Split = true,
Verbose = true
},
new[] { Image1, Image2, Image3 });
await _automationHelper.WithContainer(container =>
{
var profileManager = container.Resolve<IProfileManager>();
profileManager.Profiles.Remove(profileManager.Profiles.Where(p => p.IsDefault).First());
}).RunCommand(
new AutomatedScanningOptions
{
OutputPath = path,
ForceOverwrite = true,
ProfileName = string.Empty,
Split = true,
Verbose = true
},
new[] { Image1, Image2, Image3 });
PdfAsserts.AssertPageCount(1, $"{FolderPath}/test.1.pdf");
PdfAsserts.AssertPageCount(1, $"{FolderPath}/test.2.pdf");
PdfAsserts.AssertPageCount(1, $"{FolderPath}/test.3.pdf");
Assert.False(File.Exists($"{FolderPath}/test.4.pdf"));
Assert.False(File.Exists($"{FolderPath}/test.5.pdf"));
Assert.False(File.Exists($"{FolderPath}/test.6.pdf"));
AssertRecoveryCleanedUp();
}

[Fact]
public async Task MultipleImages()
{
Expand Down
3 changes: 2 additions & 1 deletion NAPS2.Lib/Automation/AutomatedScanning.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,8 @@ private async Task<bool> DoExportToPdf(string path, bool email)
}
};
int digits = (int) Math.Floor(Math.Log10(_scanList.Count)) + 1;
string actualPath = _placeholders.Substitute(path, true, scanIndex++, _scanList.Count > 1 ? digits : 0);
string actualPath = _placeholders.Substitute(path, !_options.ForceOverwrite,
scanIndex++, _scanList.Count > 1 ? digits : 0, !_options.ForceOverwrite);
op.Start(actualPath, _placeholders, fileContents, _config.Get(c => c.PdfSettings), _ocrParams);
if (!await op.Success)
{
Expand Down
4 changes: 2 additions & 2 deletions NAPS2.Lib/Pdf/SavePdfOperation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ public bool Start(string fileName, Placeholders placeholders, ICollection<Proces
int i = 0;
foreach (var imagesForFile in imagesByFile)
{
var currentFileName = placeholders.Substitute(fileName, true, i, singleFile ? 0 : digits);
// TODO: Overwrite prompt non-single file?
var currentFileName = placeholders.Substitute(fileName, true, i, singleFile ? 0 : digits);
// TODO: Overwrite prompt non-single file?
Status.StatusText = string.Format(MiscResources.SavingFormat, Path.GetFileName(currentFileName));
InvokeStatusChanged();
if (singleFile && IsFileInUse(currentFileName, out var ex))
Expand Down
11 changes: 6 additions & 5 deletions NAPS2.Sdk/ImportExport/Placeholders.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,22 @@ internal abstract class Placeholders
/// <param name="incrementIfExists">Whether to use an auto-incrementing file number to make the file name unique.</param>
/// <param name="numberSkip">The file number will be at least one bigger than this value.</param>
/// <param name="autoNumberDigits">The minimum number of digits in the file number. Only has an effect if the path does not contain a numeric placeholder like $(n) or $(nnn).</param>
/// <param name="incrementPlaceholderIfExists">Whether to increment the placeholder number to make the file name unique.</param>
/// <returns>The file path with substitutions.</returns>
[return: NotNullIfNotNull("filePath")]
public abstract string? Substitute(string? filePath, bool incrementIfExists = true, int numberSkip = 0,
int autoNumberDigits = 0);
int autoNumberDigits = 0, bool incrementPlaceholderIfExists = true);
Copy link
Owner

@cyanfish cyanfish Nov 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not make this logic more complicated - I think it should be possible to re-use the existing incrementIfExists parameter, we just need to double check the callers and make sure everything is going to behave as expected.


public class StubPlaceholders : Placeholders
{
public override string? Substitute(string? filePath, bool incrementIfExists = true, int numberSkip = 0,
int autoNumberDigits = 0) => filePath;
int autoNumberDigits = 0, bool incrementPlaceholderIfExists = true) => filePath;
}

public class EnvironmentPlaceholders : Placeholders
{
public override string? Substitute(string? filePath, bool incrementIfExists = true, int numberSkip = 0,
int autoNumberDigits = 0)
int autoNumberDigits = 0, bool incrementPlaceholderIfExists = true)
{
if (filePath == null) return null;
return Environment.ExpandEnvironmentVariables(filePath);
Expand Down Expand Up @@ -99,7 +100,7 @@ public DefaultPlaceholders(DateTime? dateTimeOverride = null)

[return: NotNullIfNotNull("filePath")]
public override string? Substitute(string? filePath, bool incrementIfExists = true, int numberSkip = 0,
int autoNumberDigits = 0)
int autoNumberDigits = 0, bool incrementPlaceholderIfExists = true)
{
if (filePath == null)
{
Expand All @@ -116,7 +117,7 @@ public DefaultPlaceholders(DateTime? dateTimeOverride = null)
if (match.Success)
{
result = NumberPlaceholderPattern.Replace(result, "");
result = SubstituteNumber(result, match.Index, match.Length - 3, numberSkip, true);
result = SubstituteNumber(result, match.Index, match.Length - 3, numberSkip, incrementPlaceholderIfExists);
}
else if (autoNumberDigits > 0)
{
Expand Down