Remember history status filter, show filtered count in view

[melyux]

This remembers the currently status filter in the History viewer as a user setting, added with #8820 (and answers #3406 a little better).

Since we had to (and still do) grab statuses from every book by opening up the files, I abided by @poire-z's concerns (here) to make sure no extra work is added when the filter is off ("all"). I do this by only scanning the files for their status if there is a filter selected, and this data is re-used when showing the filter-selection dialog. If there is no filter currently selected, the filter-selection dialog scans the files like it did before. This means that just like before, if you have no filter selected, opening the History viewer will not scan all your history files, which can (apparently) be slow with large histories. Only when you have a filter selected will this scan occur when the History viewer is opened, but that's something you sign up for when you select a filter, so no surprises.

Other changes:

• Add number of books in the filter to the top title when a filter is selected.
• Move file status scanning to its own function because it can potentially now be called from two places.
• Removed the statuses_fetched variable because this can be inferred from whether the current filter is all or not.
• Rename status_text to filter_text for slightly better understanding, especially in the genFilterButton callback.
• Move updateItemTable down to be closer to places it's called from.

Question: Why don't we keep book statuses in history.lua? And how can a book be both in your history and "new"? :P We should probably also add a way to put a book on hold from the history viewer too.

---

This change is 

[mergen3107]

Thank you very much for this PR!

[hius07]

And how can a book be both in your history and "new"?

A book becomes "new" after resetting its settings. It remains in the history.

[hius07]

As you may notice, we do not show any counts in the window titles.
Your "current battery percentage" is the first one.
Is it a good UI practice?

[poire-z]

I abided by @poire-z's concerns (here) to make sure no extra work is added when the filter is off ("all")

Thanks, and thank you for wording that 4 different ways :) I assure you I'm reassured :)

Move updateItemTable down to be closer to places it's called from.

Don't abuse this kind of move around when not strictly necessary (it may be necessary when you add a use of a local function, as it needs to be defined above their usages - but not for methods):

• it makes it harder to review, as we don't see the diff and the changes you made - I had to open 2 browser tabs and use Ctrl-Tab to switch between them to notice what you changed
• it kills git blame (or make it really harder to get the info) for this section of the code you moved around (and filemanagerhistory.lua has a bit of... history :)

As you may notice, we do not show any counts in the window titles.
Your "current battery percentage" is the first one.
Is it a good UI practice?

Dunno, no real opinion. We have other dynamic titles (statistics and calender month and days, dictionary title... so a dynamic count - or battery level - does not seem that different).
About this one in History title, I'm not a filter user - and I understand it's not displayed when filter=="all" so I won't be annoyed - but I would be bothered if always seeing History (47) as it's rather futile info.
I don't know about other users of filters - but it feels as, now that it is saved as a global setting, people may want to get their History with always the same filter - so they may consider this as noise too.

Btw, what's the filter you would be (always?) using? Or is it just for transitional history inspection, ie. seeing all "on hold" books and opening some of them to decide which to pick - and when done restoring History filter to "all" ?

[melyux]

@poire-z I undid the move of the method. Out of curiosity, when can you do these git-unfriendly changes? An aesthetic-only PR? You don't have to worry about ever seeing History (47) either because no number is shown if no filter is chosen. For people with a set filter... I honestly don't think they'd mind the number. It's good to know how many books you're reading at once, and it would change frequently enough to not be noise. For me, I use the "Reading" filter to quickly switch between my currently reading books, which are spread across multiple folders so file browser is not an option. Also, file browser (at least in the cover browser mode) doesn't mark "finished books" properly (planning to work on that at some point) so this works better to filter them out.

I spent a few more hours going through the storyboard of this view after @hius07 pointed out the problem above. I was fetching statuses every time the filter dialog opened. The code should now actually only fetch statuses once per session like before. This will happen either on the first dialog open like before (if no filter is set) or upon opening the History view (if a filter is set), and never again until it's closed and opened again. There is an existing complex web of what calls what involved here that I distrubed, all caused by the necessity of making that expensive file enumeration. I mostly mended it.

I brought the code to a state where it shouldn't introduce any new bugs, but I did find two existing bugs:

1. If you open a book for the first time, it will not show up in the "Reading" filter until after you have closed it. Maybe this is WAD, I don't know. Maybe we don't consider a book to be "Reading" unless we've closed it at least once. I doubt it, it's probably a bug. It's because we only set it as "reading" if it has a sidecar file, which is not saved until you've closed the book once (or suspended? or 15 minutes? I guess).
2. After a status fetch is done, doing a "Reset settings" on a book does not remove it from the "Reading" filter and doesn't add it to the "New" filter, and does not update the counts for each filter, until you close the History view and open it again. Given that you can do this "Reset settings" action from the History view, this is pretty bad. So if you open the filter dialog once, the statuses for books will never be updated again even if you change them. It's because updateItemTable() is called after "Reset settings" and that doesn't do any status fetching itself. Deletion works properly because it actually updates the reading history file itself, and updateItemTable() uses that.

[poire-z]

(The following is just my opinion, not KOReader's official policy - but as being one of its police officers, I'm taking the law into my own hands :)

Out of curiosity, when can you do these git-unfriendly changes?

I would answer, as a general rule: never :)
For the reasons explained above. But also because aesthetic is in the eyes of the beholder :) And even if you and I would find some code nicer if organized in a different way, the original author may have had a reason to order it that way (even if most probably he didn't think about it - but who are we to be sure of that? :) - and sometimes the original author odd way to write things may give us some hint about his thought process.
So, as a general rule: I refrain from touching other people code, if only by politeness.

An aesthetic-only PR?

Also, KOReader's code being written by so many people, it is by essence mixed/ugly. We try to enforce some coding style when the PR is being reviewed to keep the variations minimal, but once merged, it's in the git history, and somehow put in stone.
And because it is written by so many people, having small changes and a usable git history is best for the next person hacking the file you just change - and for us (if, say, you lose interest in one month and go away) if we need to understand and fix some strange behaviour that might be related to what you changed. An aesthetic-only PR, even if made later, will make this nearly as complex. (But indeed, if you have to do it, it's nicer to separate the move around and the changes in 2 different commits, and have one of the commits explicitely mention "zero code and logic change, just moved things around" so we can trust you and skip it when researching).
The overall aesthetic of the KOReader's code base is minimal on my TOHAVE list :)

So, when? When there is a whole refactoring of the code in question - and when changing so many things it become yours, and the new aesthetic choice is rightly yours :) (I think @NiLuJe is the one doing that the most often - killing a whole lot of accumulated mess, and redoing it all.)

I undid the move of the method.

The method you moved around was small enough that I woudn't have really insisted on that. But thanks :)
I was just mentionning that so you get the idea, and so you don't go shuffling all things around in your next more impacting PRs :)

[hius07]

1. If you open a book for the first time, it will not show up in the "Reading" filter until after you have closed it.

Look at the menu Settings - Documents - Auto-save book metadata.

2. After a status fetch is done, doing a "Reset settings" on a book does not remove it from the "Reading" filter and doesn't add it to the "New" filter, and does not update the counts for each filter

You can fix this by adding statuses_fetched = false to the reset settings callback (maybe the title refreshing will be required, depending on the active filter).

[NiLuJe]

I wholeheartedly concur with @poire-z on the git concerns, FWIW ;). git blame saves lives ;). (Well, time, really ;p).

[Frenzie]

Yup, definitely never.

[melyux]

1. If you open a book for the first time, it will not show up in the "Reading" filter until after you have closed it.

Look at the menu Settings - Documents - Auto-save book metadata.

Yeah I was thinking whether the conditions for considering something "Reading" in the filter should be expanded to include the currently open book as well even if it doesn't have a sidecar file yet. But it will after 15m or closing or suspend, so it's probably fine to leave it as it is.

You can fix this by adding statuses_fetched = false to the reset settings callback (maybe the title refreshing will be required, depending on the active filter).

I fixed it by adding that and a little extra:

if self.filter ~= "all" then
    self._manager:fetchStatuses(true)
else
    self.statuses_fetched = false
end

So that should fix this bug and everything should be good. But... why do I need to do self._manager:fetchStatuses(true) instead of just self:fetchStatuses(true)? If I do the latter, I get attempt to call method 'fetchStatuses' (a nil value).

I see existing lines with nested callbacks that call self:updateItemTable() directly and others that call it with self._manager:updateItemTable(). What's with that?

koreader/frontend/apps/filemanager/filemanagerhistory.lua

Lines 242 to 245 in 1c42641

	ok_callback = function()
	UIManager:close(hist_dialog)
	require("readhistory"):clearMissing()
	self:updateItemTable()

koreader/frontend/apps/filemanager/filemanagerhistory.lua

Lines 86 to 89 in 1c42641

	ok_callback = function()
	filemanagerutil.purgeSettings(item.file)
	require("readhistory"):fileSettingsPurged(item.file)
	self._manager:updateItemTable()

[NiLuJe]

What's with that?

Without looking at the details, I would simply assume that self is the current instance of the FileManagerThingy module, while self._manager is the current FileManager instance.

[hius07]

I think it's good.

[poire-z]

Just tested this "Reset settings", with History without filter and not having invoked the top left button, and when taping on the Reset button, I get tons of log DEBUG DocSettings: data is read from (so, actually reading the docsettings, while it's not needed when no filter), which I don't get without this PR.

[hius07]

Oh yes, the wrong self.

[melyux]

The wrong self?

[melyux]

That should fix it. I still don't get why some callbacks use self and others use self._manager

[poire-z]

Yep, seems fixed.
As about self vs self._manager, it took me a few minutes to get why:

koreader/frontend/apps/filemanager/filemanagerhistory.lua

Lines 190 to 203 in 4f3d867

	function FileManagerHistory:onShowHist()
	self.hist_menu = Menu:new{
	ui = self.ui,
	width = Screen:getWidth(),
	height = Screen:getHeight(),
	covers_fullscreen = true, -- hint for UIManager:_repaint()
	is_borderless = true,
	is_popout = false,
	title_bar_left_icon = "appbar.menu",
	onLeftButtonTap = function() self:showHistDialog() end,
	onMenuHold = self.onMenuHold,
	onSetRotationMode = self.MenuSetRotationModeHandler,
	_manager = self,
	}

and how we use/set onMenuHold and onSetRotationMode (while the onLeftButtonTap feels immune to this confusion).
We define FileManagerHistory:onMenuHold(item) as a method (:), which makes the first argument implicitely be self, it's the same as FileManagerHistory.onMenuHold(self, item), a function/callable with 2 arguments.
We then pass it when creating a Menu instance, and it overrides the class-defined method/callable Menu:onMenuHold.
When Menu calls it as Menu:onMenuHold, it's like it is called as Menu.onMenuHold(self,item) with self here being the Menu instance.
Luckily, when we created the Menu instance, we pass _manager = self = the FileManagerHistory instance that we can hopefully reach from the callback we defined.

Dunno if this way of doing it was a necessity, or we just inherited it some clumsy bit of code.
Feels just using onMenuHold = function(item) self:onMenuHold(item) end would be the right way to handle this. But no idea if this would cause other issues.

[hius07]

this way onMenuHold cannot get correct item.

"Incorrect item" doesn't mean a wrong long-pressed book from the history.
It is an item from some other table (something very long, maybe the full Menu table).

[poire-z]

Yeah, that was probably needed, to access both Menu and FileManagerHistory instances' own stuff - so, one way or the other...
I'm not suggesting we should rewrite it, as it works.
But the person who wrote it that way would have saved us some time if he had just put some comments explaining why it's needed to do it that way and what is what.

[NiLuJe]

Feels just using onMenuHold = function(item) self:onMenuHold(item) end would be the right way to handle this.

Unfortunately this doesn't work, this way onMenuHold cannot get correct item. If we want to use self instead of self._manager in onMenuHold, this works:

local hist = self
function self.hist_menu:onMenuHold(item)
    hist:onMenuHold(item)
end

(We have similar trick in bookmarks)

What about onMenuHold = function(_, item) self:onMenuHold(item) end?

(First arg is the object when it's used as a method, which is probably why you were seeing a MenuThing in item)

[hius07]

Great, seems that works! I'll test this way in some more cases.

[NiLuJe]

I would still leave a comment like @poire-z suggested, though.

FWIW, I feel like the most self-explanatory approach is the one in your latest quote (and that's despite me not being a fan of that sort of syntax and its heavy reliance on lexical scoping ;p).

[poire-z]

I was going to say this might need some thinking/wrapping for BiDi - but trying it with Hebrew & Arabic, it seems to work as is and look as expected in RTL:

[poire-z]

Unrelated, but just noticed when testing the above:

@Frenzie : dunno if Arabic has had its tag updated on weblate from ar_AA to ar (we have only l10n/ar), but frontend/ui/language.lua uses only ar_AA.
So, switching language to Arabic on my emulator has no effect - I needed to hack my settings.reader.lua to set ["language"] = "ar", to get Arabic to display.

Looking at older appimages, v2020.09 has ar_AA, while v2021.03-12 has ar.
I'm sure I had seen/tested Arabic after march 2021 (edit: may be I just tested Arabic with book content, ie TOC items - not the translations themselves, and I didn't notice they were not there) - so may be something broke elsewhere?
Is it just my emulator ? Does Arabic work on a device ?

[Frenzie]

@poire-z That change will presumably be the switch from Transifex from Weblate (on my phone atm), but have we really had a nonfunctional Arabic for 2.5 years without noticing? 😧

[Frenzie]

On an old installation you would've had an increasingly outdated still mostly working localization.

[Frenzie]

There was some shenanigans related to the Transifex switch and I must've failed to notice the discrepancy.
koreader/koreader-translations@3238f54

[Frenzie]

So to summarize, ar_AA was from Transifex and therefore wasn't picked up by Weblate. Arabic was then "incorrectly" added through Weblate, after which I fixed the problem bringing the old translation into the fold. But I forgot or didn't realize at the time I also had to update the frontend.

On a semi-related note, if there are any languages sufficiently translated not present in the UI, they have to be manually added once in a while. The last time I did so was in May: #9082

[mergen3107]

@melyux

Again, thank you so much for this fix! Working great in current nightly. Are there any edge cases you would like me to check?

I use the sorting almost every day.

[melyux]

I think just using it is testing enough, can't ask for more than that. Daily users find bugs much more easily every day than the actual devs ever can, right?

I was thinking of adding the "Put book on hold" functionality to the History items' long press popup. You can only do it from the book status widget right now. Maybe you can take a crack at some code? :h

[mergen3107]

Yeap, the put on hold thingy might be great!

Not only from History, but also from FM. Like, having a light browse, users can put books on hold, mark as finished, or add as Reading on the fly. Then open history, apply filter and start reading.

I am however still a noob (well, try-and-break-then-fix guy maybe) in Lua and programming overall (or do Matlab and Fortran count?) :D So I understand I am put to devs’ mercy here, just can give opinions and feedback alright