Configuration for the alternate status bar in crengine

[zwim]

Although the alternate status bar is not as smart as the lower one, I do like the status informations on top.
With this plugin the top bar can be configured.

---

This change is 

[poire-z]

For reference, the available settings I found out: #5848 (comment)

A few thoughts:

• You made it a plugin, but as it tweaks core stuff, I think I wouldn't mind it being in core, with a sub-menu in Status bar> (as we don't say it's about the footer :) - with a first sub-menu item explaining why it's only with Cre and why it is limited. Might be confusing, dunno.
• I don't like much the use of document.configurable - because it's some obscure thing to me :) I guess the benefit you get is that it's saved automatically - but you then use/read them explicitely with readSetting("copt_title") which make the whole thing a bit confusing (like all the copt/kopt stuff already is). Any thoughtful reason for you to use it? Or you just took what you found?
• I might not mind much the complexity if it stays a plugin - but if it goes into core, it shouldn't have a per-document configuration, and a global configuration: our bottom status bar does not have that. Removing this "feature" to only save global settings that will apply to the top status bar on all (past and future) books would make it clearer (and smaller menu code), and similar to the bottom bar. Keeping it might make you need to adjust it on all previously opened document, isn't it?
• There should be a check to not enable, not display it, or not add it to main menu, for non-cre documents.
• The method to handle that should logically be AltStatusBar:onReadSettings() (which we are sure is called after setttings are read and before document is rendered), rather than :init() (which no contract ensures it will stay called at this moment).

[Frenzie]

You made it a plugin, but as it tweaks core stuff, I think I wouldn't mind it being in core

That was my first thought too. :-)

[zwim]

For reference, the available settings I found out: #5848 (comment)

Thanks, I will add the remaining switches, when the code is ready.

* You made it a plugin, but as it tweaks core stuff, I think I wouldn't mind it being in core, with a sub-menu in `Status bar>` (as we don't say it's about the footer :) - with a first sub-menu item explaining why it's only with Cre and why it is limited. Might be confusing, dunno.

Plugin or core, I don't mind. But a sub-menu is confusing. Maybe 'Status bars' with a submenu for each bar?
Something like:
Status bars >
---- Top bar >
---- Bottom bar >

* I don't like much the use of `document.configurable` - because it's some obscure thing to me :) I guess the benefit you get is that it's saved automatically - 

Yes

* but you then use/read them explicitely with `readSetting("copt_title")` which make the whole thing a bit confusing (like all the copt/kopt stuff already is). Any thoughtful reason for you to use it? Or you just took what you found?

I took local status_line = config:readSetting("copt_status_line") or G_reader_settings:readSetting("copt_status_line") or 1 from readercoptlistener.lua as reference, but I will change this.

* I might not mind much the complexity if it stays a plugin - but if it goes into core, it shouldn't have a per-document configuration, and a global configuration: our bottom status bar does not have that. Removing this "feature" to only save global settings that will apply to the top status bar on all (past and future) books would make it clearer (and smaller menu code), and similar to the bottom bar. Keeping it might make you need to adjust it on all previously opened document, isn't it?

Hmm. Now, displaying the top status bar is per document and the default value for fresh opened documents is set with copt_status_line in settings.reader.lua (Long press on bottom menu). So I think it is consequent to store the settings of the top bar per document as well.

* There should be a check to not enable, not display it, _or_ not add it to main menu, for non-cre documents.

Interestingly the menu-entry is only shown with crengine and not with mupdf. But a check does no harm.

* The method to handle that should logically be `AltStatusBar:onReadSettings()` (which we are sure is called after setttings are read and before document is rendered), rather than :init() (which no contract ensures it will stay called at this moment).

Yup.

[poire-z]

But a sub-menu is confusing. Maybe 'Status bars' with a submenu for each bar?

Dunno. I think I would prefer we not add footer stuff into a submenu of Status bar>.
May be Alt status bar> at the same level? (and easier to plug it into the reader_menu_order.lua).

Also, if you move it into core, I'm not sure what's the best place to handle that. ReaderFooter sounds like not the best place :) ReaderTypeSet is where we handle other crengine specific stuff, but top status bar is not typesetting...

Now, displaying the top status bar is per document and the default value for fresh opened documents is set with copt_status_line in settings.reader.lua (Long press on bottom menu). So I think it is consequent to store the settings of the top bar per document as well.

May be - but was that really a good idea? Possibly, it was done the same way as other typesetting settings are used and set (per document), but I think for footer and top bars, your preferences in what is shown is something kind of global. Having it per document, for me, is more confusing as if you change something, it will only affect future newly opened books - previously opened books stick with what were your altStatusBar setting at the time...
BUT, there's a small reason for the copt_status_line to stay per-document: it affects the height of crengine pages - so, changing it causes a re-rendering. Having this state per-document allows the rendering cache to be valid on re-opening and avoid that re-rendering. I think this might still be worth keeping.
For the other settings (what's to be displayed in the alt status bar), these don't change the height of the status bar (except font size), so no matter how you change them, they won't cause a re-rendering, so are quite safe to be tweaked and considered globals.

[zwim]

It's a menu like "Status bar", just below that (unfortunately on the second page). Is there an option/possibility to show more menu items on bigger displays?

All "new" options are global now.

I would prefer to leave the code as a plugin. What would be the benefit in putting it into core?

Maybe we should name the thing "Upper Status Bar", alternate seems a bit misleading (the one or the other).

[poire-z]

Maybe we should name the thing "Upper Status Bar", alternate seems a bit misleading (the one or the other).

Alt status bar: off | on was initially Status bar: full | mini (which sounded innacurate, as the footer is now more "full" featured and can be taller than the top bar :)
There exists since a long time a logic of toggling between them. To not touch this logic, "alt status bar" made more sense than "top status bar" (which would imply we'd get both).
May be the logic of toggling is not the best, and it should be footer + optionnal top bar, dunno.

BUT (and I think @NiLuJe will agree), the status bar handling and its side effects on the footer, the page height, the top and bottom page margins are quite complicated. So, changing this logic and making all that possibly more consistent feels like quite a bit much more work than what you - and I - want to get into :)

I would prefer to leave the code as a plugin. What would be the benefit in putting it into core?

Consistency, logic ?
But given all the questions and problems like I have mentionned above, if we want to make it more logical in the core, it will require more thinking - and contribution via code reviews about some code I don't really know much or care about.
So, even if inconsistent and a bit ugly, I might be fine with all that being a plugin - so that I can disable it and don't care much about side effects and integration :)

But actually: :/ I really don't feel confortable with all that code - and your plugin interaction with the document configurable / per document settings / bottom toggle menu toggle - but it feels like things are remote and there may be bad interaction - but may be it just works, dunno, dontwannano :)

[zwim]

I might be fine with all that being a plugin - so that I can disable it and don't care much about side effects and integration :)

Sounds good.

[poire-z]

Sounds good.

Not really "good", it just sounds like we/I were lazy and scary :)

I think that, with keeping enable/disable global/per doc in this menu, you'll have some sync issue with what the bottom menu toggle shows (like we may already have when toggling things with gestures/dispatcher, as the bottom menu toggles manage their own state - it's hard to fix, can't find the issue were I mentionned that).
So, it's not super good doing some new stuff that introduce this inconsistency
edit: tested, and it seems to work. may be actually just because you update document.configurable, so it reflects in the bottom menu. @yparitcher : may be that could be an idea for the dispatcher vs bottom menu state mismatches.

I wrote the following before testing it - keeping it as it still apply (just a bit less strongly :)
Still not super good to have two ways to toggle the same thing (and the way you show it to set per-doc vs global, with 2 menu items, is may be a first introduction of UX difference vs our more or less UI default of using tap for per-doc and long press for set as default).

We could think about killing one or the other.
Killing the bottom menu feels like much work and risks, and people are used to find it here.
I think it would just make more sense to have in your new menu only the available top bar items with one intro menu saying:
You can set here which items you want in the top status bar, that will only be shown on CRE documents. Enabling (per document or by default) is to be done in the bottom menu.

Yes, it's not super nice - but this menu with items is not something one will go at modifying often. You play with it one day, decide what you like, and forget it. Then you enable it with the bottom menu (once, as global default) or per document - and forget it.

And if this menu is to have only checkable items and don't change much of the existing behaviour and introduce no risk, it might as well be into core menus. Where, I still don't know :) but even if you made it a plugin, you forced the menu to show among the core menus and not in the Tools> menu where plugins usually are :)

Follow up thought: it feels like all your plugin code could just fit in the currently really small frontend/apps/reader/modules/readercoptlistener.lua (at least, I wouldn't be shocked finding it in it :) as it already deals with the first set up of the top status bar:
https://github.com/koreader/koreader/blob/master/frontend/apps/reader/modules/readercoptlistener.lua

[yparitcher]

I think that, with keeping enable/disable global/per doc in this menu, you'll have some sync issue with what the bottom menu toggle shows (like we may already have when toggling things with gestures/dispatcher, as the bottom menu toggles manage their own state - it's hard to fix, can't find the issue were I mentionned that). So, it's not super good doing some new stuff that introduce this inconsistency
edit: tested, and it seems to work. may be actually just because you update document.configurable, so it reflects in the bottom menu. @yparitcher : may be that could be an idea for the dispatcher vs bottom menu state mismatches.

I think you are right, in order to fix this propery probably requires a deep dive into ConfigDialog, which is a little too much for me at the moment 😄

[poire-z]

I think you are right, in order to fix this propery probably requires a deep dive into ConfigDialog, which is a little too much for me at the moment :)

I understand :) but may be it isn't?
I get the feeling that in the normal way of bottom settings:
Hitting a toggle does 2 things (possibly the order does not matter): 1) update document.configurable (use for toggle state) and 2) send the event
So, may be Dispatcher could have for each relevant action the name of the configurable thing to update (may be it's already the name of the setting) so it could 1) send the event as usual, and 2) just additionally flip the relevant document.configurable (if it's as easy as that :)

[yparitcher]

from my quick read of Confidialog the configurable is stored and updated separate from the event, and not all setting have them, so to add this to Dispatcher would require another database of which events have a configurable and what it is named. (more indirection/duplication)

I will take a deeper look a little later.

[zwim]

We could think about killing one or the other.

I have killed the enable functionality in the menu and added an info-message.

[poire-z]

now in core, plugin is diabled

If everything works, I find it perfect and logical to have it here :)
Just a bit of tidying needed (removed leftovers from the plugin, keep EventListener instead of WidgetContainer, remove the check for provider==crengine as ReaderCoptListener is already only loaded with crengine...)

[zwim]

Should we increase the number of elements in the settings menu? If yes, should we increase it globally with max_per_page_default = 12, from touchmenu.lua?

[NiLuJe]

I can already give you @poire-z's answer: no ;p.

(I would tend to agree with him, btw ;)).

[poire-z]

I can already give you @poire-z's answer: no

:) But I don't think I would mind that much: I have limited to 5 the ones (Font, Typography language) that affect the text below, so there's room to see how it affects it. These are non-negotiable :)

But I don't know. If when we add stuff, we need to increment max_per_page_default , we won't stop :)
But I also don't like having Page 1 of 2 because we happen to have 11 elements.
We might better think about reorganizing them, moving some into others or in another menu.

But this Alt status bar one does not feel like it should have the proeminence of the others: it's a little thingy that works only on half the books, and that most users don't use (well, so is probably night mode :)
I really wouldn't mind having it into Status bar> Alt status bar> , even at top of it (cause this alt status bar is at top of screen :)

[NiLuJe]

Yeah, +1 on moving it inside Status bar, that probably makes sense ;).

[poire-z]

I really wouldn't mind having it into Status bar> Alt status bar> , even at top of it

Dunno if we yet have stuff that register/addToMainMenu that can plug into another module's menu :/
Might need some little trick to have ReaderFooter steal it from CoptListener, or CoptListener plugging it into ReaderFooter, or something :) if we want (I do) have it all inside a same module (ReaderCoptListener). I can look into how to do that if we go that road.

[zwim]

It can be inserted at the end with sorting_hint = "status_bar", but we want it on top :(

[zwim]

We can add an additional sorting_hint_top=true to menusorter to add an item on top ;)

Yes, but don't we agree, that the "Alt status bar" should be the first entry in the "Status bar" menu?
Isn't it an elegant way to achieve this with no side effects.

For future enhancements you can insist not to use "sorting_hint_top" if it is not consensus to do so?

[hius07]

+1 inside Status bar, on the top, and separator after it

[poire-z]

We can add an additional sorting_hint_top=true to menusorter to add an item on top ;)

I was thinking about having ReaderCoptListener:getAltStatusBarMenuItems(), and have ReaderFooter itself do, when it builds its own menu items:

if self.ui.crelistener then
  table.insert(mymenu, 1, self.ui.crelistener:getAltStatusBarMenuItems()
end

But I'm also fine with the sorting_hint_top as it's not much added.

[zwim]

Alternative to introduce sorting_hint_top we could add a sorting_hint_position and

    if v.sorting_hint_position then
         table.insert(sorting_hint_menu, math.min(v.sorting_hint_position, #sorting_hint_menu + 1), v)
     else
         table.insert(sorting_hint_menu, v)
     end

It would cost nothing, but might help future enhancements, dunno?

[Frenzie]

NB The sorting hint thing is not intended as the primary method for positioning; that's what the menu order is for. The sorting hint provides some backwards compatibility to people with their customized menu order and a way for third-party plugin authors to hook in. So more flexibility there is welcome, but in "core" plugins it's only intended as a fallback.

[zwim]

@poire-z: Isn't ReaderCoptListener:getAltStatusBarMenuItems(), a bit roundabout.

More transparent is the sorting_hint_top thing or building a real menu for status_bar within reader_menu_order. The later one is outside my time-budget now. If someone wants step in, its no problem.

So I would go for sorting_hint_top if there is no veto.

[poire-z]

building a real menu for status_bar within reader_menu_order

No, that's too complicated and totally uneeded, we don't want 10 items more in reader_menu_order.lua.

So I would go for sorting_hint_top if there is no veto.

Fine with me, but I don't know much about the menu order stuff and its spirit, so trusting @Frenzie thoughs on it.

Isn't ReaderCoptListener:getAltStatusBarMenuItems(), a bit roundabout.

Yes it is, but that's the kind of stuff we have lots in the code :) using stuff from other modules - and it makes kind of sense and explicit what we do:

--- a/frontend/apps/reader/modules/readerfooter.lua
+++ b/frontend/apps/reader/modules/readerfooter.lua
@@ -1697,6 +1697,11 @@ function ReaderFooter:addToMainMenu(menu_items)
     end
     table.insert(sub_items, getMinibarOption("book_title"))
     table.insert(sub_items, getMinibarOption("book_chapter"))
+
+    -- If using crengine, add Alt status bar items at top
+    if self.ui.crelistener then
+        table.insert(sub_items, 1, self.ui.crelistener:getAltStatusBarMenu())
+    end
 end

--- a/frontend/apps/reader/modules/readercoptlistener.lua
+++ b/frontend/apps/reader/modules/readercoptlistener.lua

+-- Will be added by ReaderFooter at top of its "Status bar" menu
+function ReaderCoptListener:getAltStatusBarMenu()
+    return {
+        text = _("Alt status bar"),
+        separator = true,
+        sub_item_table = {
+            {
+                text = _("About alternate status bar"),
+                keep_menu_open = true,
+                callback = function()
+                    UIManager:show(InfoMessage:new{
+                        text = "blah",
+                    })
+                end,
+                separator = true,
+            },
+        }
+    }
+end
+
 return ReaderCoptListener

[zwim]

@poire-z: Thank you for your help with roundabout :)

[poire-z]

Hint for @yparitcher : looks like this event handler is just there to update the bottom menu state (the main handler is in ReaderFont).
So, at worst, add such stupid handlers in readercoptlistener.lua and readerkoptlistener.lua ?
(Althought I'm not sure why there is this one, and only this one :)

[yparitcher]

this is basically re implementing ConfigDialog stuff in other places. If we go this route we should probably remove the configurable stuff from ConfigDialog and have it done once, leaving the event as the source of truth.

[poire-z]

Improvising, to be corrected by @Frenzie:

On CRE documents only, an alt status bar can be displayed at top of screen, with or without the regular bottom status bar.
Enabling this alt status bar, per document or by default, can be done in the bottom menu.
You can set here which information this top status bar will show.

[hius07]

Works perfectly, thanks.

Now we have different names for the same indicators in the Top and Bottom bars. Maybe it's better to unify?
Title - Book title
Clock - Current time
Page number - Current page
Reading percentage - Progress percentage
Battery - Battery status
Top status bar font size - Font size

I'm not sure which ones are better though.

[zwim]

I think we should take over the wording from the bottom bar.

[poire-z]

Yes, use the ones from the bottom bar, as they are all already translated.

[protist]

Hi all, I love the alternative status bar! I like using multiple bars to show more information. However, I found it super confusing that I enable the upper status bar with the bottom pop-up menu, but configure it with the normal upper menu. I know there's been a bit of conversation here about this already, but just my 2c.

[NiLuJe]

That's an interesting thought.

After all, the footer is also completely configured from the top menu, so, murdering that bottom toggle with fire & brimstone and leaving everything to the top menu does make some kind of sense?

[protist]

@NiLuJe TBH when I read the release notes, I just went to the top menu, analogous to configuring the bottom status bar. That felt like more of a natural place for me.

[hius07]

Yeah, the first menu item "About..." can be replaced with a checkbox "Enable..." to avoid the 2nd page