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

Customizable statusline, df #1168

Merged
merged 3 commits into from
Apr 9, 2023
Merged

Customizable statusline, df #1168

merged 3 commits into from
Apr 9, 2023

Conversation

rrveex
Copy link
Contributor

@rrveex rrveex commented Mar 21, 2023

As requested here: #1103

  • make statusline configurable (default as before)
  • diskfree: linux/freebsd different types in system-call

Edit: well, kind of as requested. I put diskfree on the "ruler", right-hand side of the statusbar. Like this, it doesn't jump around that much while navigating.

image

I could change infostat to inforstat or something, waiting for comments.

@gokcehan
Copy link
Owner

@rrveex Thanks for working on this.

As much as I see how this feature is useful, I'm usually not a fan of adding platform specific code other than maybe unix and windows. Is there a way to make this work the same with all unix systems? I'm guessing this currently does not work on BSDs other than FreeBSD and also maybe MacOS.

So far I think we called the information on the right side of filenames info, bottom left line as stat , and bottom right line as ruler. So this option should probably be named something like ruler. We can also come up with better names for the individual fields.

In general, I will wait for more feedback on this patch for a while if that's ok.

@rrveex
Copy link
Contributor Author

rrveex commented Mar 21, 2023

@gokcehan

work the same with all unix systems

done

named something like 'ruler'

done

wait for more feedback on this patch

sure :)

@gokcehan
Copy link
Owner

gokcehan commented Apr 9, 2023

@rrveex I almost forgot about this PR. It seems like we are not getting more feedback on this so I think we can merge it. Thanks for the patch.

@gokcehan gokcehan merged commit eb84772 into gokcehan:master Apr 9, 2023
gokcehan added a commit that referenced this pull request Apr 9, 2023
gokcehan added a commit that referenced this pull request Apr 9, 2023
@gokcehan
Copy link
Owner

It seems that this patch broke the build on illumos, netbsd, openbsd, and solaris as these platforms only have statvfs instead of statfs. I should have run the cross build script on my machine before r29 release to test the patch myself. It is fine to not implement a feature on some specific platforms but we should not break the build for what it is worth. Unfortunately, I could not find an easy way to fix these builds yet without adding platform specific files or using build tags.

gokcehan added a commit that referenced this pull request Apr 30, 2023
@gokcehan
Copy link
Owner

I have now pushed a temporary fix to use separate files for the builds. Hopefully, we can find a better solution for this later on.

@rrveex
Copy link
Contributor Author

rrveex commented May 1, 2023

this patch broke the build on illumos, netbsd, openbsd, and solaris

Sorry, I didn't think of testing other platforms :( (actually, I was even kind of proud of myself for bothering to fire up some Windows to check)

Don't know about a "better" solution. I don't think it's even possible in Go to have something like "this feature is only available for xyz OS" - you either have it or the compiler will complain about the undefined symbol in the "common" code. (But I'm no Go expert, perhaps it's possible).

Guess it's a question of how many lf users have non-Win/Lin platforms vs. how important the "tricky" features are vs. the effort to maintain several os_ sources.

Personally I'd go with the "several os_" - it's the way Go was designed and the probability is pretty high for people using the not-so-common OSes to have the knowledge to contribute to "their" specific parts of lf.

I also don't mind at all if you throw away df if it's too much bother.

@gokcehan
Copy link
Owner

this patch broke the build on illumos, netbsd, openbsd, and solaris

Sorry, I didn't think of testing other platforms :( (actually, I was even kind of proud of myself for bothering to fire up some Windows to check)

Actually, I'm the one to be blamed because I asked you to change the patch, though I think maybe the initial version of the patch also breaks those builds as well, I'm not sure.

Don't know about a "better" solution. I don't think it's even possible in Go to have something like "this feature is only available for xyz OS" - you either have it or the compiler will complain about the undefined symbol in the "common" code. (But I'm no Go expert, perhaps it's possible).

I feel like the real issue here is that Go is missing statvfs calls on some platforms. I'm not sure if there is a reason why they follow a different route here compared to the C api. I might open an issue to ask about this so maybe they can add this to the missing platforms in the future.

Guess it's a question of how many lf users have non-Win/Lin platforms vs. how important the "tricky" features are vs. the effort to maintain several os_ sources.

Personally I'd go with the "several os_" - it's the way Go was designed and the probability is pretty high for people using the not-so-common OSes to have the knowledge to contribute to "their" specific parts of lf.

You're right, several os_ approach seems to be the way to go, but I think I'll leave things as it is for now.

I also don't mind at all if you throw away df if it's too much bother.

I think this was a welcomed addition and it seems to work well so I think we should keep it. Thanks again.

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

Successfully merging this pull request may close these issues.

2 participants