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 native 'mkdir' command. #1179

Closed
wants to merge 18 commits into from
Closed

Add native 'mkdir' command. #1179

wants to merge 18 commits into from

Conversation

zSnails
Copy link

@zSnails zSnails commented Mar 27, 2023

Adds a mkdir command to allow the user to quickly create directories.

When using $mkdir the screen will flicker, and the newly created directory will not be selected by default, this can cause confusion as trying to find the directory within a big directory could be an eyesore.

To address this the user might want to create a custom command, however, users who lack the technical knowledge or just want something quick might find this cumbersome at worst.

Another issue one might encounter is that the behavior of the mkdir command might vary across operating systems.

This PR attempts to solve these issues.

By having a built in command everyone will have the same functionality out of the box.

You can still override the command, so users who've created their own command can still use it.

closes #887

@ilyagr
Copy link
Collaborator

ilyagr commented Mar 27, 2023

You can't use n since that's mapped to search-next.

Does this have an advantage over map CD push $mkdir<space>? If so, I think this would be nice to insert in the PR and commit description.

@zSnails
Copy link
Author

zSnails commented Mar 28, 2023

@ilyagr oops my bad, forgot about that one haha! I went ahead and changed the keybind!

With the mkdir command being built-in users can use it directly without having to setup any external mappings or custom commands.

It also makes it more consistent, everyone will be using the same implementation, and if they don't like something about it they can always override it with a custom command.

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! I have a couple more suggestions:

  • It would be good to link the original issue so that it gets closed automatically.
  • You might want to update the wiki section afterwards to encourage others to use this new feature.

doc.go Outdated
@@ -435,6 +436,11 @@ A custom 'delete' command can be defined to override this default.
Rename the current file using the builtin method.
A custom 'rename' command can be defined to override this default.

mkdir (modal) (default 'n')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be changed to a as well:

Suggested change
mkdir (modal) (default 'n')
mkdir (modal) (default 'a')

And remember to run go generate again so the changes are reflected.

Copy link
Author

Choose a reason for hiding this comment

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

Oops! Completely forgot about that line haha

@zSnails
Copy link
Author

zSnails commented Mar 28, 2023

Everything should be good to go now.

If the feature gets merged I'll make sure to update the wiki.

eval.go Outdated
@@ -2284,7 +2328,7 @@ func (e *callExpr) eval(app *app, args []string) {
switch app.ui.cmdPrefix {
case "!", "$", "%", "&":
app.ui.cmdPrefix = ":"
case ">", "rename: ":
case ">", "rename: ", "mkdir: ":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, one more comment from me.

This will prevent the user from using backspace to 'cancel' out a mkdir command (i.e. pressing backspace when the input line looks like mkdir: |. I think it's better to allow the user to do this, similar to other commands like find and mark-save.

rename is different because it prefills the line with the original filename, and users will generally press and hold backspace to delete the original filename, see #1052 for more details.

Copy link
Author

Choose a reason for hiding this comment

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

I also thought about that but decided to leave it because I wasn't sure.

eval.go Outdated
app.ui.loadFile(app, true)
} else {
if err := remote("send load"); err != nil {
app.ui.echoerrf("rename: %s", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I went through the code one last time, just in case, I think this is a copy/paste error:

Suggested change
app.ui.echoerrf("rename: %s", err)
app.ui.echoerrf("mkdir: %s", err)

Copy link
Author

Choose a reason for hiding this comment

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

That's right, fixed.

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

I am approving this PR, however I am not the project owner so I cannot merge this, and we will have to wait.

Thanks once again for your changes! 😃

@bluesuncat
Copy link

bluesuncat commented Mar 28, 2023

Edit:
The default "new directory" key for many application is f7.
This is the case for ranger and most other file managers on Linux, Windows and I think even MacOS.
I would argue that f7 would be a better default since it would be more intuitive for users.

@zSnails
Copy link
Author

zSnails commented Mar 30, 2023

@bluesuncat is it really the default in many applications? To be fair I've never seen F7 used for creating new directories, if anything I've seen and used more a for that purpose. But I see no downside on changing it from a to F7 whatsoever

@joelim-work
Copy link
Collaborator

I haven't used any other terminal file managers myself, but after some quick research I found that mc (Midnight Commander) uses F7 and Ranger copied from it. Also, joshuto appears to use mk by default, but we can't use it because m is already mapped to the mark-save command. If anyone else has knowledge of other file managers, that would also be helpful.

To me, it looks like a tradeoff - F7 retains compatibility with existing file managers, but is also less convenient than a as it is further away from the home keys on a keyboard.

Another option would be to just leave this unmapped by default, and a default keybinding can always be added later.

@zSnails
Copy link
Author

zSnails commented Mar 30, 2023

Right, although I don't mind if the keybind gets changed from a to F7, I feel that a is also more intuitive, I chose a because it means append or add which makes sense when creating directories, 'append directory' or 'add directory' to the current one.

Leaving it unmapped is also a viable option, although I believe doing so may prevent people from actually using it. On second thought, I don't think people would leave such a core feature unused.

@joelim-work
Copy link
Collaborator

Another thing to consider is that both ranger and joshuto map a as 'rename append', which is pretty much the same as r in lf, at least until recently when the rename command was changed to position the cursor before the extension in #1162.

If there are no plans to implement 'rename append' in lf, then I'm not sure what else relates to the concept of 'appending', and perhaps this is a suitable use for a key. I do think it is more intuitive than F7 if you ignore other file managers and look at it purely from a Vim perspective.

@laktak
Copy link
Contributor

laktak commented Mar 30, 2023

I did not follow the discussion but what is the advantage over having a native command vs. adding it to lfrc?

On advantage of lfrc is that you can modify it however you like, e.g. I prefer to cd into the new directory:

cmd mkdir %{{
    IFS=" "
    if [[ $1 == -d ]]; then
        shift
        name="$(date -I) $*"
    else
        name="$*"
    fi
    mkdir -p -- "$name"
    lf -remote "send $id cd \"$name\""
}}

@bluesuncat
Copy link

Changing the default to be unmapped sounds good to me as well.

One thing to point out here is that for an application like lf that users heavily customize, having as few default key mappings as possible is probably best, especially for keys that are easy to reach and that may already be used by custom commands.

@zSnails
Copy link
Author

zSnails commented Mar 30, 2023

@laktak Yeah you can modify it however you like, and that functionality won't go away.

Most users are familiar with their file manager software being able to create new directories by default, but lf is missing that feature.

With it being built-in users won't have to setup an external command inside their lfrc. This is also more consistent, as everyone will get the same functionality.

If they don't like the default behavior they can always override it with a custom command.

@zSnails
Copy link
Author

zSnails commented Mar 30, 2023

@bluesuncat Yeah, leaving the command unmapped by default seems to be the best solution.

@ilyagr
Copy link
Collaborator

ilyagr commented Mar 30, 2023

but after some quick research I found that mc (Midnight Commander) uses F7 and Ranger copied from it

Random piece of trivia: I believe this ultimately comes from https://en.wikipedia.org/wiki/Norton_Commander. Or, at least, if Norton Commander copied it from somewhere, I don't know what that would be.

If you're on Windows, the best successor to Norton Commander AFAIK is https://www.farmanager.com/index.php?l=en. You're welcome to try it out, but make sure to return to using lf afterwards :)

@laktak
Copy link
Contributor

laktak commented Mar 30, 2023

@zSnails got to agree with @ilyagr here, $mkdir is already built in.

@zSnails
Copy link
Author

zSnails commented Mar 30, 2023

As a side note, when using $mkdir I experience flickering, and the new directory doesn't get automatically selected, which can cause confusion on large directories.
Both of these issues are solved by using the internal command.

@ilyagr
Copy link
Collaborator

ilyagr commented Mar 30, 2023

As a side note, when using $mkdir I experience flickering, and the new directory doesn't get automatically selected, which can cause confusion on large directories. Both of these issues are solved by using the internal command.

Thanks, this answers my question from the very beginning of the thread. I think it should be more than a side note; you should put this in the the PR description.

I'm guessing that if you modify @laktak's suggestion from #1179 (comment), changing lf -remote "send $id cd \"$name\"" to lf -remote "send $id select \"$name\"", it should also not flicker (sine it uses &) and select the directory. If that snippet is not in the wiki, we should put it there.

I personally don't have a preference on whether this PR goes in or not. Having it work out of the box is useful, but having a snippet in the wiki/docs might also be good enough.

@joelim-work
Copy link
Collaborator

This is something I had not realised that existed earlier, but I think it's worth mentioning this part of the FAQ:

ranger has more things builtin whereas lf is more barebones.

Among some major features, ranger comes bundled with its own file opener named rifle whereas lf requires an external tool to manage file associations and open files. ranger also comes with many predefined commands for common operations such as creating a directory whereas lf users are expected to directly use corresponding shell commands for these operations or define their own commands. While these things may not be direct advantages or disadvantages for all, they may provide a better out-of-the-box experience to ranger users.

I am now leaning towards the idea of not introducing any new code if it can be achieved through configuration.

BTW there already is an example mkdir command in the wiki:

cmd mkdir %{{
    IFS=" "
    mkdir -p -- "$*"
    lf -remote "send $id select \"$*\""
}}

Because creating a directory is a fairly common operation, perhaps it can be added to the lfrc.example file as well?

@zSnails
Copy link
Author

zSnails commented Mar 31, 2023

I didn't know about that on the FAQ!

But wouldn't this also apply to the 'rename' command? Renaming is also a trivial operation, yet it still is a built in feature.

I find that to be contradicting of the philosophy in the FAQ.

I agree that the example mkdir command should be added to the lfrc.example. It'll make for a more streamlined experience, as new users will most likely download the example and build their configuration from there.

@FabsCR
Copy link

FabsCR commented Mar 31, 2023

yue

@joelim-work
Copy link
Collaborator

But wouldn't this also apply to the 'rename' command? Renaming is also a trivial operation, yet it still is a built in feature.

I find that to be contradicting of the philosophy in the FAQ.

@zSnails That is a good point. I checked the commit history, it looks like the rename made a built-in command in #197. I am not sure about the motivation behind this change since it was from a long time ago, but if I had to make a guess then I think it would be because renaming a file is a more dangerous operation because it can overwrite a file if the new path already exists.

In any case, because rename is an already existing feature, I think it's unlikely to be removed (but still technically possible) since it will break existing users' workflows. This is one of the challenges of maintaining a software project - once you add a new feature, it becomes difficult to remove it.

@ilyagr
Copy link
Collaborator

ilyagr commented Mar 31, 2023

One possible difference with rename is that it's useful to be able to start with the original filename and edit it interactively.

+1 on adding mkdir to lfrc.example if this pr doesn't go in. We can add a mapping there too.

Another advantage of this pr, by the way, is that it works on Windows. I forget how one creates directories with cmd.exe (it's been a while since I used Windows), though I'm sure there's a way.

@zSnails
Copy link
Author

zSnails commented Mar 31, 2023

Another advantage of this pr, by the way, is that it works on Windows. I forget how one creates directories with cmd.exe (it's been a while since I used Windows), though I'm sure there's a way.

Yeah, I mentioned that when I updated the PR earlier today.

Another issue one might encounter is that the behavior of the mkdir command might vary across operating systems.

As far as I know, lf is not tied to any specific shell, and you can choose the shell of your preference using the set shell command. However, there are some differences in behavior when using certain shells such as fish, where mkdir is built-in, and PowerShell, where mkdir is an alias to the New-Item cmdlet. Nushell (Nu) also has a built-in mkdir command that you can use to create new directories directly from the prompt. Depending on the shell you're using, some options may be missing or different, for example, PowerShell's mkdir lacks options like -p, but has a similar option called -Force.
Having a consistent approach across different platforms is always beneficial in my opinion

It's convenient when there's a consistent way of doing something across different shells. It saves time and avoids the hassle of constantly adjusting your settings when switching between them. Also, it promotes uniformity and helps prevent errors that can arise from varying configurations.

@gokcehan
Copy link
Owner

gokcehan commented Apr 1, 2023

I would like to join the discussion because this will likely determine our decisions in the future when we get similar patches for touch, chmod, chown, ln, ln -s and so on. A few quick historical notes:

  • :delete was added because we wanted a prompting mechanism by default for safety and single letter prompts without enter was kind of complicated and also asynchronous operation was useful.

  • :rename was added because it was useful to keep the current name in the prompt and many users were using horrific ways to do this with shell in their custom commands. It also required handling and prompting various cases for safety.

If I remember correctly, we had requests for a builtin :mkdir before, but we left it out in favor of $mkdir and %mkdir. I still think a builtin :mkdir is an anti-pattern as it does not have any similar advantages as above examples. Instead, we have been adding features to lf to make the underlying shell commands as usable as possible. For example, we added % commands so the ui would not flicker and errors are shown in the ui, we added :push to be able to define these commands as keybinds, and we implemented completion for shell commands so %mk<tab> could also work if you don't want a dedicated keybinding for this purpose. Personally, I have been using the latter approach myself as I like to keep the full power of shell. For example you can create multiple directory with a single command (e.g. %mkdir foo bar). You can also use brace expansions if you have bashism (e.g. %mkdir foo-{bar,baz}), and when you need more power, you can go even further with usual shell idioms (e.g. %seq 10 | xargs -I{} mkdir out-{}). In a way, I consider having easy access to the shell as the most powerful feature of lf. If the use of shell is considered a second-class citizen, what is the point of terminal file managers in the first place?

The second aspect of this patch is to have a default keybinding this operation. If you ask me, the current state of this patch is the worst possible scenario. Modal commands are useful but they can be confusing for new users as they are usually only meant to be used with a keybinding. We will likely have users trying to type :mkdir foo and wonder why there is a second prompt afterwards. Essentially with this patch, the default state of the program configuration will be unusuable. Ideally, I think we should not have any modal commands without a default keybinding. I see now that we currently have :delete and :filter as modal commands without keybindings. :delete is unbound by default for safety so users can explicitly map <delete> to fully delete or trash. I don't have much opinion about :filter but maybe we could have done something better.

@zSnails
Copy link
Author

zSnails commented Apr 1, 2023

All of those are valid points. I guess we're better off just dropping the PR altogether.

Also regarding the default keybind, the patch originally introduced the a keybind for this reason, as I felt it made sense to have a default keybind to the command.

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

The more I think about this, the more I believe this PR represents a step in the wrong direction. If this PR gets merged, users will invariably request other similar commands (e.g. touch, chmod, ln as stated above) to be implemented builtins as well. This not only adds bloat and maintenance burden to the code, but also steers lf in a direction where it becomes a tool that replaces the shell, rather than one that works seamlessly with the shell.

I was going to suggest that this be added to the lfrc.example file to demonstrate how to use push, but I see that there's already an example in there:

lf/etc/lfrc.example

Lines 48 to 50 in 27ab67a

# define a custom 'rename' command without prompt for overwrite
# cmd rename %[ -e $1 ] && printf "file exists" || mv $f $1
# map r push :rename<space>

That example should be sufficient for users to work out how to implement a mkdir mapping, and if not then it is also mentioned in the wiki. We can also leave it up to users to decide on what key they want to use, since I'm not sure if we ever settled on a being a suitable default.

I think the best thing to do would be to close this PR, and also the related issue. Don't think that your effort has been wasted - if anything the discussion that has taken place here is very valuable in terms of the direction that lf should take. 🙂


EDIT: I am not actually sure how to 'unapprove' a PR now that I have approved it, perhaps it's too late?

@joelim-work
Copy link
Collaborator

BTW the README.md contains a Non-Features section, perhaps we can add an entry there for this.

@zSnails
Copy link
Author

zSnails commented Apr 2, 2023

@joelim-work adding it to the Non-Features section of the readme sounds like a good idea, it might be good for preventing future PRs for things like these!

EDIT: I am not actually sure how to 'unapprove' a PR now that I have approved it, perhaps it's too late?

As far as I know the PR can still be closed, I don't know if closing it will close the related issue though.

@ilyagr
Copy link
Collaborator

ilyagr commented Apr 2, 2023

I was going to suggest that this be added to the lfrc.example file to demonstrate how to use push, but I see that there's already an example in there:

I actually think that mkdir would be a better example than rename there. So, I made #1188. Feel free to comment there, that PR includes
a longer explanation.

I also think that implementing mkdir in lfrc.example makes it more visible, so that it's
not necessary to also add it to "Non-features". (unless my PR also gets rejected)

@joelim-work
Copy link
Collaborator

@zSnails I'm quite certain the linked issue will only be closed if the PR is merged, not if the PR is closed.

@ilyagr By "Non-Features" I meant mkdir (and similar commands) as a builtin, but it would have to be phrased carefully otherwise users would think that there is no way to create a directory at all. I think it can be added along with your changes to the lfrc.example file (had a quick look at them, it looks fine to me).

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

gokcehan commented Apr 2, 2023

I have now added a list of such commands to the non-features section in the readme which should prevent similar patches in the future. I am also thinking of recycling the discussion here to add something about this to the FAQ for what it is worth. Thank you all for the feedback.

I am generally not too happy to make changes to the example configuration though it is not specifically related to #1188 . The issue with the example configuration is that its purpose is still ill-defined and I am the one to be blamed for this. If anything, I think maybe we should get rid of it altogether or make it even more minimal by getting rid of everything but the most fundamentals (shell options and openers comes to mind). I think there is value in having a fully-featured configuration file though I'm not sure if the example configuration is appropriate for that as these things are quite opinionated by nature. At some point I thought maybe we could add a wiki page for users to share their dotfiles and/or link it from readme. It might as well be a separate project on its own similar to how oh-my-zsh or vim distributions work. I'm not sure if there is enough interest for such things though.

@gokcehan gokcehan closed this Apr 9, 2023
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.

Implement 'mkdir' command
7 participants