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

WIP: Barely working zforms support. #287

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aero31aero
Copy link
Member

This is just a proof of concept. I'm not sure where to add what code and I essentially wrote a lot of unneeded functions to export things from the message for now. I'll clean this up in the next few commits.

The current interface is to press a number key from 1 to 9 to select one of the choices available in a message:

image

This creates an obvious restriction of maximum 9 choices per message. I wonder if a popup menu to choose would be a better UI?

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Jan 31, 2019
@aero31aero
Copy link
Member Author

I'll do a little bit of a self review on this:

  • We should probably abstract the message sending functionality in MessageBox.respond_to_submessage(key) and avoid having to expose 2 functions from MessageBox.

  • We should create a separate submessages.py file that does validation AND identification of various (message)widgets and also create (urwid)widgets.

  • Along with the above, add 2 functions for the zform choices widgets: get_widget_to_render() and send_response(key), both being used inside MessageBox functions.

  • Currently we always call .send_stream_message. Figure out how to call .client.send_message directly or add an if check for PMs.

  • Consider adding navigation like <next option> <previous option> <select option> if there are more than 9 options.

  • Unit tests.

@neiljp
Copy link
Collaborator

neiljp commented Feb 3, 2019

This seemed to crash for me, with an IndexError on L458 of boxes.py? I'm not sure if anyone else has tried it?

@neiljp
Copy link
Collaborator

neiljp commented Feb 16, 2019

@aero31aero Just checking if you're wanting some kind of review on this, or plan to work on it more first?

@zulipbot
Copy link
Member

Heads up @aero31aero, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback wanted has conflicts size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants