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

Send a message by pressing Ctrl + Return or Command ⌘ + Return #245

Conversation

tyreseluo
Copy link
Contributor

@tyreseluo tyreseluo commented Nov 8, 2024

  • Shift + Return inserts a new line, as handled by Makepad's TextInput widget.
  • Return by itself will currently do nothing.
  • Ctrl + Return or Command ⌘ + Return will send the message, which behaves just like clicking the Send button.

@tyreseluo tyreseluo added the waiting-on-review This issue is waiting to be reviewed label Nov 8, 2024
@tyreseluo tyreseluo marked this pull request as ready for review November 8, 2024 08:53
@tyreseluo tyreseluo added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Nov 8, 2024
@tyreseluo tyreseluo self-assigned this Nov 8, 2024
@tyreseluo tyreseluo added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Nov 11, 2024
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Since this project is still a work-in-progress and we don't support editing an existing message, I think it's best to adopt the least-surprising UX behavior of only sending a message when Ctrl + Enter is pressed, rather than just Enter alone.

This behavior is also more common in my experience with other messaging clients. In addition, it errs on the side of caution by preventing a user from accidentally sending a message before they were ready to send it.
Thus, a plain (unmodified) Enter key press should insert a new line.

Finally, don't forget that different platforms use different keys for their main "modifier" key. Windows and Linux do typically use Ctrl, but macOS uses Command ⌘. So make sure your code works with all platforms here.

Of course, in the future, the precise behavior of Enter would be a user-customizable setting.

@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Nov 11, 2024
@tyreseluo
Copy link
Contributor Author

tyreseluo commented Nov 12, 2024

@kevinaboos So I'm only adding window and mac shortcuts for sending messages now. Wait until we can edit the message, then do a carriage return to add a new line.

@tyreseluo tyreseluo changed the title Add enter and ctrl + enter shortcuts when sending messages and can edit messages on multiple lines. Add ctrl + enter shortcuts to send new message. Nov 12, 2024
@tyreseluo tyreseluo changed the title Add ctrl + enter shortcuts to send new message. Add ctrl + enter and ⌘ + enter shortcuts to send new message. Nov 12, 2024
@tyreseluo tyreseluo added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Nov 12, 2024
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks.

Technically speaking, this isn't exactly the correct behavior that we want, since this will result in the message being sent if Ctrl + Return is pressed on macOS, and if Logo + Return is pressed on Windows or Linux.

However, I think this is an improvement that should be made within Makepad's TextInput widget, so I'll accept this PR for now since it still mostly works.

@tyreseluo
Copy link
Contributor Author

tyreseluo commented Nov 12, 2024

@kevinaboos But if it is added to the TextInput component in makepad does it affect the situation in other projects when they also use the relevant shortcut, but not ctrl + enter or logo + enter.But either way, for now it's just an option for the current situation. Later on it will still allow user to do customization as well.

@kevinaboos
Copy link
Member

@kevinaboos But if it is added to the TextInput component in makepad does it affect the situation in other projects when they also use the relevant shortcut, but not ctrl + enter or logo + enter.But either way, for now it's just an option for the current situation. Later on it will still allow for customization as well.

No no, to clarify what I meant, the TextInput widget should emit an action that includes the modifier keys held down whilst the Return key was pressed. I will add this to Makepad later.

@kevinaboos kevinaboos removed the waiting-on-review This issue is waiting to be reviewed label Nov 12, 2024
@tyreseluo
Copy link
Contributor Author

oh,ok, I misunderstood.

@kevinaboos kevinaboos changed the title Add ctrl + enter and ⌘ + enter shortcuts to send new message. Send a message by pressing Ctrl + Return or Command ⌘ + Return Nov 12, 2024
@kevinaboos kevinaboos merged commit 0c96997 into project-robius:main Nov 12, 2024
@tyreseluo tyreseluo deleted the feature/add_enter_ctrl_enter_send_message branch November 12, 2024 06:06
@kevinaboos
Copy link
Member

So I implemented this in Makepad, but on second thought I don't think it's necessary, as it's not much of an improvement over what you've done in this PR.

@kevinaboos
Copy link
Member

Just as a heads up, I implemented a different improvement to Makepad and am now using it in #252; this makes the code from this PR slightly more efficient while also avoiding platform-specific code within the Robrix app itself (which is one of our goals for makepad/robius)

@tyreseluo
Copy link
Contributor Author

You actually right. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants