Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A bundle of tweaks #9691

Merged
merged 15 commits into from
Oct 29, 2022
Merged

A bundle of tweaks #9691

merged 15 commits into from
Oct 29, 2022

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Oct 27, 2022

Depends on koreader/koreader-base#1544 & koreader/koreader-base#1545


This change is Reviewable

@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 27, 2022

(Rebase me).

First commit ended up sliiightly scarier than I expected, but... 'tis the season :D.

@poire-z
Copy link
Contributor

poire-z commented Oct 27, 2022

(Approved as "ok on the principle", not that I approve the unit tests failing :)

@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 27, 2022

Yeah, unit tests should be fixed by the base bump ;p.

@hius07
Copy link
Member

hius07 commented Oct 27, 2022

Side question. Should we use such alignment more often? (looks good)

self.ges_events = {
ScrollableTouch = not ignore.touch and { GestureRange:new{ ges = "touch", range = range } } or nil,
ScrollableSwipe = not ignore.swipe and { GestureRange:new{ ges = "swipe", range = range } } or nil,
ScrollableHold = not ignore.hold and { GestureRange:new{ ges = "hold", range = range } } or nil,
ScrollableHoldPan = not ignore.hold_pan and { GestureRange:new{ ges = "hold_pan", range = range } } or nil,
ScrollableHoldRelease = not ignore.hold_release and { GestureRange:new{ ges = "hold_release", range = range } } or nil,
ScrollablePan = not ignore.pan and { GestureRange:new{ ges = "pan", range = range } } or nil,
ScrollablePanRelease = not ignore.pan_release and { GestureRange:new{ ges = "pan_release", range = range } } or nil,
}

@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 27, 2022

I mean, I'd be all for it, as that how I setup clang-format for C stuff ;o).

Here, the pattern kinda screamed for it, though, so it was an easy decision (by which I mean, it's a very neat series ;)).

@NiLuJe NiLuJe changed the title A couple of tweaks A bundle of tweaks Oct 27, 2022
@NiLuJe NiLuJe force-pushed the master branch 2 times, most recently from 665052a to 30b33a3 Compare October 27, 2022 22:52
@NiLuJe NiLuJe force-pushed the master branch 3 times, most recently from f39b852 to 07b9667 Compare October 28, 2022 17:44
Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 79 of 84 files at r1, 4 of 4 files at r3, 2 of 2 files at r4, 7 of 7 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Frenzie, @pazos, and @poire-z)

Get rid of the doc & seqtext fields, as they are not actually used (nor
are they particularly useful, the event handler's name should be pretty
self-explanatory).

Also, tweak the key_events documentation to highlight the quirks of the
API, especially as far as array nesting is involved...

Random drive-by cleanup of the declarations of key_events & ges_events
to re-use the existing instance object (now that we know they're sane
;p) for tables with a single member (less GC pressure).
(i.e., do the usual flush & fsync dance, because FAT32 is the worst).
& Nia).

And also disable the jump marker on those, like we did for the Libra 2.
Because apparently people are confused by the UIManager:quit ones...
standby logging messages to debug (except for warnings/errors, of course).
Otherwise, stuff that inherits logger functions on require don't get
logged (i.e., framebuffer), and the debug guards aren't hooked.
Tone it down for everyone, as it's been running smoothly for a while
now, but add a dedicated extra warning on buggy boards that it might
randomly implode.
(This involves moving it to the instance object to avoid inheritance).

Pocketbook: Disable rotation_map on the Era (fix koreader#9556)
It would appear that InkView handles the translation for us, now...
I'd completely forgotten they'd announced one ;o).
input modules.

There was a weird mix of touch being handled there, and key/msc here,
which was weird and made the logs extremely confusing to read.
defaults.persistent.lua

We were pcall'ing the parsing, but not the execution...
The funky Lua syntax quirks means that it is possible to pass the former
but not the latter ;).

Fix koreader#9700, de-facto regression since koreader#9546
Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Frenzie, @pazos, and @poire-z)

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Frenzie, @pazos, and @poire-z)

@NiLuJe NiLuJe merged commit c740ee3 into koreader:master Oct 29, 2022
-- deligate gesture listener to readerui
self.ges_events = {}
-- delegate gesture listener to readerui
self.ges_events = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NiLuJe
Regression

The 4 places you changed self.ges_events from {} to nil do not let me do any gestures in the reader (even swipe to open the menu)

./luajit: frontend/ui/widget/container/inputcontainer.lua:257: bad argument #1 to 'pairs' (table expected, got nil)

yparitcher added a commit to yparitcher/koreader that referenced this pull request Oct 30, 2022
yparitcher added a commit that referenced this pull request Oct 30, 2022
@hius07
Copy link
Member

hius07 commented Mar 23, 2023

(Writing here as it's one of the latest PR changing the Menu widget)

On a tap the coroutine is created:

function MenuItem:onTapSelect(arg, ges)
-- Abort if the menu hasn't been painted yet.
if not self[1].dimen then return end
local pos = self:getGesPosition(ges)
if G_reader_settings:isFalse("flash_ui") then
logger.dbg("creating coroutine for menu select")
local co = coroutine.create(function()
self.menu:onMenuSelect(self.table, pos)
end)
coroutine.resume(co)
else

This was introduced in #1637 to support OPDS authorization.
Since then OPDSBrowser has been changed a lot and now does not check for a coroutine existence.
So is this coroutine creation needed anymore?

@poire-z
Copy link
Contributor

poire-z commented Mar 23, 2023

Not really familiar with corountines (or what I learned about them when I needed to make Trapper, I have forgotten), but indeed, we never really need to use coroutines, so I guess if we'd rewrite this menu select code now, we wouldn't even think about adding anything coroutine in there :)
So, if everything keeps working without, nothing against cleaning that out.

@Frenzie
Copy link
Member

Frenzie commented Mar 23, 2023

I forgot that was in there. What that code is doing seems sensible for that kind of scenario though? What happens now? I don't really use OPDS much, let alone with authorization.

@hius07
Copy link
Member

hius07 commented Mar 23, 2023

What happens now?

Earlier there was an option to authorize to opds server manually via the dialog entering login/pwd.
Now login/pwd must be saved to the server properties, no online dialog for login exists.

@Frenzie
Copy link
Member

Frenzie commented Mar 23, 2023

If it's not used anywhere I suppose it should be fine to remove since then it's also somewhat untested after all. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants