File list: new display options#12954
Conversation
| function ReadCollection:isFileInCollections(file, ignore_show_mark_setting) | ||
| if ignore_show_mark_setting or G_reader_settings:nilOrTrue("collection_show_mark") then |
There was a problem hiding this comment.
Same comment as #12940 (comment) :)
But I don't see in this PR any isFileInCollections() call with this added second parameter (?).
There was a problem hiding this comment.
Currently it is called to check whether to draw the mark only, without the second arg.
Added the second arg for future possible calls.
| local function genSeriesSubMenuItem(text_, value_) | ||
| return { | ||
| text = text_, |
There was a problem hiding this comment.
Do we need to use text_ and value_ (with a trailing underscore) ?
(It surprised me, so I don't think we anywhere use such naming, even to avoid conflict with upper scoped variables if there really were some.)
There was a problem hiding this comment.
Several times before, I encountered conflicts in naming the standard args (like text, file etc) in local function.
So I prefer to not think about it and just give unique names.
There was a problem hiding this comment.
There was a problem hiding this comment.
Well, I'd rather like you to think a bit and see such naming is not needed in such an obvious context :) and so we don't start a new trend and future developers think this is the thing to do :)
There was a problem hiding this comment.
Good unique names, I believe.
There was a problem hiding this comment.
A single trailing underscore seems relatively error-prone at first glance, though ;).
(As in, it's an easy target for hard-to-spot typos)
There was a problem hiding this comment.
koreader/frontend/apps/reader/modules/readerhighlight.lua
Lines 2059 to 2064 in 2b59151
Can you advise 3 good unique names? Wasting time.
There was a problem hiding this comment.
In this context, there are item and content upper values - and it's just fine and perfect to use item_, content_ : it avoids the conflict, and we know what these are by using the readable and same kind of name.
In the context of your 2 PRs, there are no upper text and value - so they have the unique names you like, and don't need any appended _. And there is no risk of hard-to-spot typo.
| keep_menu_open = true, | ||
| callback = function() | ||
| local ConfirmBox = require("ui/widget/confirmbox") | ||
| UIManager:close(self.file_dialog) |
There was a problem hiding this comment.
Why these are not needed ? The file_dialog auto-closes when button pressed ? Or you want to keep them open?
There was a problem hiding this comment.
They're the main menu items. What is file_dialog?
There was a problem hiding this comment.
Dunno, don't want to have to look at the code and where these comes from :) So, you're telling me that there is self.file_dialog, and they probably come from some old copy&paste ? Trusting you.
There was a problem hiding this comment.
It's the upper menu, not popup dialog.
There was a problem hiding this comment.
So, you're telling me that there is no self.file_dialog, and they probably come from some old copy&paste ?
There was a problem hiding this comment.
I don't know, erroneous lines from 2017.
There was a problem hiding this comment.
Yup, looks like an old c/p thinko at a quick glance.
(1) Option to show/hide collection mark.
(Now I have all my books in one flat collection. "Collections search" allows to build a collection for, say, a certain author quickly.)
(2) Option to have fixed font size in List display mode, taken from #9094 (comment).
(3) Radio buttons in the "Series" submenu. To avoid confusion comparing to adjacent "Progress" and "Display hints" menues.
This change is