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

[feat, plugin] Add AutoScroll #5295

Merged
merged 20 commits into from Sep 1, 2019

Conversation

@Frenzie
Copy link
Member

commented Sep 1, 2019

Fixes #3019.

@Frenzie Frenzie added this to the 2019.09 milestone Sep 1, 2019

}

function AutoScroll:_enabled()
return self.auto_scroll_sec > 0

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

This makes more sense to me as a completely separate enabled setting, but for starters I just wanted to get the base functionality working, and deviating from AutoSuspend would've taken more time.

Frenzie added 2 commits Sep 1, 2019
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2019

No issue with the whole thing.
May be just the name, "Autoscroll", as it doesn't scroll but turn pages :) "Autoturnpage" ? :)
If we ever want to add a real autoscroll (that would "GotoViewRel", 0.01 every second, if that ever works), there would be some conflict (unless this could fit into this plugin).

Anyway, if the plugin is named autoscroll, and that setting gets into the main settings store, better name it with the plugin name as prefix, that is: autoscroll_timeout_seconds instead of auto_scroll_timeout_seconds.


local delay

if PluginShare.pause_auto_scroll then

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 1, 2019

Contributor

A bit unclear to me: are you using that evil BackgroundRunner? Or doing the UIManager.schedule / unschedule yourself?
(looks like the later - but that PluginShare made me thought initially you went with BackgroundRunner).
Anyway, is that PluginShare.pause_auto_scroll needed ? A left-over from copied plugin? Something for the future to stop it from elsewhere?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

Oh yeah, a vestigial function. This is a KeepAlive hook for the AutoSuspend plugin.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

Come to think of it, it should still be kept. This one and AutoSuspend reset with an InputEvent hook, but this plugin doesn't emit input events, only GotoViewRel.

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2019

If we ever want to add a real autoscroll (that would "GotoViewRel", 0.01 every second, if that ever works), there would be some conflict (unless this could fit into this plugin).

I see that as an expansion of this plugin, like set scroll distance or something.

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2019

@poire-z Any further comments?

value_max = 20,
precision = "%.2f",
value_step = .1,
value_hold_step = .5,

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 1, 2019

Contributor

So, 1 = 1 page, 0.3 = 30% of the page?
Do value_min/max = 20, or anything > 1, has some use case?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

I think so, yes. Or if not it's close enough for it not to matter. One would just decrease or increase the value to achieve the desired effect.

I just noticed something odd though. In paged mode fractional values result in an instant end of book dialog. And related, sending this event when one's already showing pops up more on top of it.

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 1, 2019

Contributor

What happens when you're doing a dict lookup and enjoying the result? The page still scroll in the background without any issue?

Might need to pause this autoscroll whenever the readerview is not the top one ? :|

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

What happens when you're doing a dict lookup and enjoying the result? The page still scroll in the background without any issue?

If you're doing it really slowly without interacting, sure.

Might need to pause this autoscroll whenever the readerview is not the top one ? :|

Is that currently possible?

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 1, 2019

Contributor

Dunno :) probably not.
Might need to ask UIManager if there's something in the stack above the reader...
Or have UIManager send some event when covered/uncovered, when a widget has requested it (and give the names of the events, or some fixed event with the widget name/id :)
Probably quite some work, for a little plugin :) (pinging @NiLuJe if he sees something easy.)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 1, 2019

Member

The least intrusive would probably be to add something to the effect of "am I on top of the stack", and query that from the plugin as-needed?

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 1, 2019

Member

(That might have to be sneakier: am I the highest visible widget on the stack).

Or not, Been a while since I played w/ UIManager.

Otherwise, possibly as easy as checking if self._window_stack[1].widget == widget ;).

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

The top of the stack is probably queried easily enough, but ReaderUI isn't the top of the stack. There's statusbars, bookmarks, and all kinds of stuff like that on top of it.

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 1, 2019

Contributor

Or just a simple method UIManager:getTopWidgetId() => "reader" or "#1234ab32" that the widget could just call before deciding to send the onGotoViewRel.


local InfoMessage = require("ui/widget/infomessage")
UIManager:show(InfoMessage:new{
text = T(_("Autoscroll is now active and will automatically turn the page every %1 seconds."), self.autoscroll_sec),

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 1, 2019

Contributor

"turn the page" - but we now can turn some fraction of the page :)
So, just "scroll", or if distance == 1 then "turn the page" else T("scroll %1%% of the page", distance) ?

Frenzie added 5 commits Sep 1, 2019
@@ -369,6 +369,8 @@ function UIManager:close(widget, refreshtype, refreshregion, refreshdither)
for i = 1, #self._window_stack do
self:setDirty(self._window_stack[i].widget)
end
-- Inform everyone who's the boss now.
self:broadcastEvent(Event:new("TopWidget", ((self._window_stack[#self._window_stack] or {}).widget or {}).name))

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

@poire-z @NiLuJe Feels a bit dirty but it seems to work. Any suggestions on a better way? Instead of unscheduling the plugin on an event there could also be a method that's checked before turning the page or something.

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 1, 2019

Member

How much do we end up firing that TopWidget event, ultimately?

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 1, 2019

Contributor

How much do we end up firing that TopWidget event

Each time we UIManager:show() a widget, which is not that much, but it goes thru all the widgets to broadcast it. Which might be negligeable, I don't know, but still :)

Instead of unscheduling the plugin on an event there could also be a method that's checked before turning the page or something.

Like I suggested above?:

Or just a simple method UIManager:getTopWidgetId() => "ReaderUI" or "#1234ab32" that the widget could just call before deciding to send the onGotoViewRel.

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 1, 2019

Contributor

(Why did you remove the one on :show() and kept only the one on :close() ? )

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

How much do we end up firing that TopWidget event, ultimately?

Not much at all. So it's distributing the event to all widgets every once in a while vs always checking the top widget every x seconds. But of course this event implementation would affect everything and the plugin just users of the plugin, so out the event will go. ;-)

Why did you remove the one on :show() and kept only the one on :close() ? )

It wasn't working right, ran into some kind of loop.

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 1, 2019

Contributor

(Well, the event could have some other usages, like stopping statistics, to get a more accurate time to read a page.
Unless all the little times we spend dict or wikipedia lookup'ing need to be accounted in this, assuming it's part of the reading time :)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

I don't know, I think the statistics are pretty cool but I'm not very concerned about their accuracy personally.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 1, 2019

Author Member

@poire-z Btw, that issue I noticed with it continuously popping up end of document dialogs was magically resolved by your suggestion, because now it automatically pauses when that dialog is up. :-)

@AlanSP1

This comment has been minimized.

Copy link

commented Sep 1, 2019

To me, automatic page turning, or autopageturner sounds better...

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2019

@AlanSP1 Possibly, but note that autoscroll is a "traditional" term. See, e.g., here. Then again, the same applies to page turner.

Frenzie added 3 commits Sep 1, 2019
@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2019

Alright, final screens (sorry, the others show the wrong menu position and I didn't notice until after I took the screenshots ;-) ):


@poire-z
poire-z approved these changes Sep 1, 2019
Copy link
Contributor

left a comment

(Auto-turn pages in the menu would have been clearer - but ok if you want to stay with "Autoturn" as the name of a thing/feature.)

@Frenzie Frenzie merged commit 4cdc3ab into koreader:master Sep 1, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:autoscroll branch Sep 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.