windows version

[pm100]

100% conversion to crossterm (no dual mode)

seems to work fine, test on windows 10 using new windows terminal and powershell
works on my rasp pi system too

tests all compile but dont run

[pm100]

ok, it build now on linux and mac in github CI, I dont have a mac.

of course the github CI doesnt build the windows version

All the tests fail, but they at least compile.

Also on linux the terminal is left in an odd state, needs a reset command. Noy investigated at all.

[imsnif]

Awesome. :) Let's start iterating over this. I think the first step would be to get the tests to pass. I'd start by commenting out the terminal_events assertion in each test and seeing if the snapshot tests after it pass. If they do, you can update the terminal events. Once that's done, I think we'll be very close to merging.

Another question that came to my mind today: how do the paths look on windows? do they make sense? (with all the backslashes, C:\ stuff, etc.?)

[pm100]

they have \ in them.

[pm100]

OK, progress

I changed the display of paths to always be / in tests. Most tests passed after that. I actually prefer the / display, many dev oriented tools show / on windows

there are 4 failing tests

• enter_folder_small_width. this fails because it seems that the snap does not have the (x = small files) in it, but crossterm is displaying it. I tried to track this down but cannot resolve it. This also fails on linux, so you could look at it.
• delete with no permission tests (2 of them). They fail because on windows read only just means read only. I can still delete the files. So in fact the deletion works OK. You will need a more complicated perm change, for now it could just be a non-windows test
• zoom with small files. This fails because we are getting out of sequence with the long chain of snapshots. I don't understand why. One of the snaps in the test is blank, but the crossterm test doesnt generate anything. I will keep looking. Fails on linux too

Worth noting. crossterm key codes combine the character and the control keys. So I had to add a new key! macro sub match of sh_char. If you look for the detection of + you will see I have to test for + char with or without shift key; there are 2 plusses on my kb on that needs shift (above =) and one that doesnt (on the num pad)

[imsnif]

Happy to hear. :) Just a heads up, I won't have time to look at this today - promise to get to it tomorrow.

[imsnif]

enter_folder_small_width. this fails because it seems that the snap does not have the (x = small files) in it, but crossterm is displaying it. I tried to track this down but cannot resolve it. This also fails on linux, so you could look at it.

Hrm - for me it's actually failing for a different reason. In the SELECTED string below I see one extra character:
SELECTED: subfolder_with_quite_a_long_nam( instead of SELECTED: subfolder_with_quite_a_long_nam. What is it for you?

delete with no permission tests (2 of them). They fail because on windows read only just means read only. I can still delete the files. So in fact the deletion works OK. You will need a more complicated perm change, for now it could just be a non-windows test

I see... alright, we can skip this test on window for now.

zoom with small files. This fails because we are getting out of sequence with the long chain of snapshots. I don't understand why. One of the snaps in the test is blank, but the crossterm test doesnt generate anything. I will keep looking. Fails on linux too

This also fails for a different reason for me. The layout is a little different in one of the snapshots (even though it's still correct).

Also for me, when I run this on a large folder (eg the root of my hd), the loading animation of the text above looks wrong. One letter always stays bold after the wave has passed.
Putting all of these together, my current suspicion is that crossterm does something off-by-one regarding string lengths and/or window sizes.

Maybe before debugging this though, let's make sure we're on the same page regarding the failures? If the failures are different for you, maybe we can try and understand why?

[pm100]

first one. I see the same error. I looked in the .snap file it doesnt have the (x=small files) in it, even though the UI always displays that no matter how wide or narrow the display. I assumed that the diff engine was picking up the '(' of the (x=small files). But the .new snap only has the '('. On screen when using the app, as far as I can see termion version and crossterm version look identical with those files

[pm100]

last one, I dont fully understand the issue, the test infratrusture is pretty complex but I am sure we are seeing the same thing. It was just my interpretation

[pm100]

I noticed an off by 1 on the title line too. sometimes I see CC:/foo/bar instead of C/foo/bar. I will continue to investigate. Here it is on linux

maybe if i track that down the others will resolve too

BTW - I am also looking at the memory hog issue. I think that can be dramatically improved, I am currently arm-wrestling with the rust lifetime checker over something :-(

[pm100]

well the out by one errors I was seeing were fixed by pulling the latest tui and crossterm. the crates.io versions dont fix it. It seems to be tui version, I tried to see if they had any recent closed issues that looked relevant but could not see any.

Sadly that does not fix the test failures :(.

They definitely had an out by one error on the first draw diff of each redraw.

[pm100]

fixed error number 1, breaking change in latest tui. They changed the way styles are set, I already had to update yr code because the modifier fn was removed and add / remove modifiers fns added instead. But there are more subtle effects. I filed issue with tui but worked around it for the bottomline.rs.

changed

        buf.set_string(
            area.width - small_files_len - 1,
            status_line_y,
            small_files_legend,
            Style::default()
        );

to

        buf.set_string(
            area.width - small_files_len - 1,
            status_line_y,
            small_files_legend,
            Style::default().fg(Color::Reset).bg(Color::Reset).remove_modifier(Modifier::all())
        );

If you draw over an exisitng string (which you are doing here sometimes, and for sure in the test) in the old code style::default forced that cell to 'plain., In the new code new style is merged in, and merging style::default does nothing because its empty. So I have made a style that forces 'plain'

I realized that the tests have nothing to do with changing to crossterm, you are using your own backend.

I will fix #4 next

[pm100]

still #1, actually you have an out by one error, it was harmless but that tui change exposed it. the problem is that you are rendering a last line that goes '.......long_name' in fact there is only enough space for '....long_nam' so you should have displayed nothing according to render_currently_selected, but in fact you place 'long_name' in the buffer.

The final 'e' is where the '(' of (X=small files) overwrites the buffer and messes the style.

The out by one is because you pass in max_len as area.width - small_files_text.len() but you draw starting at offset 1 not 0.

[pm100]

for #4. The test is wrong. I run your current install version set up exactly as per the test it doesnt do what the test says it should do.

There are 5 files. The test goes, and this is what my test build and yr current installable download show

start => 1,2,3 on screen (45 = pluses)
+ => 3 & 1 (4,5 = +)
+ => 1 (4,5 = +)
+ => 4,5 on screen
+ => no change , max zoom
- => 1, (4,5 =+)
- => 3 & 1 (4,5)
0 => 1,2,3 (4,5)

the snapshots have

snap 1 => 1,2,3 on screen (45 = pluses)
snap 2 => 3 & 1 (4,5 = +)
snap 3 => 1 (4,5 = +)
snap 4 => 4,5 on screen
snap 5 => no change , max zoom
snap 6 => 1, (4,5 =+)
snap 7 => 1,2,3 (4,5)

it missed the screen with 3 & 1 being shown, it goes straight to the fully zoomed out.

In many cases you add extra padding events.push(None), you did not do that in this test, maybe thats why.

[pm100]

found the tui bug that caused the out by one errors in the title fdehau/tui-rs#347. not in crates.io yet, only in github main

[imsnif]

Wow, great work @pm100!! My apologies if the code base here is a little rough around the edges or too complex at times. Not so many people have gone through it yet, and I think in certain cases you're a pioneer. I find your input very valuable.

About the snapshots: you're right, I also noted the mistake in that test when doing some debugging the other day. If you want to fix it to do the right thing, that would be great - but no stress over it. If the mistake was already there, it's not too bad to leave it for another time IMO.

Otherwise: is there anything else I can clarify, explain or that you want me to take a look at? This turned out to be quite a complex fix, I see.

[pm100]

LOL, I was going to comment on how cool I thought the code was. First time I have looked into this kind of app (text based GUI). I have a ton of spare time, retired former founder/CTO/architect/Lead Dev, got bored and decided to learn rust.

"but no stress over it" - compared to what my job was like this is truly fun. Zero pressure :-)

Anyway I can fix that test.
Problem is that tui not pushed their latest fix. I can change cargo.toml to refer to their github repo rather than crates.io but not sure if you like that, I am not sure how the versioning works then (do we always get their latest bits)

[pm100]

still on test fail #1

Complex here. The original failure was because tui has a bug when overlapping text is drawn fdehau/tui-rs#378. However, as I pointed out, your code tries to not write overlapping text, but it has an out by 1 error.

You have 3 candidate bottom lines depending on how much space you have, in this test you should choose the last one (plain file name, no word 'SELECTED" , no sizes) but you in fact display the second one "SELECTED: xxxxx"but no sizes. and the last letter gets chopped by the (

Now I fixed your out by one and the correct message is displayed . But now the test fails

:-( I think I should correct the test since it blesses incorrect output at the moment.

[pm100]

WOOHOO!!

• All tests pass
• updated travis to run windows tests too (I had to suppress the root warning in title bar during windows test cos they are running the windows tests as admin but not the linux ones as root)
• fixed linux term being left in bad state
• fixed your out by one error (and commented on a TODO you had )
• ship it !

[imsnif]

Very cool. :) I'm away tomorrow, so will take a deeper look on Tuesday.

[imsnif]

So I've gone over this. Great work, and thanks for putting so much effort into doing this!
I ran this locally and it looks great.

I left some nitpicks in the code, mostly some things we need to clean up and re-adding the event assertions in the tests.

One thing I mentioned and am a little concerned about is getting the TUI dependency from github. It would be far better to get it from crates.io, because github is mutable (even if we point to a specific commit, it can be changed - maliciously or accidentally with a history re-write). As I mentioned in the comment, I think it would be best to open an issue and ask about the release if we're stuck because of it. Maybe they'll agree to just release what we need if they have bigger things in the pipeline? At least we can understand the timeframe and act accordingly.

Looking forward to seeing this merged soon!

[pm100]

I will clean all these up. I mentioned earlier that I was not happy with the pointer to tui github. I will ask them to push a new version to crates.io. In the mean time I will fork a copy and point at that (just to ensure no unexpected changes go through). Or maybe you would prefer to fork it.

[pm100]

OK all done as per request.

All test uncommented, command sequences redone.

The only possible work item left is the 2 'commented out' tests for windows due to how file permissions work. I can look at those if you like (ex MSFT person here). But lets get this merged and then create new issue cos that will take some time.

Anyway this has been fun. First time on github, first time using git, first rust collaboration.

[imsnif]

Looks great, my friend! I made some minor changes (mostly deleting printlns and outdated comments) and I'm very happy to go ahead and merge this now.

This has been fun for me as well. If you'd like to solve those windows test, that would be great. And of course, I'd be very happy to see more contributions from you if you so wish. :)

[thisismygitrepo]

Could you please add windows version to the release?
There is only one executable for linux. diskonaut-0.11.0-unknown-linux-musl.tar.gz
All other rust packages have pre-built binaries for every system
I managed to build it by cargo but its very expensive and it would be handy if prebuilt is provided with every release for every OS.
Thanks