Not a fan of device/DeviceInfo.kt's code, to put it mildly. Those booleans and weird temporary hash maps…

• are the TOLINO and TOLINO_EPOS3 booleans exclusive?
• what about other TOLINO_XXX booleans?
• EINK is never set to EinkDevice.TOLINO_VISION6, but checked in device/EPDFactory.kt
• same with HANVON_960 & HYREAD_MINI6
• LIGHTS is never set to LightsDevice.ONYX_NOTE_PRO, but checked in device/LightsFactory.kt
• LIGTS can be set to LightsDevice.ONYX_PALMA, but is never checked in device/LightsFactory.kt

Tentative rework (diff).

Not a fan of device/DeviceInfo.kt's code, to put it mildly. Those booleans and weird temporary hash maps…

Yeah, it was good enough for a few devices but it doesn't scale :)

AFAICT all identifiers are exclusive except NOOK and TOLINO which are used as a catch them all for EPD routines.

Tentative rework

It looks that you got rid of LightsDevice and QuirksDevice. Maybe search and replace too much? :)

Not a fan of device/DeviceInfo.kt's code, to put it mildly. Those booleans and weird temporary hash maps…

Yeah, it was good enough for a few devices but it doesn't scale :)

AFAICT all identifiers are exclusive except NOOK and TOLINO which are used as a catch them all for EPD routines.

How is that suppose to work with the deviceMap hashmap? Since the loop only honors the first match?

Also, is the iteration order deterministic? Does it matches the insertion order? The official documentation is confusing: the keys accessor is an unordered set, but with a reversed method, so I'm assuming the iteration order is not random… ¯\(ツ)/¯

Tentative rework

It looks that you got rid of LightsDevice and QuirksDevice. Maybe search and replace too much? :)

Nope, both gone, see the diff.

How is that suppose to work with the deviceMap hashmap? Since the loop only honors the first match?

I don't understand the question :). It honors the first match. It will return whatever controller or fallback to default if none.

Also, is the iteration order deterministic? Does it matches the insertion order?

There is no gurantee elements will appear in the specific order they were insterted. They should in a case like this, IIRC.

If that's what you want you might want to look at https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/-linked-hash-map/.

But why?

AFAICT there's no device with two or more different controllers for the same kind of driver.

Nope, both gone, see the diff.

Ok, sorry. That totally works for me if you think it is easier to maintain. Maybe rename DeviceInfo.EinkDevice to Device.Id since that's what the variable holds?

Isn't it a little weird to put all these TOLINO variant stuff in this generic Device file? https://github.com/benoit-pierre/android-luajit-launcher/blob/c41aa5ee8ee4c6b0c4557f7daf02dde64872a4aa/app/src/main/java/org/koreader/launcher/device/Device.kt#L41C1-L49C52

How is that suppose to work with the deviceMap hashmap? Since the loop only honors the first match?

I don't understand the question :). It honors the first match. It will return whatever controller or fallback to default if none.

If both the TOLINO & TOLINO_EPOS3 can be true, then that code is problematic.

Also, is the iteration order deterministic? Does it matches the insertion order?

There is no gurantee elements will appear in the specific order they were insterted. They should in a case like this, IIRC.

If that's what you want you might want to look at https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/-linked-hash-map/.

But why?

See above.

AFAICT there's no device with two or more different controllers for the same kind of driver.

Nope, both gone, see the diff.

Ok, sorry. That totally works for me if you think it is easier to maintain. Maybe rename DeviceInfo.EinkDevice to Device.Id since that's what the variable holds?

OK.

Isn't it a little weird to put all these TOLINO variant stuff in this generic Device file? https://github.com/benoit-pierre/android-luajit-launcher/blob/c41aa5ee8ee4c6b0c4557f7daf02dde64872a4aa/app/src/main/java/org/koreader/launcher/device/Device.kt#L41C1-L49C52

Yeah, I don't like that is_tolino flag, a dedicated quirk would be cleaner.

Isn't it a little weird to put all these TOLINO variant stuff in this generic Device file? https://github.com/benoit-pierre/android-luajit-launcher/blob/c41aa5ee8ee4c6b0c4557f7daf02dde64872a4aa/app/src/main/java/org/koreader/launcher/device/Device.kt#L41C1-L49C52

Yeah, I don't like that is_tolino flag, a dedicated quirk would be cleaner.

I'm all in for a refactor. After a quick search I'm not sure if they're still required. But if they do they're better handled elsewhere.

If the only usage is https://github.com/koreader/koreader/blob/1e55dda4c765be3316d7ca87be3052943cb1be86/frontend/device/android/device.lua#L141C3-L145C8 we could get rid of it completely.

It should be moved to the wiki as yet another example in https://github.com/koreader/koreader/wiki/Android-tips-and-tricks#customize-keys

If both the TOLINO & TOLINO_EPOS3 can be true, then that code is problematic.

either a TOLINO_EPOS3 is not catched as a TOLINO or the TOLINO_EPOS3 shouldn't be a valid EinkDevice ¿?

But yeah, that's ultra annoying to deal with. Thanks for the refactor 👍

If both the TOLINO & TOLINO_EPOS3 can be true, then that code is problematic.

either a TOLINO_EPOS3 is not catched as a TOLINO or the TOLINO_EPOS3 shouldn't be a valid EinkDevice ¿?

But yeah, that's ultra annoying to deal with. Thanks for the refactor 👍

There is a bug in the code. TOLINO_EPOS3 uses NGL4EPDController in EPDFactory.kt. A similar problem exists for TOLINO_VISION6.
Maybe we need to add an exclusion for the TOLINO match. But by any lucky these devices are not identified by STR_TOLINO BRAND/MODEL as the case of contentEquals("tolino_vision2").

EDIT:

Seems to be the case not catched by the // Tolino (catch them all)

Tolino Epos 3
Android 8.1.0
Eink driver: Nook gl4
Light driver: found none
Device info:
Manufacturer: rakuten kobo inc.
Brand: rakutenkobo
Model: tolino epos 3
Device: tolino
Product: tolino
Hardware: sun8iw15p1
Platform: virgo

Device info:
Manufacturer: rakuten kobo inc.
Brand: rakutenkobo
Model: tolino vision 6
Device: tolino
Product: tolino
Hardware: sun8iw15p1
Platform: virgo

        // Tolino (catch them all)
        TOLINO = !MODEL.contentEquals("tolino epos 3") && !MODEL.contentEquals("tolino vision 6")
        
        &&
        
        (
        
			BRAND.contentEquals(STR_TOLINO) && MODEL.contentEquals("imx50_rdp")
            || MODEL.contentEquals(STR_TOLINO) && (DEVICE.contentEquals("tolino_vision2")
            || DEVICE.contentEquals(STR_NTX))
        )

In any case, the TOLINO check is now done after more targeted TOLINO_XXX checks.

If the only usage is https://github.com/koreader/koreader/blob/1e55dda4c765be3316d7ca87be3052943cb1be86/frontend/device/android/device.lua#L141C3-L145C8 we could get rid of it completely.

It should be moved to the wiki as yet another example in https://github.com/koreader/koreader/wiki/Android-tips-and-tricks#customize-keys

Agreed, I updated the wiki page.

superseded by #526