Skip to content

Add Menu proof of concept - #639

Closed
jsmnbom wants to merge 8 commits into
masterfrom
menu-poc
Closed

jsmnbom wants to merge 8 commits into
masterfrom
menu-poc

Conversation

@jsmnbom

@jsmnbom jsmnbom commented May 26, 2017

Copy link
Copy Markdown
Member

Not yet completed but I would like some feedback :)

This adds the ability to create Menus that are navigable using InlineKeyboards.
Please see menubot.py for how I'd like it to work. Currently only normal Buttons (that work much like InlineKeyboardButton but with menu support) are implemented, but I'd like it to have a ToggleButton and a RadioButton too. This should work via chat_data and/or user_data, but I can't think of which cases it should use one or the other (or maybe it should be up to the user?), so I'd like some help with this :)

There's another problem with this currently, and that is that since we currently don't have any persistence implemented (#638), if you restart the bot, then old menus will no longer work. This can be curcumvented currently by giving each Button a unique name attribute that will be used instead of UUIDs, but while that works for now, when we implement persistence, this should be updated.

If you have any questions and/or comments as to how you'd like this implemented, please comment below or contact me on telegram @bomjacob.

@jsmnbom jsmnbom added the 📋 help-wanted work status: help-wanted label May 26, 2017
It's gross but it works?
@jsmnbom

jsmnbom commented May 27, 2017

Copy link
Copy Markdown
Member Author

Okay, I've implemented one method of having a BackButton... the code is starting to look really gross... but I'm too lazy to fix it tbh.
To have a backbutton it stores a list (a stack if you will) of where the user has been. I'd honestly prefer it if all that was contained in the callback button itself so we don't have to store state (which quickly gets old - currently it doesn't ever get cleaned which is a big problem). The issue with that is telegrams 64 byte limit on callback query data. To have it that way we'd need to use something shorter than uuids (perhaps just enumerate the buttons and have a namespace?)

jsmnbom added 2 commits May 27, 2017 22:33
Singletons are quite frankly gross and requiring the user to initialize their menus shouldn't be a huge deal
Also remove str_type stuff since it was unused
Add a id_from_update method since it was used in several places but looked incredibly gross as a "oneliner"
Also rename the name parameter of buttons to uuid to better detail what it does, and allow the toggle and radio buttons to have names seperate from their uuid
Pass update around (unfortunately many properties aren't properties anymore)
Add a post_init method to buttons that are called after they've gotten assigned their parent_menu attribute
@jsmnbom

jsmnbom commented May 28, 2017

Copy link
Copy Markdown
Member Author

Okay, I've added what I think would be the best way to have toggle and radio buttons. Please see the example bot (menubot.py) for implementation.
I now have four questions/problems remaining:

  1. Should the stack be in the callback_query data instead? This would mean needing to use something else than uuids.
  2. Currently there's no way to call a method when a toggle or radio button is pressed. Is that necessary?
  3. No way to set user based defaults. Is that a problem?
  4. No way to change buttons depending on user like how @BotFather does with /mybots. This is something that the current code doesn't handle at all (and why it's just a proof of concept). I would love to hear ideas on how this could be mitigated (or maybe we should just leave it for now?).

The way I see it is that if we get persistence (#638) then 1. isn't a big problem. (there's another problem with how to handle if the bot author changes their menus... and therefore their uuids)
2. and 3. is needed if you wanna implement some sort of settings interface, so we should maybe look into that. Would love to hear if anyone has any specific menu requirements that I haven't thought of. Currently I've sorta just modelled it after telegrams @ShopBot and @BotFather.

@JosXa

JosXa commented Oct 2, 2017

Copy link
Copy Markdown
Contributor

@bomjacob I'm motivated to check this out, but I'm getting some weird errors with the example code. I merged master, could you check if it's still running good for you?

@codecov

This comment has been minimized.

@codecov

This comment has been minimized.

@jsmnbom

jsmnbom commented Apr 30, 2018

Copy link
Copy Markdown
Member Author

I've fixed a lot of my issues by reworking it to use the factory patterns (still not sure I like it better though... thoughts?). And once we resolve #1017 and #1080 I think it might be good enough.
@JosXa what errors are you getting? Could you try again... remember to use own token :P (yes I know I accidently commited my own)

@JosXa

JosXa commented Apr 30, 2018

Copy link
Copy Markdown
Contributor

Great this gets attention again :) I'm gonna check it out

@jsmnbom

jsmnbom commented Oct 22, 2018

Copy link
Copy Markdown
Member Author

So the way I see it there are fundementally 4 ways of doing inline menus.

  1. Encode the stack and data in the callback_data directly.

    • Pros:
      Easy to implement and understand
    • Cons:
      Not secure by default (data isn't signed)
      callback_data has a max length of 64 ASCII chars so menus can't be very deep or set very much data
      Since callback_data is string only so you have to cast datatypes a lot
  2. Encode the stack and data in the callback_data and sign it using HMAC

    • Pros:
      Secure
      If we pickle the data + base64 encode it we can have arbitrary datatypes
    • Cons:
      We can now encode even less data since the hmac hash takes space
  3. Put UUIDs in callback_data and have stack and data in an internal dict

    • Pros:
      Secure (no way to predict uuids)
      We can have arbitrary datatypes
      We can have as much data as we want
    • Cons:
      We need to worry about persistence
      We need to think about how long to store that data and how/when to delete it
      ^ is more difficult than one might think at first, since a user could open a bunch of menus, which could quickly fill up RAM
      Could add a menu timeout to fix above issue
  4. Put the stack and data (and sign it using HMAC) in the menu message itself, using hidden link tags

    • Pros:
      Secure
      We can have arbitrary datatypes (if we pickle the data first)
      We can have quite a bit of data
      No need to worry about having menus timeout as with method 3
    • Cons:
      If we try to encode a truly massive amount of data then the message might get too long
      When a user copies the text of a menu, the data link is also copied, which degrades UX

Therefore I have decided to close this PR, since which one of these methods are best is very application dependent (method one is great for some things - botfather even seems to use something similar)

I have implemented a very rough version of method 4 in my latest project. Mostly cause it seemed fun to try, and was the method with the (in my opinion) fewer cons. See menu.py, utils.py for the data link encoding algorithm, and settings.py for usage.

@jsmnbom jsmnbom closed this Oct 22, 2018
@JosXa

JosXa commented Oct 22, 2018

Copy link
Copy Markdown
Contributor

Telegram now checks if the inline data you send actually originated from a bots's inline keyboard button, thus options 1 and 2 (due to security concerns) are irrelevant. I can't judge on 3 vs. 4

@github-actions github-actions Bot locked and limited conversation to collaborators Aug 21, 2020
@harshil21
harshil21 deleted the menu-poc branch August 30, 2022 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

📋 help-wanted work status: help-wanted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants