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

Add usage info to sort actions #2004

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MatthiasKunnen
Copy link

The actions in the Sort toggles (t) prompt were not very clear to me and lead to a lot of looking up with some confusion:
'a'u/'d'u/'e'xt/'r'ev/'s'z/'t'm/'v'er/'c'lr/'^T'?

So I dove into the source to find out what all the options do and added a help option.
Now, when responding to the prompt with ?, the info of the options is displayed. This follows the Don't memorize! approach taken by nnn.

The same approach as the general help is employed. I have looked into reusing the same code for both help functions but did not find a clean way to do it.

Regarding the order of the usage info, I debated between same order as the prompt, alphabetical, and grouped by type (e.g. all file size/disk usage together) and decided on alphabetical. If you would like prompt order, or any other order, let me know.

The binary size did not increase, to my surprise, it stayed at 126 440 B (compiled on x64 Arch Linux, gcc 14.2.1 20250207).

@@ -689,7 +689,7 @@ static const char * const messages[] = {
"['l's]/'o'pen/e'x'tract/'m'nt?",
"keys:",
"invalid regex",
"'a'u/'d'u/'e'xt/'r'ev/'s'z/'t'm/'v'er/'c'lr/'^T'?",
"'a'u/'d'u/'e'xt/'r'ev/'s'z/'t'm/'v'er/'c'lr/'^T'/?",
Copy link
Owner

Choose a reason for hiding this comment

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

The question mark is literally a question mark. In fact we can remove it.

The short forms are chosen in such a way that it should not take long to get familiar with the them.

Adding the details adds to the binary size and we want to avoid that. Please add a new section SORT below the FILTERS section in the man page and add the same details there.

Copy link
Author

@MatthiasKunnen MatthiasKunnen Feb 20, 2025

Choose a reason for hiding this comment

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

I'm aware that question mark was literal and thought to change it to an action. This is similar to other programs such as git where the ? can be used to explain the options.

The short forms are chosen in such a way that it should not take long to get familiar with them.

Perhaps this is the case, though, in my opinion, this prompt is not friendly to new or infrequent users. It promotes memorization of all options rather than having the information at your fingertips as is the case when typing ? in the main view. Instead of a quick check with ?, not knowing what an option does, becomes; opening a new terminal, man nnn, finding the relevant section, finding the option, closing the terminal, and then using it in nnn. This can be especially unpleasant when SSH'ed into another system.

I am agreeable to your plea to keep the binary size small. That is why I checked how much the binary size would increase by. If there were to be a significant increase in size, I would agree that it would be an argument to not include it in the binary. However, my compilation of nnn revealed that the binary size is not impacted by this PR. Its size stays the exact same.

If binary size is the only reason for not including this help text in the program, would its non-impact be something that could make you reconsider?

Eager to hear your thoughts

Copy link
Owner

Choose a reason for hiding this comment

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

You are not seeing the size increase because the final binary size may include some padding. But the extra characters and the additional logic would take some bytes.

Copy link
Owner

Choose a reason for hiding this comment

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

We should be fine with the documentation in the man page, wiki and completion scripts. No need to add in the binary as well.

For the wiki, please update here: https://github.com/jarun/nnn/wiki/Usage#configuration

Copy link
Author

Choose a reason for hiding this comment

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

I'll make the changes as requested.

In addition, would you accept a user patch (https://github.com/jarun/nnn/tree/master/patches) that implements the usage info in the binary? If so, I would add a commit that turns the source changes in this PR into a user patch.

Copy link
Owner

Choose a reason for hiding this comment

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

Not required.

Copy link
Author

Choose a reason for hiding this comment

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

My question is: If I made one, would you accept/consider it? I'm interested in making one but there is no point if you dont want it

@@ -37,7 +37,7 @@ args=(
'(-s)-s[load session]:session name'
'(-S)-S[persistent session]'
'(-t)-t[timeout to lock]:seconds'
'(-T)-T[sort order]:key:(( a\:"apparent disk usasge" d\:"disk usage" e\:"extension" r\:"reverse" s\:"size" t\:"time" v\:"version" ))'
'(-T)-T[sort order]:key:(( a\:"apparent disk usage" d\:"disk usage" e\:"extension" r\:"reverse" s\:"size" t\:"time" v\:"version" ))'
Copy link
Owner

Choose a reason for hiding this comment

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

Please add this for the fish shell completion script as well.

Copy link
Author

Choose a reason for hiding this comment

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

I'll have a look

static void show_sort_help()
{
static const char helpstr[] = {
"Set sort order usage:\n"
Copy link
Owner

Choose a reason for hiding this comment

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

Please move this to the man page.

Copy link
Author

Choose a reason for hiding this comment

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

I'll be looking to add this to both wiki and man page

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