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

Dictionary changes on 4.20.14601 #49

Closed
pgaskin opened this issue Mar 3, 2020 · 22 comments
Closed

Dictionary changes on 4.20.14601 #49

pgaskin opened this issue Mar 3, 2020 · 22 comments
Assignees
Labels
discussion Category: Discussion/question (i.e. not anything specific).

Comments

@pgaskin
Copy link
Owner

pgaskin commented Mar 3, 2020

Continuing from #48.

@pgaskin pgaskin added the discussion Category: Discussion/question (i.e. not anything specific). label Mar 3, 2020
@jackiew1
Copy link
Collaborator

jackiew1 commented Mar 3, 2020

@geek1011
Better late than never! What did you have in mind for the dictionary testing you asked about 16 hours ago?

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 3, 2020

Specifically how the extra dictionaries are handled differently without the patch enabled.

I've already tested them with the patch enabled, and I've tested the behavior of images and stylesheets (both of which finally work properly).

It might also be useful to test if is possible to include an additional name in any way, but that's something which is easier to figure out by looking at the firmware itself.

@jackiew1
Copy link
Collaborator

jackiew1 commented Mar 3, 2020

I have a completely unpatched 4.20 KA1. Extra-type dictionaries are listed in the dict drop-down selection list, e.g.

Extra: cl
Extra: co

However, if you try to select one of them it just pops up an errmsg You don't have an English dictionary installed with a button to the Manage dictionaries settings.

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 3, 2020

I don't recall how, but last night, I was able to select a sideloaded dictionary (I think it was an Extra:_ one) without the patches, but I wasn't able to switch away from it.

@jackiew1
Copy link
Collaborator

jackiew1 commented Mar 3, 2020

I just tried again. I can get a sideloaded -en-en dictionary to lookup words but not an Extra.

As with the Extras, the drop-down list label shows as a truncated English - English rather than the actual full label of English - English (OED_ed3)

@taosxx
Copy link

taosxx commented Mar 4, 2020

but I wasn't able to switch away from it.

I've seen this a couple of times before (starting with firmware 4.19.x). Happened before and after patching. Issue was always gone after a reboot and re-selecting the other dictionary.

I just tried again. I can get a sideloaded -en-en dictionary to lookup words but not an Extra.

For me, before patching, all side-loaded dictionaries with "known" language codes (those that are mentioned in libnickel) worked, e.g. -fr-de, -da-en, -it-fr, -fr-fr, -de-de, etc. As expected, they were shown in the drop-down menu with their full names: Français - Deutsch, Dansk - English, etc. I haven't checked if these dictionaries must be added to the database before they will work (as far as I remember this wasn't necessary in older firmwares).

Side-loaded dictionaries with "extra" languages (those that have to be added in Kobo eReader.conf) were shown as e.g Extra: la in the drop-down menu. Going to Manage dictionaries after the error message You don't have an English - English dictionary installed, for the first time ever, actually opened my -en-en dictionary.... After patching, looking up a word worked, of course.

BTW, I noticed that if I choose to change my system language to one of the (three in my case) extra languages, the first two extra languages are shown as activated in the language settings menu (I have only one translation file installed). Not sure if this is new.

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 5, 2020

Slightly off topic, but I've released the first stable version of dictutil and created MR:thread-327854.

@PescheHelfer

This comment has been minimized.

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 7, 2020

^ pgaskin/dictutil#3

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 8, 2020

While looking into pgaskin/dictutil#3, I found something new!

It seems that you don't need funny tricks to make nickel preserve overwritten dictionaries anymore. Now, all you need to do is mark it read only!

I'll be updating dictutil with a case for this if the firmware version is 4.20.14601+.

                            loc_60f152:
0060f152 0133                   adds       r3, #0x1                             ; CODE XREF=_ZN23SyncDictionariesCommand19prepareDownloadListEv+312
0060f154 3FF4C4AE               beq.w      loc_60eee0

                            loc_60f158:
0060f158 50E8003F               ldrex      r3, [r0]                             ; CODE XREF=_ZN23SyncDictionariesCommand19prepareDownloadListEv+972
0060f15c A3F10103               sub.w      r3, r3, #0x1
0060f160 40E80032               strex      r2, r3, [r0]
0060f164 92F0000F               teq.w      r2, #0x0
0060f168 F6D1                   bne        loc_60f158

0060f16a 002B                   cmp        r3, #0x0
0060f16c 7FF4B8AE               bne.w      loc_60eee0

0060f170 786A                   ldr        r0, [r7, #0x24]
0060f172 B1E6                   b          loc_60eed8

                            loc_60f174:
0060f174 5046                   mov        r0, sl                               ; CODE XREF=_ZN23SyncDictionariesCommand19prepareDownloadListEv+284
0060f176 A6F634EC               blx        _ZNK10Dictionary8isSyncedEv@PLT      ; Dictionary::isSynced() const
0060f17a 0028                   cmp        r0, #0x0
0060f17c 3FF49EAE               beq.w      loc_60eebc

0060f180 7869                   ldr        r0, [r7, #0x14]
0060f182 ADF692EE               blx        _ZNK11SyncCommand5stateEv@PLT        ; SyncCommand::state() const
0060f186 B3F67EE8               blx        _ZN9SyncState6deviceEv@PLT           ; SyncState::device()
0060f18a 5146                   mov        r1, sl
0060f18c B4F64EEF               blx        _ZN21DictionaryFileManager23getDictionarySizeOnDiskERK6DeviceRK10Dictionary@PLT ; DictionaryFileManager::getDictionarySizeOnDisk(Device const&, Dictionary const&)
0060f190 0B46                   mov        r3, r1
0060f192 0246                   mov        r2, r0
0060f194 0128                   cmp        r0, #0x1
0060f196 C7E90223               strd       r2, r3, [r7, #0x8]
0060f19a 73F10003               sbcs       r3, r3, #0x0
0060f19e FFF68DAE               blt.w      loc_60eebc

0060f1a2 7869                   ldr        r0, [r7, #0x14]
0060f1a4 ADF680EE               blx        _ZNK11SyncCommand5stateEv@PLT        ; SyncCommand::state() const
0060f1a8 B3F66CE8               blx        _ZN9SyncState6deviceEv@PLT           ; SyncState::device()
0060f1ac 5146                   mov        r1, sl
0060f1ae CCF61AEF               blx        _ZN21DictionaryFileManager30getDictionaryPermissionsOnDiskERK6DeviceRK10Dictionary@PLT ; DictionaryFileManager::getDictionaryPermissionsOnDisk(Device const&, Dictionary const&)
0060f1b2 8304                   lsls       r3, r0, #0x12
0060f1b4 0CD5                   bpl        loc_60f1d0                           ; DATA XREF=_ZN22JavaScriptEventHandler13saveHighlightE5QHashI7QStringS1_E+446, dword_87ab40

0060f1b6 7B68                   ldr        r3, [r7, #0x4]
0060f1b8 042B                   cmp        r3, #0x4
0060f1ba 3FF424AF               beq.w      loc_60f006

0060f1be D7E90201               ldrd       r0, r1, [r7, #0x8]
0060f1c2 E317                   asrs       r3, r4, #0x1f
0060f1c4 9942                   cmp        r1, r3
0060f1c6 08BF                   it         eq
0060f1c8 A042                   cmpeq      r0, r4
0060f1ca 7FF477AE               bne.w      loc_60eebc

0060f1ce 1AE7                   b          loc_60f006

                            loc_60f1d0:
0060f1d0 D8F67CE9               blx        _Z9syncTracev@PLT                    ; syncTrace(), CODE XREF=_ZN23SyncDictionariesCommand19prepareDownloadListEv+1048
0060f1d4 037A                   ldrb       r3, [r0, #0x8]
0060f1d6 002B                   cmp        r3, #0x0
0060f1d8 3FF415AF               beq.w      loc_60f006

0060f1dc D8F676E9               blx        _Z9syncTracev@PLT                    ; syncTrace()
0060f1e0 E44B                   ldr        r3, =0x863750                        ; dword_60f574,0x863750
0060f1e2 07F12C0A               add.w      sl, r7, #0x2c
0060f1e6 E449                   ldr        r1, =0x8638f0                        ; dword_60f578,0x8638f0
0060f1e8 4268                   ldr        r2, [r0, #0x4]
0060f1ea 0120                   movs       r0, #0x1
0060f1ec 7B44                   add        r3, pc                               ; "void SyncDictionariesCommand::downloadFailed(int)"
0060f1ee 7865                   str        r0, [r7, #0x54]
0060f1f0 7944                   add        r1, pc                               ; "/export/jenkins/n/Nickel/src/io/sync/commands/SyncDictionariesCommand.cpp"
0060f1f2 7033                   adds       r3, #0x70                            ; "void SyncDictionariesCommand::prepareDownloadList()"
0060f1f4 7A66                   str        r2, [r7, #0x64]
0060f1f6 5046                   mov        r0, sl
0060f1f8 4722                   movs       r2, #0x47
0060f1fa F965                   str        r1, [r7, #0x5c]
0060f1fc 3146                   mov        r1, r6
0060f1fe 3B66                   str        r3, [r7, #0x60]
0060f200 BA65                   str        r2, [r7, #0x58]
0060f202 B9F6C6ED               blx        _ZNK14QMessageLogger5debugEv@PLT     ; QMessageLogger::debug() const
0060f206 2221                   movs       r1, #0x22
0060f208 F86A                   ldr        r0, [r7, #0x2c]
0060f20a AAF69AEA               blx        _ZN11QTextStreamlsEc@PLT             ; QTextStream::operator<<(char)
0060f20e 3969                   ldr        r1, [r7, #0x10]
0060f210 A6F6EAED               blx        _ZN11QTextStreamlsERK7QString@PLT    ; QTextStream::operator<<(QString const&)
0060f214 2221                   movs       r1, #0x22
0060f216 AAF694EA               blx        _ZN11QTextStreamlsEc@PLT             ; QTextStream::operator<<(char)
0060f21a FD6A                   ldr        r5, [r7, #0x2c]
0060f21c 2B7D                   ldrb       r3, [r5, #0x14]
0060f21e 002B                   cmp        r3, #0x0
0060f220 40F05981               bne.w      loc_60f4d6

                            loc_60f224:
0060f224 D549                   ldr        r1, =0x863926                        ; dword_60f57c,0x863926, CODE XREF=_ZN23SyncDictionariesCommand19prepareDownloadListEv+1860
0060f226 07F13804               add.w      r4, r7, #0x38
0060f22a 1E22                   movs       r2, #0x1e
0060f22c 2046                   mov        r0, r4
0060f22e 7944                   add        r1, pc                               ; "marked as read-only.. skipping"
0060f230 C4F674EF               blx        _ZN7QString15fromUtf8_helperEPKci@PLT ; QString::fromUtf8_helper(char const*, int)
0060f234 2146                   mov        r1, r4
0060f236 2846                   mov        r0, r5
0060f238 A6F6D6ED               blx        _ZN11QTextStreamlsERK7QString@PLT    ; QTextStream::operator<<(QString const&)
0060f23c B86B                   ldr        r0, [r7, #0x38]
0060f23e 0368                   ldr        r3, [r0]
0060f240 002B                   cmp        r3, #0x0
0060f242 00F03781               beq.w      loc_60f4b4

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 8, 2020

I've also confirmed pgaskin/dictutil#3 is valid! Firmware 4.20.14601 doesn't create the Dictionary table anymore. I guess there wasn't a migration (or it was broken), and that's why devices upgrading from an older version still have that table.

That would also explain the unusual behaviour with the Extra: stuff. Here's my theory (I'll test it later today):

  • Same: Nickel still generates locale names by default with Extra: LOCALE.
  • New: Nickel doesn't read the dictionary table anymore, so the name in it is ignored. In addition, entries in the table won't change anything even if it is still present.
  • New: The built-in dictionaries are hard-coded, rather than writing them to the db during migrations and reading from it at runtime.
  • Same: Nickel still has the bug where the locale splitting is messed up, so the Extra: LOCALE names are inherently broken.
  • Same: The matching can be fixed by replacing Extra: with Extra:_.
  • New: The database doesn't need to be changed anymore in addition to the patch, as the names are generated dynamically using the same string.
  • Therefore: If the dictionary table is present, it can safely be removed.
  • Therefore: The steps required to install custom dictionaries are now:
    • Copy the dictzip and mark it read-only.
    • Add it to ExtraLocales if it is not a built-in locale.
    • Replace Extra: in libnickel with any other string (same length or shorter with a null byte at the end), but not containing anything in the Unicode whitespace class.

I'll write some stuff about this for the dictutil docs after I test it.

@jackiew1, it would be nice if you could confirm this as well.

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 8, 2020

And here are the logs (I've replaced dicthtml.zip and added dicthtml-xx.zip, but made both read-only):

Mar  8 14:45:46 nickel: (  2518.512 @ 0x22514b0 / sync.debug) =================== SYNC QUEUE [ SyncClient(0x1836b78) ] ==================== 
Mar  8 14:45:46 nickel: (  2518.512 @ 0x22514b0 / sync.debug)  ----- Queued ------- 
Mar  8 14:45:46 nickel: (  2518.512 @ 0x22514b0 / sync.debug)  [ SyncDictionariesCommand(0x309018a0) ] 
Mar  8 14:45:46 nickel: (  2518.512 @ 0x22514b0 / sync.debug)  [ FrictionlessReadingCommand(0x30903500) ] 
Mar  8 14:45:46 nickel: (  2518.512 @ 0x22514b0 / sync.debug)  [ DownloadQueuedBooksCommand(0x30904720) ] 
Mar  8 14:45:46 nickel: (  2518.512 @ 0x22514b0 / sync.debug)  [ CleanupCommand(0x3092a0b8) ] 
Mar  8 14:45:46 nickel: (  2518.513 @ 0x22514b0 / sync.debug) =================================================== 
Mar  8 14:45:46 nickel: (  2518.513 @ 0x22514b0 / sync.debug)    
Mar  8 14:45:46 nickel: (  2518.513 @ 0x22514b0 / sync.debug) pumping with 4 pending commands and 0 active commands 
Mar  8 14:45:46 nickel: (  2518.513 @ 0x22514b0 / sync.debug) executing SyncDictionariesCommand(0x309018a0) 
Mar  8 14:45:46 nickel: (  2518.513 @ 0x22514b0 / sync.debug) virtual void SyncDictionariesCommand::execute() 
Mar  8 14:45:46 nickel: (  2518.730 @ 0x22514b0 / sync.debug) "dicthtml" marked as read-only.. skipping 
Mar  8 14:45:46 nickel: (  2519.101 @ 0x22514b0 / packetdump.warning) "https://kbdownload1-a.akamaihd.net/ereader/dictionaries/v2/dicthtml-xx.zip" => "Error downloading https://kbdownload1-a.akamaihd.net/ereader/dictionaries/v2/dicthtml-xx.zip - server replied: Not Found" 
Mar  8 14:45:46 nickel: (  2519.102 @ 0x22514b0) void FileSizeGetter::onFinished() FAILED! 203 "Error downloading https://kbdownload1-a.akamaihd.net/ereader/dictionaries/v2/dicthtml-xx.zip - server replied: Not Found" 
Mar  8 14:45:46 nickel: (  2519.102 @ 0x22514b0 / sync.warning) void SyncDictionariesCommand::prepareDownloadList() Get size failed:  203 0 
Mar  8 14:45:46 nickel: (  2519.102 @ 0x22514b0 / sync.debug) SyncDictionariesCommand(0x309018a0) finished 

It will still attempt to download the sideloaded dictionaries (due to the default value of IsSynced without the database), but it when it gets to replacing them, it won't due to the file permission check.

In addition, I've noticed that instead of having a migration to delete the dictionary table, they just neutered the existing migrations to add the dictionary stuff (:imp:, I personally don't think it's particularly advisable to modify migrations after the fact, but I see why they did, as it means they don't have to preserve the old dictionary cruft the migrations depend on), for example:

4.20.14601:

                            _ZN39Migration119_UpdateJapaneseDictionaries7migrateERK6DeviceR12QSqlDatabase:        // Migration119_UpdateJapaneseDictionaries::migrate(Device const&, QSqlDatabase&)
00508b4c 80B4                   push       r7
00508b4e 0120                   movs       r0, #0x1
00508b50 00AF                   add        r7, sp, #0x0
00508b52 BD46                   mov        sp, r7
00508b54 5DF8047B               ldr        r7, [sp, saved_fp], #0x4
00508b58 7047                   bx         lr

4.19.14123:

                            _ZN39Migration124_UpdateJapaneseDictionaries7migrateERK6DeviceR12QSqlDatabase:        // Migration124_UpdateJapaneseDictionaries::migrate(Device const&, QSqlDatabase&)
004eec08 2DE9F04F               push.w     {r4, r5, r6, r7, r8, sb, sl, fp, lr}
004eec0c 8046                   mov        r8, r0
004eec0e DFF8F005               ldr.w      r0, =0x933096                        ; 0x4ef200,0x933096
004eec12 95B0                   sub        sp, #0x54
004eec14 02AF                   add        r7, sp, #0x8
004eec16 1546                   mov        r5, r2
004eec18 8A46                   mov        sl, r1
004eec1a 7844                   add        r0, pc                               ; 0xe21cb4
004eec1c 0221                   movs       r1, #0x2
004eec1e 07F13C04               add.w      r4, r7, #0x3c
004eec22 A9F70EE9               blx        _ZN7QString17fromLatin1_helperEPKci@PLT ; QString::fromLatin1_helper(char const*, int)
004eec26 2946                   mov        r1, r5
004eec28 B863                   str        r0, [r7, #0x38]
004eec2a 2046                   mov        r0, r4
004eec2c BDF74AEE               blx        _ZNK12QSqlDatabase12databaseNameEv@PLT ; QSqlDatabase::databaseName() const
004eec30 07F13803               add.w      r3, r7, #0x38
004eec34 07F11000               add.w      r0, r7, #0x10
004eec38 1946                   mov        r1, r3
004eec3a 2246                   mov        r2, r4
004eec3c 3860                   str        r0, [r7]
004eec3e BB60                   str        r3, [r7, #0x8]
004eec40 E2F732EA               blx        _ZN17DictionaryManager26getDictionariesForLanguageERK7QStringS2_@PLT ; DictionaryManager::getDictionariesForLanguage(QString const&, QString const&)
004eec44 F86B                   ldr        r0, [r7, #0x3c]
004eec46 0368                   ldr        r3, [r0]
004eec48 002B                   cmp        r3, #0x0
004eec4a 40F08381               bne.w      loc_4eef54
; --- snip ---

@jackiew1
Copy link
Collaborator

jackiew1 commented Mar 8, 2020

@jackiew1, it would be nice if you could confirm this as well.

I've just run an SQLite DROP TABLE Dictionary on my KA1's database, then done a full re-boot. The KA1 already had the Extra: space/underscore patch installed.

I can confirm that all my sideloaded custom dictionaries are still working OK without the Dictionary table. Most of them are Extra types but one is a custom dicthtml-en-en.zip. I don't currently have any with the same filename as a built-in Kobo dictionary.

I have to admit I didn't see this one coming. Installing custom dictionaries should be much simpler now. Well done, Kobo ... or at least it would have been if they'd documented it somewhere. I wonder if this is just the beginning of some bigger plan?

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 8, 2020

I have to admit I didn't see this one coming. Installing custom dictionaries should be much simpler now. Well done, Kobo ... or at least it would have been if they'd documented it somewhere. I wonder if this is just the beginning of some bigger plan?

I think it's both a bit of altruism on their part (they've done quite a few things which have helped various mods in this version) and part of a plan to simplify the whole situation about double-state and inconsistencies regarding the DB, migrations, and in-memory state.

@jackiew1
Copy link
Collaborator

jackiew1 commented Mar 8, 2020

@geek1011 Odd that they didn't remove the need for the Extra: space/underscore patch at the same time. A 1-character fix must be easier than any of the other improvements/fixes they made. 😄

pgaskin added a commit to pgaskin/dictutil that referenced this issue Mar 8, 2020
* The database doesn't need to be updated anymore. On firmware 4.20.14601+,
  dictutil will still update it if it exists (to aid downgrades and prevent
  confusion), but it will show a warning.
* Nickel doesn't create the dictionary table on new installations anymore.
* Dictionaries can be prevented from being overwritten during syncing by marking
  them read-only.
* Nickel now hard-codes the list of built-in dictionaries rather than adding
  them during migrations and reading them from the db at runtime.

fixes #3
closes #4
see pgaskin/kobopatch-patches#49, pgaskin/kobopatch-patches#53
@pgaskin
Copy link
Owner Author

pgaskin commented Mar 8, 2020

@jackiew1 Maybe they thought it would fix that too? It's kind of an obscure bug.

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 8, 2020

I think we've now figured out and tested all of the relevant dictionary changes, so I'm going to close this issue now. Feel free to comment if you have any additional things, though.

In addition, I've released dictutil v0.2.1 based on the information here.

@pgaskin pgaskin closed this as completed Mar 8, 2020
@mzelyony
Copy link

Thanks @geek1011, @jackiew1
I went through the instructions and installed the sideloaded dictionary on 4.20.14622 and it seems to work fine. There are two things which concern me:

  1. In Languages and Dictionaries it shows a warning 1 dictionary download pending
  2. Despite the XXXX.zip being marked readonly it was still deleted when I unchecked it in the settings and hit Save

Thanks!

@pgaskin
Copy link
Owner Author

pgaskin commented Mar 23, 2020

  1. That's normal, it's just a side-effect of how it's implemented.
  2. You shouldn't uncheck it. Being read-only doesn't stop it from being deleted/uninstalled, only overwritten during the automated sync.

@mzelyony
Copy link

Ok, thanks

@kaveheng
Copy link

I've also confirmed geek1011/dictutil#3 is valid! Firmware 4.20.14601 doesn't create the Dictionary table anymore. I guess there wasn't a migration (or it was broken), and that's why devices upgrading from an older version still have that table.

That would also explain the unusual behaviour with the Extra: stuff. Here's my theory (I'll test it later today):

  • Same: Nickel still generates locale names by default with Extra: LOCALE.

  • New: Nickel doesn't read the dictionary table anymore, so the name in it is ignored. In addition, entries in the table won't change anything even if it is still present.

  • New: The built-in dictionaries are hard-coded, rather than writing them to the db during migrations and reading from it at runtime.

  • Same: Nickel still has the bug where the locale splitting is messed up, so the Extra: LOCALE names are inherently broken.

  • Same: The matching can be fixed by replacing Extra: with Extra:_.

  • New: The database doesn't need to be changed anymore in addition to the patch, as the names are generated dynamically using the same string.

  • Therefore: If the dictionary table is present, it can safely be removed.

  • Therefore: The steps required to install custom dictionaries are now:

    • Copy the dictzip and mark it read-only.
    • Add it to ExtraLocales if it is not a built-in locale.
    • Replace Extra: in libnickel with any other string (same length or shorter with a null byte at the end), but not containing anything in the Unicode whitespace class.

I'll write some stuff about this for the dictutil docs after I test it.

@jackiew1, it would be nice if you could confirm this as well.

How should I copy and paste the dictzip file as read only? when I paste the file in dict folder it changes to read and write

@pgaskin
Copy link
Owner Author

pgaskin commented May 5, 2020

How should I copy and paste the dictzip file as read only? when I paste the file in dict folder it changes to read and write

You should be able to do this from the file's properties dialog. If not, try doing it over SSH or telnet and see if that sticks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Category: Discussion/question (i.e. not anything specific).
Projects
None yet
Development

No branches or pull requests

6 participants