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

\t (tab char) separators aligned incorrectly #663

Open
OlegFedko opened this issue Aug 7, 2023 · 6 comments
Open

\t (tab char) separators aligned incorrectly #663

OlegFedko opened this issue Aug 7, 2023 · 6 comments

Comments

@OlegFedko
Copy link

OlegFedko commented Aug 7, 2023

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
Copy link

nowhszh commented Aug 8, 2023

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

  • Different editors align tabs differently, which can be confusing. For example the same text with tabs is displayed differently by vim and vscode.
  • I removed the part that replaced the space from the code and noticed that the mouse clicks were shifted. (Surely this must not be the main cause)

@OlegFedko
Copy link
Author

It looks, that untabify() function is wrong:

while ( position >= 0 ) {
const auto spaces
= TabStop - ( ( initialPosition.get() + position + totalSpaces ) % TabStop );
line.replace( position, 1, QString( spaces, QChar::Space ) );
totalSpaces += spaces - 1;
position = type_safe::narrow_cast<LineLength::UnderlyingType>(
line.indexOf( QChar::Tabulation, position ) );
}

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 added a commit to nowhszh/klogg that referenced this issue Aug 8, 2023
@nowhszh
Copy link

nowhszh commented Aug 8, 2023

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
Copy link
Owner

variar commented Aug 8, 2023

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

@nowhszh
Copy link

nowhszh commented Aug 8, 2023

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
Copy link

nowhszh commented Aug 8, 2023

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

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

No branches or pull requests

3 participants