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

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

Merged

Conversation

sunlin7
Copy link
Contributor

@sunlin7 sunlin7 commented May 16, 2024

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.

(funcall #'spacemacs/delete-current-buffer-file
spacemacs-delete-current-buffer-file-arbitrary)
(unless spacemacs-delete-current-buffer-file-arbitrary
(message "You can customer the \
Copy link
Contributor

Choose a reason for hiding this comment

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

s/customer/customize
s/question/confirmation

@pataquets
Copy link
Contributor

  • I'm unsure if "arbitrary" in the defcustom name conveys the meaning of "confirmation".
  • "User deletes the buffer and file without question." looks like to me a bit odd
  • I'd rather be more verbose/specific in the "to do it" message, e.g. "to delete it" or similar.

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 sunlin7 force-pushed the custom-the-delete-buffer-file branch from 8605a6a to ab89cfb Compare May 16, 2024 23:09
@sunlin7
Copy link
Contributor Author

sunlin7 commented May 16, 2024

Updated with review comments.

@smile13241324
Copy link
Collaborator

smile13241324 commented May 17, 2024

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?

Copy link
Collaborator

@smile13241324 smile13241324 left a comment

Choose a reason for hiding this comment

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

I have added some changes to make the naming of the var more clear and changed the meaning to opt out of binding improvements rather than to make it the default.

If you could add a mentioning on the home buffer just after the rolling release part it would even be better.

Comment on lines 604 to 606
(unless spacemacs-delete-current-buffer-file-arbitrary
(message "Customer the `spacemacs-delete-current-buffer-file-arbitrary' \
to delete buffer and file without confirmation.")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the warning if the legacy behaviour is wished. In this case its a conf decision and we should not complain about it.

Comment on lines 573 to 576
(defcustom spacemacs-delete-current-buffer-file-arbitrary nil
"User deletes current buffer and file without confirmation."
:type 'boolean
:group 'spacemacs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change the name to "spacemacs-keep-legacy-current-buffer-delete-bindings"

Please change the description so that the key binding change is described and which keys are set when legacy is used and which are set the other way around.

Default for this var should be nil for we emphasize the new binding which brings more functionality while we still allow to keep the old bindings.

@sunlin7 sunlin7 force-pushed the custom-the-delete-buffer-file branch from ab89cfb to c1a69fd Compare May 17, 2024 06:06
@sunlin7
Copy link
Contributor Author

sunlin7 commented May 17, 2024

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
Copy link
Contributor

pataquets commented May 17, 2024

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 😄
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
Copy link
Contributor

bcc32 commented May 17, 2024

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
Copy link
Collaborator

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.

…dings

* layers/+spacemacs/spacemacs-defaults/funcs.el:
New customer variable spacemacs-keep-legacy-current-buffer-delete-bindings
to delete current buffer and file with/without confirmation.
@sunlin7 sunlin7 force-pushed the custom-the-delete-buffer-file branch from c1a69fd to ebd6502 Compare May 17, 2024 18:48
@sunlin7
Copy link
Contributor Author

sunlin7 commented May 17, 2024

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

@smile13241324 smile13241324 merged commit 9bf42c6 into syl20bnr:develop May 20, 2024
7 of 8 checks passed
@bcc32
Copy link
Contributor

bcc32 commented May 30, 2024

@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
Copy link
Contributor Author

sunlin7 commented May 30, 2024

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

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.

4 participants