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

Allow toggling CTRL+Enter vs Enter to send ChatAreaInput #6592

Merged
merged 44 commits into from
Apr 9, 2024

Conversation

ea42gh
Copy link
Contributor

@ea42gh ea42gh commented Mar 27, 2024

current state with console.log statements....

@ahuang11
Copy link
Contributor

ahuang11 commented Mar 27, 2024

Thanks!

That looks right to me; I'd recommend creating a new env for Panel dev. Also, clearing browser console.

https://panel.holoviz.org/developer_guide/index.html#installing-the-project

Lastly, I'm hesitant on a three word keyword, maybe enter_sends?

@philippjfr philippjfr marked this pull request as draft March 27, 2024 09:53
@@ -45,7 +52,8 @@ export class ChatAreaInputView extends PnTextAreaInputView {
export namespace ChatAreaInput {
export type Attrs = p.AttrsOf<Props>
export type Props = PnTextAreaInput.Props & {
disabled_enter: p.Property<boolean>
disabled_enter: p.Property<boolean>,
shift_enter_sends: p.Property<boolean>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be updated to enter_sends too

@@ -65,6 +73,7 @@ export class ChatAreaInput extends PnTextAreaInput {
this.define<ChatAreaInput.Props>(({Bool}) => {
return {
disabled_enter: [ Bool, false ],
shift_enter_sends: [ Bool, false ],
Copy link
Contributor

Choose a reason for hiding this comment

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

This too.

@ea42gh
Copy link
Contributor Author

ea42gh commented Mar 27, 2024

changed the parameter name to enter_sends, and added the notebook - will need some changes eventually
got some problems...

  • how do I update a PR with whatever my current code is? I clicked the update branch button, hope that is it?!
  • if you run the notebook, there are a number of issues when using the final output cell:
    • default version of the code: does not appear to allow multiline inputs?
    • when shift enter is enabled:
      - the callback to set the output is disabled somehow
      - executing shift-enter redraws the whole output cell
      How do I track down why this happens?

@@ -60,7 +61,7 @@
"source": [
"#### Basics\n",
"\n",
"To submit a message, press the Enter key."
"To submit a message, press the Enter key if **``shift_enter_sends``** is False, else press Shift-Enter."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"To submit a message, press the Enter key if **``shift_enter_sends``** is False, else press Shift-Enter."
"To submit a message, press the Enter key if **``enter_sends``** is True, else press Shift-Enter."


enter_sends = param.Boolean(
default=True,
doc="If False, the shift_enter key will send the message rather than the enter key.",
Copy link
Contributor

@ahuang11 ahuang11 Mar 27, 2024

Choose a reason for hiding this comment

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

Suggested change
doc="If False, the shift_enter key will send the message rather than the enter key.",
doc="If True, pressing the Enter key will send the message; if False, Shift+Enter will send the message.",

@@ -44,7 +44,12 @@ class ChatAreaInput(_PnTextAreaInput):

disabled_enter = param.Boolean(
default=False,
doc="If True, the enter key will not submit the message (clear the value).",
doc="If True, disables sending the message by pressing the enter or the shift_enter key (clear the value).",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
doc="If True, disables sending the message by pressing the enter or the shift_enter key (clear the value).",
doc="If True, disables sending the message by pressing the Enter or Shift+Enter key (clear the value).",

@ahuang11
Copy link
Contributor

ahuang11 commented Mar 27, 2024

If something is not working properly, I'd also check: event.preventDefault() which disables any inputs, but this is looking good so far!

Comment on lines 35 to 39
console.log( "DBG: keydown event", event.key )
if (event.key === "Enter" && ( (!event.shiftKey && this.model.enter_sends)
|| ( event.shiftKey && !this.model.enter_sends))
) {
console.log( ". enter_sends: ", event.shiftKey && !this.model.enter_sends )
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe if you swap it?

Suggested change
console.log( "DBG: keydown event", event.key )
if (event.key === "Enter" && ( (!event.shiftKey && this.model.enter_sends)
|| ( event.shiftKey && !this.model.enter_sends))
) {
console.log( ". enter_sends: ", event.shiftKey && !this.model.enter_sends )
if (event.key === "Enter" && (
(event.shiftKey && !this.model.enter_sends) ||
(!event.shiftKey && this.model.enter_sends)
)) {
if (!this.model.disabled_enter) {
console.log(". . FIRE: ", this.model.value_input)
this.model.trigger_event(new ChatMessageEvent(this.model.value_input))
this.model.value_input = ""
}
event.preventDefault()
}
})

Comment on lines 36 to 37
if (event.key === "Enter" && ( (!event.shiftKey && this.model.enter_sends)
|| ( event.shiftKey && !this.model.enter_sends))
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a suggestion, this xor can be written more concisely as:

Suggested change
if (event.key === "Enter" && ( (!event.shiftKey && this.model.enter_sends)
|| ( event.shiftKey && !this.model.enter_sends))
if (event.key === "Enter" && event.shiftKey != this.model.enter_sends)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, nice! It will need a comment though: I had to really stare at it
to figure out what it does :-)

@ea42gh
Copy link
Contributor Author

ea42gh commented Mar 27, 2024

Shift-Enter appears bound to rerun the current cell in the notebook.

I was able to make this work with Ctrl-Enter instead.

@ea42gh ea42gh marked this pull request as ready for review March 27, 2024 16:58
@ahuang11
Copy link
Contributor

ahuang11 commented Mar 27, 2024

Shift-Enter appears bound to rerun the current cell in the notebook.
I was able to make this work with Ctrl-Enter instead.

Hmm let's keep shift enter with the hope that this is fixed: #6394

@ea42gh
Copy link
Contributor Author

ea42gh commented Mar 27, 2024

Shift vs Ctrl Enter:
the change to the code would be trivial.
Replace all occurrences of "Ctrl" with "Shift" in the four files involved.

Right now, Ctrl does work, and would be consistent with the
Jupyter binding "Execute Cell and Stay in Cell"
Personally, I would not care whether Send is bound to Ctrl-Enter or Shift-Enter,
as it would not make me inadvertently send an incomplete message.

I would like to make one more edit to the Jupyter notebook though:
When send is bound to Enter, how do I create a multiline input
in the ChatAreaInput? I have not been able to make it work!

@ahuang11 ahuang11 changed the title incomplete Allow toggling Shift+Enter vs Enter to send ChatAreaInput Mar 27, 2024
@ahuang11
Copy link
Contributor

ahuang11 commented Mar 27, 2024

Are you able to press shift+enter to start new lines? Also try working outside of the notebook (call .show() on the obj in the notebook)

Try the suggestion from #6592 (comment)

@ea42gh
Copy link
Contributor Author

ea42gh commented Mar 27, 2024

Are you able to press shift+enter to start new lines? Also try working outside of the notebook (call .show() on the obj in the notebook)

Try the suggestion from #6592 (comment)

  • I did implement it and left the previous logic as a comment

  • You can serve the notebook; it does work as expected.

  • Typing Shift-Enter when enter_sends is True does not work: it reruns the cell instead, i.e.,
    there is no way of entering a multiline prompt with the current code

@ea42gh
Copy link
Contributor Author

ea42gh commented Apr 9, 2024

@ahuang11 anything I need to do here?
I don't see how the current code affects the failing tests?

@ahuang11
Copy link
Contributor

ahuang11 commented Apr 9, 2024

Sorry, I'll get to this today! No need to keep updating the branch.

Copy link
Contributor

@ahuang11 ahuang11 left a comment

Choose a reason for hiding this comment

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

Looks really nice! Just wording clean up and I believe we can merge after if all tests passes!

I recommend using Add suggestion to batch functionality to commit if you agree with the changes.

examples/reference/chat/ChatAreaInput.ipynb Outdated Show resolved Hide resolved
examples/reference/chat/ChatAreaInput.ipynb Outdated Show resolved Hide resolved
panel/chat/input.py Outdated Show resolved Hide resolved
panel/models/chatarea_input.py Outdated Show resolved Hide resolved
panel/models/chatarea_input.py Outdated Show resolved Hide resolved
panel/chat/input.py Outdated Show resolved Hide resolved
examples/reference/chat/ChatAreaInput.ipynb Outdated Show resolved Hide resolved
examples/reference/chat/ChatAreaInput.ipynb Outdated Show resolved Hide resolved
ea42gh and others added 7 commits April 9, 2024 14:47
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
@ahuang11
Copy link
Contributor

ahuang11 commented Apr 9, 2024

(Encourage you to use the Add suggestion to batch or else it triggers many many commits)

Co-authored-by: Andrew <15331990+ahuang11@users.noreply.github.com>
@ea42gh
Copy link
Contributor Author

ea42gh commented Apr 9, 2024

unfortunately add suggestions to batch was greyed out?!

@ahuang11
Copy link
Contributor

ahuang11 commented Apr 9, 2024

Usually have to do it in Files changed tab. I suppose I could've mentioned that.

@ahuang11
Copy link
Contributor

ahuang11 commented Apr 9, 2024

Great thank you for adding this!

@ahuang11 ahuang11 merged commit defe76a into holoviz:main Apr 9, 2024
14 checks passed
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.

3 participants