-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-6214] Fix new web app to handling carriage return #4986
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'm no frontend developer. More opinions are welcome.
Thanks for porting the carriage return handling form the classic UI! Just a quick note. The current implementation replaces the whole line with only the last part after Since interpreters are writing output with that behavior in mind, I think it might make sense to handle it that way in the new UI too. What do you think? |
Thanks for your review. I agree with your suggestion and will test that feature. But doesn't the classic UI reflect this feature? Would it be better to fix both classic and new UI? |
I also prefer fixing both the classic and new UI so they stay consistent. Probably not, but figured I'd ask just in case. |
I am not aware of anything like this. The new implementation is then simply not backported. This allows us to make breaking changes. |
… terminal-like CR for classic and new web app
@tbonelee I fix the code to support terminal-like carriage return. Now I tested this in shell interpreter. (+ python interpreter, also) %sh
printf 'Initializing system, please wait...\rLoading components...\n'
printf 'Progress: [25%%]\rProgress: [50%%]\rProgress: [100%%]\n' ![]() ![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm just asking to be sure, but didn’t @Reamer’s comment suggest that it might be better not to backport this to the Classic UI?
If that's the case, then I guess there's no need to modify zeppelin-web
, so I wanted to check.
What is this PR for?
Fix an issue where the new Angular UI (zeppelin-web-angular) does not correctly handle carriage return (
\r
) characters.This causes outputs like progress bars(
#
) or status updates that use\r
to overwrite lines to appear ugly and unnecessarily long.This implementation is a direct port of the previous logic from the classic UI (zeppelin-web), specifically based on the original implementation in result.controller.js and result.js.
Additionally, this update changes the behavior of the carriage return (
\r
) character to better emulate standard terminal functionality.Instead of clearing the entire line, the
\r
character now moves the cursor to the beginning of the line and overwrites the existing text. This "terminal-like" partial overwrite behavior is more accurate and what most interpreters expect.This enhancement has been applied to both the classic and new UIs for consistent, standards-compliant output.
What type of PR is it?
Bug Fix
Todos
What is the Jira issue?
[ZEPPELIN-6214]
How should this be tested?
#
) is the easiest to test this change.Screenshots (if appropriate)
Questions: