IISC this doesn't handle inline keyboards attached to inline result messages yet. If we revisit this PR, that's one issue to think about

We just discussed this and will be going forward with it. A few notes:

• Switch to UUID instead ofid()
• built the data storage as LRU cache limiting memory usage
  • store the timestamp along with the timestamp so if the user passesmaxsize=None they can take care of deleting based on time stamps on their own.
  • Still provideCallbackQuery/Context/Bot.drop_callback_data for fine grained control

sketch for a LRU data class:

classLRUDict:data= {}maxsize=1000defput(self,key,object):iflen(self.data)==maxsize:self.data.pop(next(iter(self.data)))# dicts are only guaranteed to be ordered since py3.7, so maybe use OrderedDict instead …self.data[key]=object

Edit: Couple with linked list to make efficient

@Bibo-Joshi Ordered dict is not LRU.
Ordered dict (in python semantics) refers to the order of insertion.
So it's either to remove and insert again for every object we use or we use a proper LRU.
A proper LRU will be implemented so that every access to a key will cause it to be marked as recently used. This is done internally by keeping a doubly linked list which has the most used entries in the head and least used entries in the tail.

Though, writing this, I think that if the intention is to immediately remove items when they're accessed then an Ordered Dict would be enough. (Though it might be limiting if we change the design)

Merging master was surprisingly easy :D
Adapted the LRU idea. Namings can ofc be changed.
Tests run fine locally, let's see how they perform in CI.

After internal discussion, using the uuid should be enough, so the latest commit removes the signing procedure.
However I became aware of two flaws in the current setup:

• the data in the keyboard attached to the incomingCallbackQuery is currently not replaced
• pressing a button doesnot automatically mean that the keyboard will be deleted. The bot might just callanswer_callback_query(text='hint') to display some info, without changing the message or the keyboard. So we musn't delete the data on incomingCallbackQueries. This means that
  1. CallbackDataCache needs to be properly converted to a LRU-cache by updating the order of the items in the doubly linked queue
  2. We should provide convenience methods to delete the objects corresponding to the incoming query or the complete attached keyboard. Probably samething likeCallbackQuery.{clear_data, clear_keyboard_data},InlineKeyboardMarkup.clear_data,InlineKeyoardButton.clear_data() andMessage.clear_keyboard_data

Edit I: We could think of makingMessage.edit_text/reply_markup/… andCallbackQuery.edit_message_text/reply_markup/… (i.e. everything that drops the keyboard) drop the data automatically 🤔

Edit II: just skipping CQs with invalid data is a bad idea. Users might wanna reply with a prompt telling them that the button no longer works. Rather set the data attribute to the InvalidCallbackData exception or something like that.

Edit III: Inline messages. We don't get the keyboard back when buttons from those are pressed. Idea: instead of making the LRU cache per button, make it per keyboard. we have space for 2 uuids in 64chars anyway. So once we get a CQ, we know all the other buttons/objects that we sent along with it and can make use of Edit I. Ofc when answering an inline query with multiple results that have an inline keyboard, we don't know which one actually gets sent, but that's what the LRU cache is good for.

Thanks for the thorough review@harshil21, you found a lot of things to improve 😃 So far I've gone through your comments and marked all as resolved that where either clear or where you reacted with 👍 to my comments. I will get to the TODO section soonish. Regarding the other comments:

There's alsoTTLCachefrom cachetools which would mean that we wouldn't need theaccess_time logic, as we can just use their implementation. Maybe that can help simplifying the code?

Unfortunately this doesn't allow for the entries not to expire :/

Regarding slots, should we make a new branch based on this and then merge here? The implementation detail should be kept separate from this imo. This PR is big enough already

Upsie, read that too late :D already merged that

Finally I think an example for this would help users

stay tuned 🎶

I added an example that can be reviewed.
Also I just cherry-picked the stuff from#2262 (comment) + made according adjustments here, so all the open todo items in the description are "clean-up" after merge.
Only thing left from my side is to try & up the coverage a bit when the latest tests are finished

Okay example looks good. However, I noticed one thing while running the example - When a user spam clicks a button (say '1'), it becomes an instance ofInvalidCallbackData, and it fails. Of course when you remove thedrop_callback_data line, this doesn't happen anymore. But maybe such behaviour should be at least documented somewhere?

Ah, yes, that's right. In fact it could be considered a feature, see#2539 :D Anyway, I've updated the wiki above as follows:

However, if you want to keep memory usage low, you have
additional options to drop data:

• on receiving aCallbackQuery, you can callcontext.drop_callback_data(callback_query). This will delete the data associated with the keyboard attached to the message that originated theCallbackQuery. Callingcontext.drop_callback_data is safe in any case where you change the keyboard, i.e.callback_query.edit_message_text/reply_markup/…
  Note: If the user clicks a button more than one time fast enough, but you callcontext.drop_callback_data for the first resultingCallbackQuery, the second one will haveInvalidCallbackData ascallback_data. However, this is usually not a problem, because one only wants one button click anyway.

Note that I also scraped the sentence aboutdrop_callback_data not working for inline messages, b/c that's BS (commit incoming) :D

Also can we remove thecast() in the example and ask mypy to ignore them? makes it a little complicated for beginners.

I'm not too sure about this, tbh. If we dropcast, we'll have to add atype: ignore, which probably also confusing for beginners. I'd like to keep at least the cast in

# Get the data from the callback_data.number,number_list=cast(Tuple[int,List[int]],query.data)

becauseif you use type hinting, you'll have to do that cast in any case when using arbitrary callback data, so I think the example should showcase it. I could live with dropping the other two casts, though …. (edit: I did)