variar / klogg

Really fast log explorer based on glogg project
https://klogg.filimonov.dev
GNU General Public License v3.0
2.37k stars 208 forks source link

\t (tab char) separators aligned incorrectly #663

Open OlegFedko opened 1 year ago

OlegFedko commented 1 year ago

There is a log file with tab char used as separator:

2023-08-02 11:30:31.7529    12345   Info    Some text
2023-08-02 11:30:31.7529    1234    Info    Some text
2023-08-02 11:30:31.7529    123 Info    Some text
2023-08-02 11:30:31.7529    12  Info    Some text
2023-08-02 11:30:31.7529    1   Info    Some text

I'm expect that tab stop points are aligned across lines, like shown above.

In fact "Info" column is correctly aligned. But "Some text" column position is related to second column size. It looks like this:

2023-08-02 11:30:31.7529   12345  Info Some text
2023-08-02 11:30:31.7529   1234   Info        Some text
2023-08-02 11:30:31.7529   123    Info       Some text
2023-08-02 11:30:31.7529   12     Info      Some text
2023-08-02 11:30:31.7529   1      Info     Some text

Screenshot: https://snipboard.io/h3kXe4.jpg


Useful extra information

Klogg version 22.06.0.1289 (built on 2022-06-14 from commit 6340d94) [built for x86_64-little_endian-llp64] running on Windows 11 Version 2009 (winnt/10.0.22621) [x86_64], concurrency 12 Qt 6.2.4, tbb 2021.7

nowhszh commented 1 year ago

It just so happens that I have read the code, and in fact klogg intentionally replaced tab with space in the original text. i think the possible reasons are as follows

OlegFedko commented 1 year ago

It looks, that untabify() function is wrong: https://github.com/variar/klogg/blob/8a5eff03a8f22630a703bd8f848a4239a735aa91/src/logdata/include/linetypes.h#L317-L324

totalSpaces should be used if replace() is made in a copy of the string and indexOf() is called for original one. But actually it is made on same string.

nowhszh commented 1 year ago

Your analysis is correct, I checked the modification history and it seems that the behavior changed during an optimization. I tried to revert the function to the pre-modification version and it ran just as expected.

Here is the modification at that time: perf: less copying on untabify

I removed totalSpaces from the current code, and it worked fine!

variar commented 1 year ago

That is indeed a bug. I wonder if result of getUntabifiedLength matches the version with the bug or without )

nowhszh commented 1 year ago

That is indeed a bug. I wonder if result of getUntabifiedLength matches the version with the bug or without )

Wait a minute. I'll check.

nowhszh commented 1 year ago

That is indeed a bug. I wonder if result of getUntabifiedLength matches the version with the bug or without )

After #664 getUntabifiedLength works correctly, before it had been an incorrect value

variar commented 4 days ago

Should be fixed in 24.11.0.1662+

PetbkA commented 3 days ago

24.11.0.1664-x64-Qt6 on Windows - seems fixed