Skip to content

Conversation

jhasse
Copy link
Collaborator

@jhasse jhasse commented Apr 27, 2024

I find it a bit weird, that even in the old implementation, passing 10 will result in a string of size 9. Isn't this a bug? @evmar do you remember if there was a reason for that? I think d82e806 is the first commit which adds the 10 -> 9 unit test.

Maybe that should be fixed in the old implementation first.

Regarding this implementation: Using std::regex might be too slow, but it's a start and can be used as a reference implementation.

@jhasse jhasse mentioned this pull request Apr 27, 2024
@jhasse jhasse force-pushed the elide-middle-ansi-escape branch from 5c960f9 to 5c253c4 Compare April 27, 2024 14:27
@evmar
Copy link
Collaborator

evmar commented Apr 27, 2024

Ha, looking at your diff I think I just had it wrong. EIther an off-by-one or maybe a rounding mistake in the int elide_size = (width - kMargin) / 2 bit?

@jhasse
Copy link
Collaborator Author

jhasse commented Apr 27, 2024

elide_size rounds down and if the width is even (let's say 12), this means that the result would be:

[elide_size][kMargin][elide_size] == 4 + 3 + 4

To fix this we'd have to show another character either in the beginning or the end (my preference). I think this only works with an explicit (width - kMargin) % 2 == 1) check.

If you think this was unintentional I would have a go at it in another PR :)

@jhasse jhasse force-pushed the elide-middle-ansi-escape branch from 5c253c4 to 7454b0b Compare April 29, 2024 16:15
Copy link
Contributor

@mathstuf mathstuf left a comment

Choose a reason for hiding this comment

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

Seems to work here. Thanks!

Copy link
Contributor

@mathstuf mathstuf left a comment

Choose a reason for hiding this comment

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

Ok, playing around with it more, it seems to not work quite right. Here is a copy of the tmux buffer on a pane that is narrow enough to require elision (executed from with nvim, but the elision code is definitely involved here).

`tmux` buffer state
                                                                                                           [-Wunused-variable]                                                                                         3577 |     FlagClassification cls = FlagClassification::PrivateFlag;                                                                                                                                                 [-Wunused-variable]                                                                                         3582 |     FlagClassification cls = FlagClassification::LocationFlag;                                                                                                                                                [-Wunused-variable]                                                                                         3583 |     FlagKind kind = FlagKind::BuildSystem;                                                                                                                                                                     0-5->13/[email protected]] Linking CXX executable bin/ctest                                                                |              ^~~~                                                                                                                                                                                              0-4->14/[email protected]] Linking CXX executable bin/cmake                                                          [15-1->2/[email protected]] Linking CXX static library Source/libCMakeLib.a                                                                                                                                                       0-3->15/[email protected]] Linking CXX executable bin/ccmake                                                         [5-10->3/[email protected]] Linking CXX executable Tests/CMakeLib/testAffinity                                                                                                                                                    0-2->16/[email protected]] Linking CXX executable bin/ctresalloc                                                     [4-10->4/[email protected]] Linking CXX executable Tests/CMakeLib/testUVProcessChainHelper                                                                                                                                        0-1->17/[email protected]] Linking CXX executable Tests/CMakeLib/CMakeLibTests                                       [3-10->5/[email protected]] Linking CXX executable Tests/CMakeLib/runcompilecommands                                                                                                                                                                                                                                                        [2-10->6/[email protected]] Linking CXX executable Tests/CMakeLib/testDebuggerNamedPipe                                                                                                                                          
[1-10->7/[email protected]] Linking CXX executable Tests/CMakeLib/PseudoMemcheck/valgrind                                                                                                                                        
[0-10->8/[email protected]] Linking CXX executable Tests/CMakeLib/PseudoMemcheck/purify                                                                                                                                          
[0-9->9/[email protected]] Linking CXX executable Tests/CMakeLib/PseudoMemcheck/cuda-memcheck                                                                                                                                    
[0-8->10/[email protected]] Linking CXX executable Tests/CMakeLib/PseudoMemcheck/memcheck_fail                                                                                                                                   
[0-7->11/[email protected]] Linking CXX executable Tests/CMakeLib/PseudoMemcheck/BC                                                                                                                                              
[0-6->12/[email protected]] Linking CXX executable bin/cpack                                                                                                                                                                     
[0-5->13/[email protected]] Linking CXX executable bin/ctest                                                                                                                                                                     
[0-4->14/[email protected]] Linking CXX executable bin/cmake                                                                                                                                                                     
[0-3->15/[email protected]] Linking CXX executable bin/ccmake                                                                                                                                                                    
[0-2->16/[email protected]] Linking CXX executable bin/ctresalloc                                                                                                                                                                
[0-1->17/[email protected]] Linking CXX executable Tests/CMakeLib/CMakeLibTests                                                                                                                                                  
                                                                                                                                                                                                                      

It looks like newlines are being truncated somewhere.

@mathstuf
Copy link
Contributor

mathstuf commented May 6, 2024

I'll keep digging to see if I can find some other cause here.

Copy link
Contributor

@mathstuf mathstuf left a comment

Choose a reason for hiding this comment

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

It seems to be a Neovim thing; I've not seen anything wrong in terminals.

@jhasse jhasse merged commit d4b6084 into ninja-build:master May 14, 2024
@jhasse jhasse deleted the elide-middle-ansi-escape branch May 14, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants