hidden flows and custom toc removed from non-touch devices #11690

Commodore64user · 2024-04-18T00:03:19Z

options to set 'custom hidden flows' and 'custom ToC' clutter the menus of non-touch devices. There is no available 'page browser' therefore these options cannot be used in them.

Technically you could set the ToC up, but it is SUCH a pain in the *rse you might as well just remove it altogether.

this PR looks scary but all it is really doing is

if Device:isTouchDevice() then
    EXISTING CODE 
end

This change is

poire-z · 2024-04-18T06:32:36Z

this PR looks scary but all it is really doing is
if Device:isTouchDevice() then
    EXISTING CODE 
end

We care about not changing a lots of code just for indentation issues when not really needed (unlike with your other status bar or screensaver PRs where there is no alternative), to not mess git history and see who did change what when. This PR as-is would attribute all that code to you and now.
So, for this PR, you could just do at the end of that function after all the menu items are created:

if not Device:isTouchDevice() then
    -- Remove them as not really usable
    menu_items.handmade_toc = nil
    menu_items.handmade_hidden_flows  = nil
end

(it would be stuff created for nothing and some CPU wasted, but we don't care that much about non touch devices).

BUT:

Technically you could set the ToC up, but it is SUCH a pain in the *rse you might as well just remove it altogether.

People may have other kind of workflows, ie. when they want it do that on another device or their PC, and sync the docsettings to their devices, and get the feature, even if they don't/can't edit it on the NT device itself.

Frenzie · 2024-04-18T07:31:13Z

unlike with your other status bar or screensaver PRs where there is no alternative

Not entirely true, MenuSorter can be used internally even without taking everything global. (Granted, the conversion would still have to happen, but after that.)

So, for this PR, you could just do at the end of that function after all the menu items are created:

Since it seems to apply it to everything, an early return would make more sense (and not just from a diff perspective).

if touchDevice then
  return

Commodore64user · 2024-04-18T09:04:27Z

We care about not changing a lots of code just for indentation issues when not really needed (unlike with your other status bar or screensaver PRs where there is no alternative), to not mess git history and see who did change what when. This PR as-is would attribute all that code to you and now.

who wrote this code?

(it would be stuff created for nothing and some CPU wasted, but we don't care that much about non touch devices).

I do.

People may have other kind of workflows, ie. when they want it do that on another device or their PC, and sync the docsettings to their devices, and get the feature, even if they don't/can't edit it on the NT device itself.

trust me, no one is doing it. It is way, way, way too annoying (and easy to get wrong). It is not easy to navigate through a book on non-touch. It would be like threading a needle whilst tightrope walking, technically possible but no old ladies are doing it just to sew their grandchild's trousers.

Frenzie · 2024-04-18T09:08:18Z

frontend/apps/reader/modules/readerhandmade.lua

-                        UIManager:show(InfoMessage:new{
-                            text = _([[
-If the book has no table of contents or you would like to substitute it with your own, you can create a custom TOC. The original TOC (if available) will not be altered.
+    if Device:isTouchDevice() then


For an early return the code can be simplified like this:

Suggested change

if Device:isTouchDevice() then

if Device:isTouchDevice() then

return

end

We (or at least I) prefer that legibility-wise. :-)

if Device:isTouchDevice() then EXISTING CODE return end

that what you mean?

Nope. ^_^ I mean exactly what I wrote. None of the changes, only an early return.

Here's a random result that seems to offer a reasonable explanation as to why I often prefer it (if applicable): https://medium.com/swlh/return-early-pattern-3d18a41bba8

The fact that it also solves the parallel diff/blame discussion is merely incidental to me but it happens to solve that as well. ;-)

Return means it returns a value to the caller. Nothing can happen after that.

if Device:isTouchDevice() then return end EXISTING CODE

but then the whole thing would run regardless, again pardon my inexperience.

Nothing under return will run. But I see where I caused confusion, I forgot to update the condition as appropriate.

Suggested change

if Device:isTouchDevice() then

if not Device:isTouchDevice() then

return

end

I see, incidentally though you were suggesting to remove them from touch enabled devices 🤓. would have caused quite the stir.

We generally speak in pseudo-code, sorry for the confusion. It might accidentally be exactly right but it's not expected to be.

Commodore64user · 2024-04-18T09:09:48Z

what if I do something naughty and not indent?

if Device:isTouchDevice() then
EXISTING CODE 
end

not pretty but would solve the attribution and wasted memory issues. Then the person who originally wrote it could go and make it pretty again?

Frenzie

lgtm

Commodore64user · 2024-04-18T10:28:16Z

is there going to be a stable release this month?

poire-z · 2024-04-18T10:35:40Z

It's not really about the attribution, rather the history and what you see when you hit "Blame":

For this recent readerhandmade.lua, there was not much activity, but try it on other older modules:

This is one of the most valuable feature of git - so when you don't understand something, you can get there to see when and why it was introduced and follow the related past changes and further fixes.
Of course, when we refactor, all that goes away - so the refactoring must be worth it.

Frenzie · 2024-04-18T10:37:41Z

is there going to be a stable release this month?

If you're asking before the weekend then definitely no, for this month I don't know yet.

frontend/apps/reader/modules/readerhandmade.lua

poire-z · 2024-04-18T10:47:08Z

frontend/apps/reader/modules/readerhandmade.lua

    self:updateHandmagePages()
    -- Don't have these send events their own events
-    self:setupFlows(true)
-    self:setupToc(true)
+    if Device:isTouchDevice() then
+        self:setupFlows(true)
+        self:setupToc(true)
+    end
    -- ReaderToc will process this event just after us, and will


Do these 2 self:setupXyz() (but not self:updateHAndmagePages()?) really cause issues when called when nothing has been setup because there was no menu item to do so?
If none, I would just let that be as it was.
(And may be this won't break workflows such as the one I mentionned.)

~~the setupFlows definitely caused issues. I can be more specific if you want.~~

I take that back, in its original form it would complain about setupFlows, with the early return it doesn't...?

fixed now, just for you.

frontend/apps/reader/modules/readerhandmade.lua

hidden flows removed from non-touch

cbd5534

Frenzie reviewed Apr 18, 2024

View reviewed changes

return early

6c921ea

Frenzie approved these changes Apr 18, 2024

View reviewed changes

Frenzie added this to the 2024.04 milestone Apr 18, 2024

Frenzie added the NT Non Touch devices label Apr 18, 2024

poire-z reviewed Apr 18, 2024

View reviewed changes

frontend/apps/reader/modules/readerhandmade.lua Show resolved Hide resolved

poire-z reviewed Apr 18, 2024

View reviewed changes

Commodore64user added 2 commits April 18, 2024 12:07

comment added

ff1709d

revert self:setups back to original state

2a2c3ce

Frenzie reviewed Apr 18, 2024

View reviewed changes

frontend/apps/reader/modules/readerhandmade.lua Outdated Show resolved Hide resolved

add a full stop/period

a596b89

Frenzie merged commit c70c9f0 into koreader:master Apr 18, 2024
1 of 3 checks passed

Commodore64user deleted the non-touch-hidden-flows branch April 22, 2024 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hidden flows and custom toc removed from non-touch devices #11690

hidden flows and custom toc removed from non-touch devices #11690

Commodore64user commented Apr 18, 2024 •

edited by Frenzie

poire-z commented Apr 18, 2024

Frenzie commented Apr 18, 2024 •

edited

Commodore64user commented Apr 18, 2024 •

edited

Frenzie Apr 18, 2024

Commodore64user Apr 18, 2024

Frenzie Apr 18, 2024

Frenzie Apr 18, 2024

Frenzie Apr 18, 2024

Frenzie Apr 18, 2024

Commodore64user Apr 18, 2024

Frenzie Apr 18, 2024

Commodore64user Apr 18, 2024

Frenzie Apr 18, 2024

Commodore64user commented Apr 18, 2024 •

edited

Frenzie left a comment

Commodore64user commented Apr 18, 2024

poire-z commented Apr 18, 2024

Frenzie commented Apr 18, 2024

poire-z Apr 18, 2024

Commodore64user Apr 18, 2024 •

edited

Commodore64user Apr 18, 2024

hidden flows and custom toc removed from non-touch devices #11690

hidden flows and custom toc removed from non-touch devices #11690

Conversation

Commodore64user commented Apr 18, 2024 • edited by Frenzie

poire-z commented Apr 18, 2024

Frenzie commented Apr 18, 2024 • edited

Commodore64user commented Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Commodore64user commented Apr 18, 2024 • edited

Frenzie left a comment

Choose a reason for hiding this comment

Commodore64user commented Apr 18, 2024

poire-z commented Apr 18, 2024

Frenzie commented Apr 18, 2024

Choose a reason for hiding this comment

Commodore64user Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Commodore64user commented Apr 18, 2024 •

edited by Frenzie

Frenzie commented Apr 18, 2024 •

edited

Commodore64user commented Apr 18, 2024 •

edited

Commodore64user commented Apr 18, 2024 •

edited

Commodore64user Apr 18, 2024 •

edited