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

compose: Allow ctrl + enter to send in preview mode as well. #22705

Merged
merged 2 commits into from Aug 16, 2022

Conversation

N-Shar-ma
Copy link
Collaborator

When the user chose to send the composebox message on pressing ctrl +
enter instead of just enter, it only worked in writing mode but not
the preview mode.

This change makes ctrl + enter send the message even in preview mode,
when that setting is chosen.

Fixes: #21670.

Self-review checklist

  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@zulipbot
Copy link
Member

Hello @zulip/server-compose members, this pull request was labeled with the "area: compose" label, so you may want to check it out!

@timabbott
Copy link
Sponsor Member

We already have this function:

export function enter_with_preview_open() {                                           
    if (user_settings.enter_sends) {                                                  
        // If enter_sends is enabled, we attempt to send the message                  
        finish();                                                                     
    } else {                                                                          
        // Otherwise, we return to the compose box and focus it                       
        $("#compose-textarea").trigger("focus");                                      
    }                                                                                 
}                                                                                     

So the question is why that code isn't handling this; we shouldn't add a duplicate.

@timabbott
Copy link
Sponsor Member

Oh, but this is Ctrl+Enter. Well, that still presents the question of what we think should happen if user_settings.enter_sends is false; I think probably we want basically the opposite of the above function?

@@ -1137,7 +1135,7 @@ test("initialize", ({override, mock_template}) => {
user_settings.enter_sends = false;
event.metaKey = true;
let compose_finish_called = false;
override(compose, "finish", () => {
override_rewire(compose, "finish", () => {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We're generally trying to avoid new use of rewire; is this necessary?

@N-Shar-ma
Copy link
Collaborator Author

@timabbott Thank you for the feedback! I was initially trying to use the enter_with_preview_open function for ctrl+enter as well, and overcomplicated things by using the should_enter_send function.

Anyway, I thought through it all again and have a much smaller and simpler PR now, using that 1 function only, just with an optional parameter and more specific conditions. The original tests remain untouched.

Please have another look, and let me know if anything else can be improved!

timabbott and others added 2 commits August 16, 2022 14:34
I'm not sure whether the bug this fixes was a regression resulting
from d6d3683, or an old bug, but
focusing the compose box is not sufficient to end preview mode; we
should be calling the function that's explicitly for that.
When the user chose to send the composebox message on pressing ctrl +
enter instead of just enter, it only worked in writing mode but not in
the preview mode.

This change makes ctrl + enter send the message even in preview mode,
when that setting is chosen.

Fixes: zulip#21670.
@timabbott
Copy link
Sponsor Member

This seemed to not work for me -- when I hit Ctrl+Enter in preview mode with "Enter sends" enabled. But I figured out the bug was, and possibly the situation changed with the merge of d6d3683 earlier today; I've added a commit that fixes it.

I also changed it back to a simple else statement, since I think we Ctrl+Enter and Enter to have their behavior swap when the "Enter sends" setting is toggled.

Merged with those changes, thanks @N-Shar-ma!

@timabbott timabbott merged commit 87a6c39 into zulip:main Aug 16, 2022
@N-Shar-ma N-Shar-ma deleted the ctrl-enter-in-preview branch August 17, 2022 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ctrl + Enter doesn't send compose box message in preview mode.
3 participants