Skip to content

Fix script output#6450

Merged
solth merged 8 commits intokitodo:mainfrom
BartChris:fix_script_output
Apr 3, 2025
Merged

Fix script output#6450
solth merged 8 commits intokitodo:mainfrom
BartChris:fix_script_output

Conversation

@BartChris
Copy link
Copy Markdown
Collaborator

@BartChris BartChris commented Mar 4, 2025

Fixes #6449

This Pull Request tries to apply a minimal fix for the specific problem in #6449, by printing the sucess messages line by line to the task screen on script success.
The user sees only the success or the error messages. Error messages are joined with " | ". The application log tracks all messages combined without specific order.
The printed messages should not take up too much space since the notification area is limited in size and scrollable.

image

All further ideas as discssed in #6461 are not considered here.

@BartChris BartChris force-pushed the fix_script_output branch 3 times, most recently from 4839880 to c2579eb Compare March 10, 2025 19:18
@BartChris BartChris marked this pull request as ready for review March 11, 2025 17:17
@solth solth requested a review from oliver-stoehr March 24, 2025 14:40
@solth
Copy link
Copy Markdown
Member

solth commented Mar 30, 2025

@lutzhelm does this fix resolve the issue you reported (#6449) sufficiently for you?

Copy link
Copy Markdown
Collaborator

@oliver-stoehr oliver-stoehr left a comment

Choose a reason for hiding this comment

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

Code changes are fine and the script output is displayed as intended.
Is there a reason why error messages are joined in one line, but normal messages are printed on separate lines?

@lutzhelm
Copy link
Copy Markdown

lutzhelm commented Apr 1, 2025

@lutzhelm does this fix resolve the issue you reported (#6449) sufficiently for you?

I can't answer this question conclusively as I ran into several problems trying to follow the demo setup.

The message itself looks good. I'd say this solution meets our use case if the message text can be marked and copied.

@BartChris BartChris force-pushed the fix_script_output branch from a7f04c9 to 9f13d45 Compare April 2, 2025 09:38
@BartChris BartChris marked this pull request as draft April 2, 2025 09:39
@BartChris
Copy link
Copy Markdown
Collaborator Author

Code changes are fine and the script output is displayed as intended. Is there a reason why error messages are joined in one line, but normal messages are printed on separate lines?

Yes, there is a reason. I tried to refactor. But the refactoring included that i handle the exceptions in the TaskService which has access to the UI-logging Helper. But that involved not to throw the Exception in the CommandService.
This breaks the Test Suite. I will revert the changes back, because i fear to break the handling of commands in the Workflow Service.
If we still want to throw the error but print out errors line by line we could e.g. introduce a Custom Exception class which can handle list of errors. I am not sure if this is worth the hassle.

@BartChris BartChris marked this pull request as ready for review April 2, 2025 10:08
@BartChris
Copy link
Copy Markdown
Collaborator Author

@lutzhelm does this fix resolve the issue you reported (#6449) sufficiently for you?

I can't answer this question conclusively as I ran into several problems trying to follow the demo setup.

The message itself looks good. I'd say this solution meets our use case if the message text can be marked and copied.

Text can be marked and copied.

@BartChris BartChris marked this pull request as draft April 2, 2025 10:15
@BartChris BartChris marked this pull request as ready for review April 2, 2025 10:27
@BartChris
Copy link
Copy Markdown
Collaborator Author

@oliver-stoehr - I now print the error messsages line by line. This can lead to duplications in the UI, because we are not differentiating between CommandExceptions and real IOExceptions. But maybe is it not too bad.
image

Comment thread Kitodo/src/main/java/org/kitodo/production/services/command/CommandService.java Outdated
Copy link
Copy Markdown
Collaborator

@oliver-stoehr oliver-stoehr left a comment

Choose a reason for hiding this comment

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

I think the error messages are more readable this way. 👍

@solth solth merged commit b77a41d into kitodo:main Apr 3, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display output of non automatic script tasks to the assigned user

5 participants