Collections: filter by metadata#12981
Conversation
| function FileManagerCollection:showCollDialog() | ||
| local coll_dialog | ||
| local function genFilterByMetadataButton(text_, prop_) | ||
| return { | ||
| text = text_, |
There was a problem hiding this comment.
It's the second time you're doing that - first time and comment at #12954 (comment).
I'm not saying it's better or worse, but we have never nowhere been doing that (except in a few places where it is needed, if I remember right, mostly @NiLuJe - or may be it was only because of really conflicting self that we needed to transform into self_ or this because we need to access the real upper self in the inner function).
I'd like us all (pinging @Frenzie @NiLuJe) to agree if it's better and a new trend to follow - or not. And not just have your future PRs introduce this because you had the misfortune to have issues in some other more complicated contexts. Because here, the context/scope is clear: there's no conflict and risk at all.
There was a problem hiding this comment.
I think it's a good safeguard for local functions args.
There was a problem hiding this comment.
I have no strong opinion on this particular example given its brevity, but generally speaking I prefer something more unique and (imo) simultaneously clearer like metadata_button_text.
Aside from that I believe luacheck should normally be a sufficient guard against upvaluing, so I'm not sure if it's necessary to do so as a precaution.
There was a problem hiding this comment.
I only recall doing this (pun intended ;p) for self -> this (and only in case of explicit conflicts and/or where I though it could make the code more readable), yeah ;).
Like I said in the other PR, I'm not a fan of the trailing underscore approach (too easy to miss/typo/grok), but I can sort of understand the reasoning behind wanting to choose a different variable name on principle to avoid any and all potential shadowing.
I don't really have any better ideas though. A prefix sounds slightly worse than a suffix, because it will make the distinction between the different scoping layers harder to grasp (e.g., foo, foobar -> inner_foo, inner_foobar would punt off the actually important name to the end).
There was a problem hiding this comment.
Then again, a prefix is more grammatically correct than a suffix for most of the prefix I could think of (e.g., inner, caller, ...)
There was a problem hiding this comment.
So, yeah, TL;DR: probably easier to just find better names iff luacheck complains?
There was a problem hiding this comment.
But most of the time, we don't need any fix, prefix or suffix. We do use text or item as-is in methods, functions, local functions, inner local functions...
Even here, there is no conflct at all ! Read the 5 lines I quoted - and after if you want, there is no use of text or prop that would need these to have a underscore to not confuse future readers.
The real reason to add these underscore to inner local functions would be a new coding rule: any local function inside an upper function/method needs to have all their parameters name have that _ suffix - but it would make them less readable and more prone to typo (easy to forget that _ !).
And we'd need to update hundreds of the existing ones...
And this rule has no reason to exist.
The good rule that is not written but that we follow without thinking is: when conflict with upper value (and luacheck would tell us), rename, or suffix with a _ if the current name is worth being kept.
There was a problem hiding this comment.
I prefer something more unique and (imo) simultaneously clearer like
metadata_button_text.
Yeah, something based on the (calling) widget's class and/or function (as in, what it's used for in this specific context, not the function name ;p) probably makes the most sense?
There was a problem hiding this comment.
So, yeah, TL;DR: probably easier to just find better names iff luacheck complains?
The good rule that is not written but that we follow without thinking is: when conflict with upper value (and luacheck would tell us), rename, or suffix with a _ if the current name is worth being kept.
Yup, that's what I've always done and I see no reason to change. :-)
Edit:
Can we have an agreed standard, to not invent the prefix every time?
The equivalent would also be metadata_button_prop, wouldn't it?
| prop_values[doc_prop] = prop_values[doc_prop] or {} | ||
| table.insert(prop_values[doc_prop], idx) |
There was a problem hiding this comment.
I was testing the nightly and noticed that this doesn't actually work for keywords. You want to split keywords on newlines and match individual keywords. Currently it shows the tables with all the keywords on one line, and you can't actually filter by individual keywords.
Authors are also multivalued and newline delimited, but I didn't test specifically.
Here's a quick fix that I think resolves it. I can create a new PR if needed, but I don't have a working build environment currently to fully test (I gave up while fighting meson). I tested this patch manually on the nightly.
| prop_values[doc_prop] = prop_values[doc_prop] or {} | |
| table.insert(prop_values[doc_prop], idx) | |
| if button_prop == "authors" or button_prop == "keywords" then | |
| doc_props = util.splitToArray(doc_prop, "\n") | |
| else | |
| doc_props = { doc_prop } | |
| end | |
| for _, prop in ipairs(doc_props) do | |
| prop_values[prop] = prop_values[prop] or {} | |
| table.insert(prop_values[prop], idx) | |
| end |
There was a problem hiding this comment.
Thanks for the point, I'll look into it.
Sorting a collection by metadata will be added.
No plans for the file browser metadata browsing at the moment.
"Filter by ..." buttons in the popup menu:
On pressing, say, "Filter by language" we get the list of unique "language" props in the collection, showing number of books for each value:
On pressing an item we get the list of books with the prop matching.
Filtered by one prop:
We can apply another filter to the filtered list. Filtered by two props:
This change is