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 option to rm -rf irrespective of trash #1811

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Conversation

62832
Copy link
Contributor

@62832 62832 commented Feb 3, 2024

Heya. I was slightly annoyed by the fact that using NNN_TRASH completely disabled the ability to rm -rf within nnn, so I wanted to add a separate option for ^X that would just use rm -rf regardless of the trash backend being used.

Hope this is alright.

Copy link
Collaborator

@KlzXS KlzXS left a comment

Choose a reason for hiding this comment

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

I think this is a fine addition. It always seemed redundant to me that there were two keybinds for the exact same thing bound to the exact same key.

Though it is a bit dangerous for the people who've grown accustomed to using ^X for deleting files with trash enabled. The last change (there have been additions since, but those wouldn't affect existing workflows) in keybinds was in April 2022 and that was much less impactful than this would be.

src/nnn.c Outdated Show resolved Hide resolved
src/nnn.c Outdated Show resolved Hide resolved
src/nnn.h Outdated Show resolved Hide resolved
src/nnn.c Outdated Show resolved Hide resolved
@62832 62832 force-pushed the rm-only branch 3 times, most recently from 1074ceb to f3fe145 Compare February 4, 2024 19:51
@N-R-K
Copy link
Collaborator

N-R-K commented Feb 4, 2024

You can accomplish the same thing with a simple plugin. Bind it to x and then call it with alt+x. Don't think we need to have this builtin, and I especially don't think we should bind it to ^x and change existing bind.

@62832 62832 force-pushed the rm-only branch 2 times, most recently from ef73cfc to 8200401 Compare February 4, 2024 20:07
@KlzXS
Copy link
Collaborator

KlzXS commented Feb 4, 2024

On the other hand, I think this is a useful bultin. It's a bit odd that you can't specify to permanently delete a file if you configure trash. Maybe even going as far as to say that most file managers have such a feature. So, delegating it to a one-liner plugin seems a bit off.

I do agree about the keybind tough. I don't expect it to affect anybody realistically, but who knows. This would have been perfect back before keybinds were stabilized, but apparently no one thought of that at the time.

@KlzXS
Copy link
Collaborator

KlzXS commented Feb 4, 2024

...unless we do something like with batch rename where we call a plugin if it's found and otherwise do the builtin thing.

It's a bit convoluted, but would at the same time keep the existing behavior for most users while allowing something extra for those willing to opt in. Also, there was some talk recently about supporting more trash managers. I didn't follow that through, but I imagine giving them a script they can edit would allow them better compatibility than just building all those options directly in.

What do you think?

@N-R-K
Copy link
Collaborator

N-R-K commented Feb 4, 2024

do something like with batch rename where we call a plugin if it's found and otherwise do the builtin thing.

That might be a good idea. Would allow people to use other trash utilities or add special logic to skip trash and rm directly and what not.

@KlzXS
Copy link
Collaborator

KlzXS commented Feb 4, 2024

@62832 the plugin in question is .nmv and the corresponding symbol in code is UTIL_NMV. In case you want to take a look at that.

src/nnn.c Outdated Show resolved Hide resolved
src/nnn.c Outdated Show resolved Hide resolved
@jarun
Copy link
Owner

jarun commented Feb 7, 2024

@KlzXS there is one problem. For users who use the type-to-nav mode, ^X is the natural way to trash/delete. In my case, I still use rm -rf but someone used to trashing files would inadvertently remove the files.

So I am inclined to agree with @N-R-K on this aspect.

However, I think we can have the best of both the worlds. We can add a program option -X (capital-X) which would force rm -rf with ^X when a different trash is configured.

What do you think?

@62832 hold on till we conclude this discussion.

src/nnn.c Outdated Show resolved Hide resolved
src/nnn.h Outdated Show resolved Hide resolved
@jarun
Copy link
Owner

jarun commented Feb 7, 2024

@62832 I am fine with a new keybind X for rm -rf always.

Also, please squash all the commits into one for easy maintenance.

@62832
Copy link
Contributor Author

62832 commented Feb 8, 2024

Should be done now, having addressed @N-R-K's concerns and switched to using SHIFT+X as the bind for rm -rf. Ready when you are.

src/nnn.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@N-R-K N-R-K left a comment

Choose a reason for hiding this comment

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

Small documentation nit. Code seems ok to me.

src/nnn.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@KlzXS KlzXS left a comment

Choose a reason for hiding this comment

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

If we are happy with the new names of the actions and the description in the help this should be good to go.

Also a reminder that we should update the wiki after this is merged.

@jarun
Copy link
Owner

jarun commented Feb 9, 2024

We are good! I'll take care of the Wiki update. Thanks guys!

@62832
Copy link
Contributor Author

62832 commented Feb 9, 2024

Is that failing ubuntu-patches test going to be a problem?

@jarun
Copy link
Owner

jarun commented Feb 9, 2024

./patches/check-patches.sh fails in CI. @62832 please have a look.

./patches/check-patches.sh
7
O_COLEMAK=1 O_GITSTATUS=0 O_NAMEFIRST=0 O_RESTOREPREVIEW=0 [FAILED]
8
O_COLEMAK=0 O_GITSTATUS=1 O_NAMEFIRST=0 O_RESTOREPREVIEW=0 [SUCCESS]
9
O_COLEMAK=1 O_GITSTATUS=1 O_NAMEFIRST=0 O_RESTOREPREVIEW=0 [FAILED]
10
O_COLEMAK=0 O_GITSTATUS=0 O_NAMEFIRST=1 O_RESTOREPREVIEW=0 [SUCCESS]
11
O_COLEMAK=1 O_GITSTATUS=0 O_NAMEFIRST=1 O_RESTOREPREVIEW=0 [FAILED]
12
O_COLEMAK=0 O_GITSTATUS=1 O_NAMEFIRST=1 O_RESTOREPREVIEW=0 [SUCCESS]
13
O_COLEMAK=1 O_GITSTATUS=1 O_NAMEFIRST=1 O_RESTOREPREVIEW=0 [FAILED]
14
O_COLEMAK=0 O_GITSTATUS=0 O_NAMEFIRST=0 O_RESTOREPREVIEW=1 [SUCCESS]
15
O_COLEMAK=1 O_GITSTATUS=0 O_NAMEFIRST=0 O_RESTOREPREVIEW=1 [FAILED]
16
O_COLEMAK=0 O_GITSTATUS=1 O_NAMEFIRST=0 O_RESTOREPREVIEW=1 [SUCCESS]
17
O_COLEMAK=1 O_GITSTATUS=1 O_NAMEFIRST=0 O_RESTOREPREVIEW=1 [FAILED]
18
O_COLEMAK=0 O_GITSTATUS=0 O_NAMEFIRST=1 O_RESTOREPREVIEW=1 [SUCCESS]
19
O_COLEMAK=1 O_GITSTATUS=0 O_NAMEFIRST=1 O_RESTOREPREVIEW=1 [FAILED]
20
O_COLEMAK=0 O_GITSTATUS=1 O_NAMEFIRST=1 O_RESTOREPREVIEW=1 [SUCCESS]
21
O_COLEMAK=1 O_GITSTATUS=1 O_NAMEFIRST=1 O_RESTOREPREVIEW=1 [FAILED]
22
make: *** [Makefile:326: checkpatches] Error 1
23
Error: Process completed with exit code 2.

@jarun
Copy link
Owner

jarun commented Feb 9, 2024

@62832 please refer to: https://github.com/jarun/nnn/tree/master/patches#resolving-patch-conflicts

@62832
Copy link
Contributor Author

62832 commented Feb 9, 2024

@jarun Should be sorted now, conflicts have been resolved with the Colemak patch.

@jarun
Copy link
Owner

jarun commented Feb 9, 2024

Re-ran CI.

@jarun jarun merged commit ea2427f into jarun:master Feb 9, 2024
7 checks passed
@jarun
Copy link
Owner

jarun commented Feb 9, 2024

Thank you!

@jarun jarun mentioned this pull request Feb 9, 2024
19 tasks
@62832 62832 deleted the rm-only branch February 9, 2024 15:54
@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants