Skip to content
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

feat: support for ANSI color sequences(#338) #655

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

Conversation

nowhszh
Copy link

@nowhszh nowhszh commented Jul 23, 2023

Image 1:
image
Image 2:
image

@nowhszh
Copy link
Author

nowhszh commented Jul 23, 2023

Currently it's only to show what it looks like after parsing, and it will still take some time before it's merged.

Here is the final plan:

  • Cross-line ANSI sequences parsing is not in the plan (Image 1), because saving all the parsing results of a file takes up too much memory space, and cross-line ANSI sequences are a small probability event.
  • Only support parsing colors from ANSI sequences.

@variar
Copy link
Owner

variar commented Jul 24, 2023

That's a good start! Moving all escape handling to a separate class should make it easier to reuse. In current implementation escape sequences are dropped when reading lines from file. That way searching never sees them. If they are passed to UI and then transformed into actual colors, then they have to be dropped before the line is passed to hyperscan.

One idea to think about -- drop the escape sequences at the same place they are dropped now and construct a set of highlighters that should be applied to that line, could be in raw lines struct. This way there is no need to change search and quickfind code. And AbstractLogView change would be to add any such highlighters to the set.

@nowhszh
Copy link
Author

nowhszh commented Jul 24, 2023

That's a good start! Moving all escape handling to a separate class should make it easier to reuse. In current implementation escape sequences are dropped when reading lines from file. That way searching never sees them. If they are passed to UI and then transformed into actual colors, then they have to be dropped before the line is passed to hyperscan.

One idea to think about -- drop the escape sequences at the same place they are dropped now and construct a set of highlighters that should be applied to that line, could be in raw lines struct. This way there is no need to change search and quickfind code. And AbstractLogView change would be to add any such highlighters to the set.

Thank you very much, I will think about this and give reasons for my final choice!

@nowhszh
Copy link
Author

nowhszh commented Jul 25, 2023

@variar
I simply implement a set of ways to get log data(nowhszh@b138153 ). The data obtained in this way is more like raw data and it has the following features:

  • More freedom of use. Users can process the data in their own way
  • Easier to extend. When you need to add a unified way of handling data, you only need to modify the OneLineLog class.
  • Reduce the frequency of file reads. Avoid loading data from a file more than once when using data in different formats in the same location.

If you think this approach is OK, I will implement it more completely and apply it in the current PR

@nowhszh nowhszh force-pushed the ansi branch 7 times, most recently from 0ee2f44 to cce45e6 Compare July 28, 2023 18:16
@nowhszh
Copy link
Author

nowhszh commented Jul 28, 2023

Development is basically complete.
The next step is to improve the code quality and fix the fatal bugs.

@Junbo-Zheng
Copy link

Development is basically complete. The next step is to improve the code quality and fix the fatal bugs.

@nowhszh Can you please show a screenshot of the effect after integration into klogg? I mean according to your the latest PR with klogg.

@nowhszh
Copy link
Author

nowhszh commented Jul 30, 2023

Development is basically complete. The next step is to improve the code quality and fix the fatal bugs.

@nowhszh Can you please show a screenshot of the effect after integration into klogg? I mean according to your the latest PR with klogg.

I can't record on my computer. I'll record on a different computer tomorrow.

@nowhszh
Copy link
Author

nowhszh commented Jul 31, 2023

Development is basically complete. The next step is to improve the code quality and fix the fatal bugs.

@nowhszh Can you please show a screenshot of the effect after integration into klogg? I mean according to your the latest PR with klogg.

Functionality is available in the current commit code, but some code needs to be optimized before the merge is complete
Peek 2023-07-31 10-13

@nowhszh nowhszh force-pushed the ansi branch 2 times, most recently from 1ea2633 to 8ad7bea Compare July 31, 2023 15:36
@nowhszh
Copy link
Author

nowhszh commented Jul 31, 2023

@variar developed and tested, please review the code.
The SGRParser feature was developed to be more generic, so it doesn't use the Qt string and container libraries internally. It has a separate repository SGRParser, and perhaps I'll follow up with templates to make it more suitable for klogg.

@nowhszh nowhszh marked this pull request as ready for review July 31, 2023 16:00
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.

None yet

3 participants