Skip to content

Feat: Implement hierarchical split functionality for PDF bookmarks #799

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
Chapoly1305 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feat: Implement hierarchical split functionality for PDF bookmarks #799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d253ded
Implement hierarchical split functionality for PDF bookmarks with ove…
Chapoly1305 Nov 13, 2025
a26c1df
Merge branch 'torakiki:master' into pdfsplit-tree
Chapoly1305 Mar 12, 2026
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
47 changes: 46 additions & 1 deletion pdfsam-service/src/main/java/org/pdfsam/service/task/TaskExecutionController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
import org.sejda.model.notification.event.TaskExecutionCompletedEvent;
import org.sejda.model.notification.event.TaskExecutionFailedEvent;
import org.sejda.model.notification.event.TaskExecutionStartedEvent;
import org.sejda.model.parameter.SplitByOutlineLevelParameters;
import org.sejda.model.parameter.base.TaskParameters;
import org.sejda.model.task.NotifiableTaskMetadata;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -85,10 +88,52 @@ public void request(TaskExecutionRequest event) {
LOG.trace("Task execution request received");
usageService.incrementUsageFor(event.toolId());
currentModule = event.toolId();
executor.execute(() -> executionService.execute(event.parameters()));

TaskParameters params = event.parameters();

// Check if this is a hierarchical split request
if (isHierarchicalSplitRequest(params)) {
LOG.debug("Executing hierarchical split task");
executor.execute(() -> executeHierarchicalSplit(params));
} else {
executor.execute(() -> executionService.execute(params));
}
LOG.trace("Task execution submitted");
}

private boolean isHierarchicalSplitRequest(TaskParameters params) {
// Check if this is a hierarchical split request by checking the class name
// We use the class name to avoid a circular dependency on the tool module
return params.getClass().getSimpleName().equals("HierarchicalSplitByOutlineLevelParameters");
Comment on lines +105 to +107
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

isHierarchicalSplitRequest matches on getSimpleName(). This is brittle (a different parameters class in another package with the same simple name would be treated as hierarchical) and makes refactors risky. Prefer matching on the fully qualified class name (e.g., getName()) or a stable marker exposed by the parameters type.

Suggested change
// Check if this is a hierarchical split request by checking the class name
// We use the class name to avoid a circular dependency on the tool module
return params.getClass().getSimpleName().equals("HierarchicalSplitByOutlineLevelParameters");
// Check if this is a hierarchical split request by checking the fully qualified class name
// We use the class name to avoid a circular dependency on the tool module
return params.getClass().getName()
.equals("org.pdfsam.tools.splitbybookmarks.HierarchicalSplitByOutlineLevelParameters");

Copilot uses AI. Check for mistakes.
}

private void executeHierarchicalSplit(TaskParameters params) {
if (!(params instanceof SplitByOutlineLevelParameters)) {
LOG.error("Invalid parameters type for hierarchical split");
GlobalNotificationContext.getContext()
.notifyListeners(new TaskExecutionFailedEvent(
new IllegalArgumentException("Invalid parameters type for hierarchical split"),
NotifiableTaskMetadata.NULL));
return;
}

try {
// Use reflection to instantiate and execute the hierarchical task
Class<?> taskClass = Class.forName(
"org.pdfsam.tools.splitbybookmarks.HierarchicalSplitByBookmarksTask");
Object task = taskClass.getDeclaredConstructor().newInstance();

// Execute the task
java.lang.reflect.Method executeMethod = taskClass.getMethod("execute",
SplitByOutlineLevelParameters.class);
executeMethod.invoke(task, params);
} catch (Exception e) {
LOG.error("Failed to execute hierarchical split task", e);
GlobalNotificationContext.getContext()
.notifyListeners(new TaskExecutionFailedEvent(e, NotifiableTaskMetadata.NULL));
Comment on lines +121 to +133
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

executeHierarchicalSplit invokes the task via reflection but only calls execute(...). Since HierarchicalSplitByBookmarksTask relies on BaseTask.before(...) to compile the regex (titleMatchingPattern) and on after() to close the opened documentHandler, skipping the lifecycle means: regex filtering won’t work and the input document may never be closed (resource leak). Also, failures will likely emit duplicate TaskExecutionFailedEvents because the task already notifies failures and the controller notifies again when the reflective call throws. Please route hierarchical execution through the standard TaskExecutionService (preferred) or, if reflection is unavoidable, explicitly call before(...) and after() in a finally block and avoid double-notifying on failures (unwrap InvocationTargetException).

Suggested change
// Use reflection to instantiate and execute the hierarchical task
Class<?> taskClass = Class.forName(
"org.pdfsam.tools.splitbybookmarks.HierarchicalSplitByBookmarksTask");
Object task = taskClass.getDeclaredConstructor().newInstance();
// Execute the task
java.lang.reflect.Method executeMethod = taskClass.getMethod("execute",
SplitByOutlineLevelParameters.class);
executeMethod.invoke(task, params);
} catch (Exception e) {
LOG.error("Failed to execute hierarchical split task", e);
GlobalNotificationContext.getContext()
.notifyListeners(new TaskExecutionFailedEvent(e, NotifiableTaskMetadata.NULL));
// Route hierarchical split through the standard TaskExecutionService so that
// task lifecycle (before/after) and notifications are handled correctly.
executionService.execute(params);
} catch (Exception e) {
// Let TaskExecutionService / task infrastructure handle failure notifications
// to avoid duplicate TaskExecutionFailedEvent emissions.
LOG.error("Failed to execute hierarchical split task", e);

Copilot uses AI. Check for mistakes.
}
}

@EventListener
public void onShutdown(ShutdownEvent event) {
executor.shutdownNow();
Expand Down
12 changes: 12 additions & 0 deletions pdfsam-tools/pdfsam-split-by-bookmarks/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,18 @@
<groupId>org.sejda</groupId>
<artifactId>sejda-model</artifactId>
</dependency>
<dependency>
<groupId>org.sejda</groupId>
<artifactId>sejda-core</artifactId>
</dependency>
<dependency>
<groupId>org.sejda</groupId>
<artifactId>sejda-commons</artifactId>
</dependency>
<dependency>
<groupId>org.sejda</groupId>
<artifactId>sejda-sambox</artifactId>
</dependency>
<dependency>
<groupId>org.pdfsam</groupId>
<artifactId>pdfsam-ui-components</artifactId>
Expand Down
6 changes: 6 additions & 0 deletions pdfsam-tools/pdfsam-split-by-bookmarks/src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@
requires org.pdfsam.injector;
requires org.kordamp.ikonli.javafx;
requires org.kordamp.ikonli.unicons;
requires org.sejda.model;
requires org.sejda.commons;
requires org.sejda.core;
requires org.sejda.impl.sambox;
requires org.sejda.sambox;
requires org.slf4j;

provides Tool with SplitByBookmarksTool;
}
Loading
Loading