syl20bnr / spacemacs

A community-driven Emacs distribution - The best editor is neither Emacs nor Vim, it's Emacs *and* Vim!
http://spacemacs.org
GNU General Public License v3.0
23.56k stars 4.9k forks source link

* layers/+spacemacs/spacemacs-defaults/funcs.el: customer variable for spacemacs/delete-buffer-file #16403

Closed sunlin7 closed 1 month ago

sunlin7 commented 1 month ago

A customer variable to control the spacemacs/delete-current-buffer-file behavior to provide smoth change for users.

@smile13241324 @sunlin7 I have not pulled develop myself yet, but I'm afraid this might cause a quite annoying UX issue to users: Wouldn't this change cause users using previously learnt binding to suddenly delete without confirmation? If true, this would be a blatant UX no-no, IMO. See also: Principle of least astonishment

If proposed bindings are desirable, a more progressive rollout would prevent users to suffer it. I'd rather push initially just the binding change from SPC f D to SPC f d, leaving SPC f D for later, with just a "moved" message for some time to allow users to adjust. At a later time, we can rollout the SPC f D binding as intended here. The latter can even just be skipped completely if extra safety is desired for users, and left to them.

pataquets commented 1 month ago

However, to prevent you from wasting any further effort, I'd like to get @smile13241324 take on the overall approach before moving forward. I'm not sure if adding yet another defcustom is the way to go or if there is another alternative solution which she'd like better.

sunlin7 commented 1 month ago

Updated with review comments.

smile13241324 commented 1 month ago

I wonder what was bound to spc f d before, if nothing than spc f d and spc f D would be treated equally by Spacemacs this is ask for confirmation.

In this case I assume that nobody really used the capital version as its one press extra per use.

If this is so then the behaviour stays the same and no smoothening of the transition is necessary.

For the exceptional case that someone relied on this behaviour I would vote for adding a conf var to restore the old behaviour but not warn if it is used.

In addition we could add a warning on the splash screen under the rolling release message listing the breaking change and when it occurred.

Edit: I think this should warn most of the affected users, what do you think?

sunlin7 commented 1 month ago

Hi @smile13241324 Thanks for your comments, I push new changes.

And I do searched the before, the SPC f d is not bind to any function. That's why shift the delete current buffer and file to SPC f d.

The SPC f D requests pressing SHIFT key can be regarded as another form of confirmating.

If I missed other key points, please let me know. Thanks.

pataquets commented 1 month ago

SPC f d wasn't bound to anything before, so it's safe to use, I guess. I can confirm it, since I miss holding Shift from time to time and nothing happens :smile: I must admit that I like SPC f d best, as it's simpler and saves some finger-twisting, improving "RSI-ness". But changing SPC f D to no confirmation is dangerous. I disagree with @sunlin7's point about holding Shift as "a way to confirm", specially once you get used to it and becomes "muscle memory". You do it quite fast and without too much thinking, once you start the sequence. I use SPC f D quite often, so this is purely based on my own experience, but it's reasonable to assume that it can be a frequently used feature for some users. Disclaimer: getting used to this it's not such a big deal for me, and I expect to adjust fairly quick. But for users doing it more often can be dangerous. Thus my concerns about safety. If we can leave "D without confirmation" as an opt-in for now, maybe with a message stating that you can configure it to help users become aware of the new custom, we can finally set it as default in the future, after some prudential time has elapsed to give users more chances of discovering the upcoming default behaviour change.

bcc32 commented 1 month ago

I strongly agree that we should not change the meaning of SPC f D without warning. Users will have this key sequence in muscle memory by now and expect it to prompt for confirmation. Changing its meaning should be gradual and opt-in, if we do it at all.

smile13241324 commented 1 month ago

Hmmm sure we can do it this way, this would mean a second var to make spc f D non asking, with a standard value of nil.

@sunlin7 would you be willing to add this one too?

I think then the transition should be very smooth.

sunlin7 commented 1 month ago

Sure! I pushed the the modified changes that intruduce a variable spacemacs-prompt-current-buffer-delete-bindings to do that. Please help review again. Thanks

bcc32 commented 1 month ago

@sunlin7, I think #16403 doesn't quite do what was agreed upon.

this would mean a second var to make spc f D non asking, with a standard value of nil.

The variable introduced actually has the opposite meaning but with a standard value of nil, so at the current tip of the develop branch, SPC f D by default does not prompt, which is a dangerous change in the behavior. Undoing the breaking change in #16423.

sunlin7 commented 1 month ago

I readed your pr, and it's okay for me. Thanks