imsnif / diskonaut

Terminal disk space navigator 🔭
MIT License
2.49k stars 66 forks source link

feat: only show small files legend when visible #75

Closed pjsier closed 4 years ago

pjsier commented 4 years ago

Only show the small files legend when small files are present, updates snapshots, closes #15. Doesn't address the edge case in that issue though. A few notes:

Let me know if I should make any changes, thanks!

imsnif commented 4 years ago

@pjsier - I'm sorry for taking some time to get to this. Busy days, I promise to get to it soon.

pjsier commented 4 years ago

@imsnif no problem! Thanks for the update, but there's no rush at all on my end and I appreciate you taking a look whenever is convenient

imsnif commented 4 years ago

Hey @pjsier - thanks again for your patience!

I played around with this and it looks great. I also went over the changes in the code and I'm very happy with the way you went about it. Aside from replying to your comments below, I honestly have nothing else to add.

So after addressing these, I'd be happy to merge this. I promise not to take so long this time!

I wasn't sure about splitting the rendering of the small files legend into a separate function like render_controls_legend that would return the length of the string since it's used for the other functions, but to keep things simple for now I left it as is.

How about moving everything inside if !self.hide_small_files_legend into a render_small_files_legend function? I think that can create a nice separation while allowing us not to pass stuff around too much. What do you think?

Based on the changes, it seems like this might break the usefulness of enter_folder_small_width as a test case if it's supposed to show a partial display of the status row

Eh, that's not too bad I think. :) It still tests other stuff (eg. the shorter length quit message). We could adjust the test to have unrenderable small files in order to really be sure the lines never overlap, but I think this is grand as it is right now. Thanks for spotting this and bringing it up.

The hide_small_files_legend function is potentially confusing because unlike hide_delete it takes an argument of whether or not it should be hidden. Because of the context where it's used, this seemed more straightforward than calling it conditionally, but let me know if that doesn't work.

I agree completely. I like how you did it. Only thing I would possibly change would be the boolean variable name to something like "should_be_hidden", to avoid any possible confusion. But that would be really going overboard. :)

pjsier commented 4 years ago

@imsnif thanks for the feedback! Other than the test case I think I addressed everything in the latest commit

imsnif commented 4 years ago

Looks great. Thanks for this!