Skip to content

Fix Dict serialization#73

Merged
u-235 merged 1 commit into
eBookProjects:masterfrom
RipleyTom:fixes
Jan 26, 2026
Merged

Fix Dict serialization#73
u-235 merged 1 commit into
eBookProjects:masterfrom
RipleyTom:fixes

Conversation

@RipleyTom

Copy link
Copy Markdown
Contributor

This fixes the search function after reloading a file that had a dict generated.

stream << it.value()->documents.count(); serialized a qsizetype(u64 on most platforms) but
stream >> numOfDocs; read an int(s32)

This broke reading the serialized Dict.
I removed the need to serialize the size as QT will do it when for us and it is not needed.
I added stream.setVersion( QDataStream::Qt_5_15 ); to both read and write functions to ensure backward compatibility in the future.
I increased DICT_VERSION as it slightly changes the format of the serialization.

@u-235

u-235 commented Jan 26, 2026

Copy link
Copy Markdown
Member

Hi. Why not just change the type of the numOfDocs variable? In this case, existing dictionaries would remain valid and would not have to be updated. Adding stream.setVersion is certainly helpful, but the version is incorrect. The minimum supported version is still stated to be Qt 5.5.

@RipleyTom

RipleyTom commented Jan 26, 2026

Copy link
Copy Markdown
Contributor Author

If I changed the numOfDocs type it would break the Dict created with QT5 version.
.count() type returning u64 is a change introduced by QT6. I think it is better to have a clean slate.

@u-235 u-235 merged commit fd62005 into eBookProjects:master Jan 26, 2026
10 checks passed
@gonwan

gonwan commented Jan 26, 2026

Copy link
Copy Markdown
Contributor

QVector<>(size) reserves the size and avoid reallocation, especially for files with large index, I suppose. Better keep it.

@RipleyTom

Copy link
Copy Markdown
Contributor Author

The deserializer handles that internally and reserves the space in one go just like we did, why wouldn't it?

@gonwan

gonwan commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

You are right. Misread the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants