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. |
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... |
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?
But only build the default mapping list once.
There was a desync of the charpos or something, which caused del/ret to delete stuff at the old position. Del is still wonky in that it behaves like return when at the end of the line.
e.g., Del doesn't do weird stuff anymore, and Home/End move across the *current* line.
So, don't do it, because I don't think we need a deep copy at all her,e seeing as this is already an instance member, not a class one.
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.
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)
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