Clarify our OOP semantics across the codebase#9586
Conversation
|
Looks a bit scary :) v2022.10 or v2022.11 ?
What is the 1% ? Thank you for having removed all these noisy |
I... don't think we actually do it anywhere, but it obviously still works, because a Widget is still a table, after all ;). e.g., Which is a terrible example because that just means SubSubWidget == SubWidget but with a different name ;p. |
Dealer's choice ;). (I've been running with it applied for a couple days and nothing's blown up so far, but I perfectly understand if we want to wait a bit ;)). |
AFAICT, everything properly instantiates w/ new, so we're good. (Also, in quite a few of the classes involved, new actually == extend; c.f., CacheItem for example where I made that explicit, but a number of classes had a new method that didn't do anything more than setup the inheritance chain). There are a few oddities, LuaSettings & co instantiating w/ open, for instance (ha!); or Device being a singleton, meaning init gets called at module scope. |
|
Don't have internet yet, some issue with the fiber in my street, so might as well throw it in. |
NiLuJe
left a comment
There was a problem hiding this comment.
Reviewed 201 of 202 files at r1, 1 of 1 files at r2, 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Frenzie, @pazos, and @poire-z)
|
I sure would like to know what's got CircleCI's panties in a bunch, but it's been throwing errors 500 for a few days over here... |
NiLuJe
left a comment
There was a problem hiding this comment.
Reviewed 35 of 37 files at r4, 4 of 4 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Frenzie, @pazos, and @poire-z)
You had me worried - but that's indeed just the Home and End physical keyboard keys. |
Looked, and I agree (in case you need confirmation :) |
All of the above? ;o) (And I'm trying to keep to stuff mostly related to this PR. e.g., parts of the dimen stuff was an old, old workaround... because of base classes being registered via new, which meant init doing funky stuff to base classes ;)). Let's say I'm cautiously optimistic it won't break too many things ;p. |
They're implementations, not instantiated objects. Part I: frontend/ui (i.e., Widgets).
It's been dead code since koreader/android-luajit-launcher#283
We very, very, very, very rarely actually feed an existing Widget object to extend (because that way lies inheritance madness). In 99% of the cases, we pass it the class object of our new subclass, which is just a table with its dedicated or overloaded static fields.
Instances are created via open
i.e., with inheritance magic. Also, simplify the LuaData API, that extra random table just to pass a single field name was weird?
InputContainer/WidgetContainer: If necessary, compute self.dimen at paintTo time, like most of our widgets OverlapGroup: Compute self.dimen at *init* time, because for some reason it needs to do that, but do it directly in OverlapGroup instead of going through a weird WidgetContainer method that it was the sole user of.
Doesn't appear to be any rhyme or reason re: init vs. paintTo, wheee! Anyway, fill the Geom tables we create to make sure we won't need to grow it later while I'm there.
NiLuJe
left a comment
There was a problem hiding this comment.
Reviewed 3 of 7 files at r7, 3 of 4 files at r8, 3 of 3 files at r9, 5 of 8 files at r10, 27 of 27 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Frenzie, @pazos, @poire-z, and @zwim)
* Minor cosmetic tweaks to ffi/pic to follow our usual class semantics
|
Crash when Wikipedia lookup: Dunno how you'll want to fix that : :new ? re-set all the ReaderDict fields in ReaderWikipedia ? Call super/ReaderDict:init() ? |
|
@poire-z: I was a bit unclear on what exactly the |
|
Okay, yeah, looking at DictQuickLookup, it needs to be a static class member (if only for the "close all open windows on hold" feature), I'll fix that ;). |
Also, make references are actually dropped no matter how the widget is closed by relying on onCloseWidget ;). Enable the "long-press-on-close" on the actual Close button, too, instead of only on the title bar's cross. Fix: koreader#9586 (comment)
Also, make sure references are actually dropped, no matter how the widget is closed, by relying on onCloseWidget ;). Enable the "long-press-on-close" trick on the actual Close button, too, instead of only on the title bar's cross. Fix: koreader#9586 (comment)
Also, make sure references are actually dropped, no matter how the widget is closed, by relying on onCloseWidget ;). Enable the "long-press-on-close" trick on the actual Close button, too, instead of only on the title bar's cross. Fix: #9586 (comment)
Works when a scrollable has not yet scrolled. Once you scroll and the first line is not the top line, all go messy :) Home moves some lines up, End too. function TextBoxWidget:moveCursorHome()
self:moveCursorToCharPos(self.vertical_string_list[self.current_line_num] and self.vertical_string_list[self.current_line_num].offset or #self.charlist)
endLooking at fixing that. - self.current_line_num = screen_line_num
+ self.current_line_num = screen_line_num + self.virtual_line_num - 1 |
|
^ will PR that fix, there are a few minor other related cleanups needed. |
Basically: * Use `extend` for class definitions * Use `new` for object instantiations That includes some minor code cleanups along the way: * Updated `Widget`'s docs to make the semantics clearer. * Removed `should_restrict_JIT` (it's been dead code since koreader/android-luajit-launcher#283) * Minor refactoring of LuaSettings/LuaData/LuaDefaults/DocSettings to behave (mostly, they are instantiated via `open` instead of `new`) like everything else and handle inheritance properly (i.e., DocSettings is now a proper LuaSettings subclass). * Default to `WidgetContainer` instead of `InputContainer` for stuff that doesn't actually setup key/gesture events. * Ditto for explicit `*Listener` only classes, make sure they're based on `EventListener` instead of something uselessly fancier. * Unless absolutely necessary, do not store references in class objects, ever; only values. Instead, always store references in instances, to avoid both sneaky inheritance issues, and sneaky GC pinning of stale references. * ReaderUI: Fix one such issue with its `active_widgets` array, with critical implications, as it essentially pinned *all* of ReaderUI's modules, including their reference to the `Document` instance (i.e., that was a big-ass leak). * Terminal: Make sure the shell is killed on plugin teardown. * InputText: Fix Home/End/Del physical keys to behave sensibly. * InputContainer/WidgetContainer: If necessary, compute self.dimen at paintTo time (previously, only InputContainers did, which might have had something to do with random widgets unconcerned about input using it as a baseclass instead of WidgetContainer...). * OverlapGroup: Compute self.dimen at *init* time, because for some reason it needs to do that, but do it directly in OverlapGroup instead of going through a weird WidgetContainer method that it was the sole user of. * ReaderCropping: Under no circumstances should a Document instance member (here, self.bbox) risk being `nil`ed! * Kobo: Minor code cleanups.
Also, make sure references are actually dropped, no matter how the widget is closed, by relying on onCloseWidget ;). Enable the "long-press-on-close" trick on the actual Close button, too, instead of only on the title bar's cross. Fix: koreader#9586 (comment)
Basically:
extendfor class definitionsnewfor object instantiationsThat includes some minor code cleanups along the way:
Widget's docs to make the semantics clearer.should_restrict_JIT(it's been dead code since Tackle mcode_alloc issues once and for all android-luajit-launcher#283)openinstead ofnew) like everything else and handle inheritance properly (i.e., DocSettings is now a proper LuaSettings subclass).WidgetContainerinstead ofInputContainerfor stuff that doesn't actually setup key/gesture events.*Listeneronly classes, make sure they're based onEventListenerinstead of something uselessly fancier.active_widgetsarray, with critical implications, as it essentially pinned all of ReaderUI's modules, including their reference to theDocumentinstance (i.e., that was a big-ass leak).niled!(Lots of files touched, but mostly trivial cosmetic tweaks, most actual code changes are limited to LuaSettings & friends).
Pairs with koreader/koreader-base#1525
Sponsored by
rg '^local [[:alnum:]_\-]+ = [[:alnum:]_\-]+:new'&rg "^[[:blank:]]{4}[^.[:blank:]]+[[:blank:]]*=[[:blank:]]*\{\}";p.This change is