FileManager, History, Collections: expose file dialog buttons#12845
Conversation
poire-z
left a comment
poire-z
left a comment
There was a problem hiding this comment.
Looks clean and proper - hope it works :)
| return true | ||
| end | ||
|
|
||
| function FileManager:addFileDialogButtons(row_id, row_func) -- FileManager, History, Collections file_dialog |
There was a problem hiding this comment.
(Should this one be called addFileDialogButtonRow() as it expects a row ? Or just Buttons plural is enough to make it clear?)
| self.file_dialog_added_buttons = nil | ||
| else | ||
| self.file_dialog_added_buttons.index[row_id] = nil | ||
| for id, idx in pairs(self.file_dialog_added_buttons.index) do |
There was a problem hiding this comment.
(I thing using pairs() is right - just asking so you too are sure we should not use ipairs().)
There was a problem hiding this comment.
index is a hashmap table, { row_id = row index in the buttons table }.
| function CoverBrowser.addFileDialogButtons(widget) | ||
| FileManager.addFileDialogButtons(widget, "coverbrowser_1", function(file, is_file, bookinfo) |
There was a problem hiding this comment.
(It's good these use . and not : as they are utilities functions. Took me a few seconds to get that right - widget can be a FileManager instance :) and we will call FileManager (class) .addFileDialogButtons( FileManager (instance) ) - wondering if it needs a comment - or nevermind.)
There was a problem hiding this comment.
To add buttons, in an outer module we call:
FileManager:addFileDialogButtons(row_id, row_func)
(In CoverBrowser we do FileManager.addFileDialogButtons(FileManager, row_id, row_func) for unification)
FileManager.addFileDialogButtons(FileManagerHistory, row_id, row_func)
FileManager.addFileDialogButtons(FileManagerCollections, row_id, row_func)
There was a problem hiding this comment.
To avoid triplicating the code.
There was a problem hiding this comment.
and we will call
FileManager(class).addFileDialogButtons( FileManager(instance))
No, additional buttons are saved to the baseclass.
There was a problem hiding this comment.
For FM we can add them to FileChooser, that was a choice.
I decided that FileManager was more logical, since the additional buttons to be added to the FM file dialog.
There was a problem hiding this comment.
I know, I did understand all this and it is perfect - explanations were not needed :)
It's just that reading FileManager.addFileDialogButtons(FileManager, row_id, row_func) felt odd, until I noticed that the first . wasn't a : and that FileManager.addFileDialogButtons is a function and not a method. Some little comment may have saved me a few seconds :)
Remove unused UIManager:clearRenderStack(). Previously it was used by CoverBrowser plugin when extending the FileManager popup file dialog with aditional buttons. Now the buttons are inserted through the api added in #12845.
Remove unused UIManager:clearRenderStack(). Previously it was used by CoverBrowser plugin when extending the FileManager popup file dialog with aditional buttons. Now the buttons are inserted through the api added in koreader#12845.
2 participants
This
koreader/plugins/coverbrowser.koplugin/covermenu.lua
Lines 223 to 226 in 8297d17
Closes #12823.
This change is