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

Fix v11n mapping in parallel views #361

Open
kalemas opened this issue Jan 7, 2022 · 1 comment · May be fixed by #363
Open

Fix v11n mapping in parallel views #361

kalemas opened this issue Jan 7, 2022 · 1 comment · May be fixed by #363
Assignees

Comments

@kalemas
Copy link
Member

kalemas commented Jan 7, 2022

Inspecting v3.0.2 code i found that BtModuleTextModel render text for verses in parallel mode for each module separately, without passing context of all rendering modules. That makes impossible to translate versifications.
Inspecting rendering pipeline i found correct that text items are selectable for each module separately, so requirement to render
a single verse in parallel view looks justified.
I also would note that i do not get yet, why that model use multiple data roles rather than utilize columns.

So i would make following suggestions to resolve this issue:

  1. Add additional parameter to render only single module in CEntryDisplay::textKeyRendering() and still pass full module list.
  2. Convert key parameter of CEntryDisplay::textKeyRendering() to sword::SWKey or somewhat like that so it would be casted to VerseKey and translated to v11n of requested module.
  3. I think it also would be possible to still render entry for all parallel modules in base text model and add proxy model that will split text in a way suitable for rendering in QML view. Maybe it will require some extra work, like caching data, and extra markup in html but it will improve base text model.
  4. V11n translation would be done before calling rendering, in model. This way we will have duplicate code.

In sense of performance i think 1. and 2. are better, but i still would like to have flexible basic text model with good design so it would be easily reused and improved.

@kalemas kalemas self-assigned this Jan 7, 2022
@kalemas
Copy link
Member Author

kalemas commented Jan 7, 2022

after inspecting core rendering pipeline i think 2. is best choice, it already have related functionality at KeyTreeItem::mappedKey() that may be optimized with this change.

I think KeyTreeItem should receive CSwordKey instead of QString as it only may correctly describe a position in a Module.

Also i think it would be possible to remove this parameter that is often unused:

- CTextRendering::renderEntry(KeyTreeItem const & i, CSwordKey * k)
+ CTextRendering::renderEntry(KeyTreeItem const & i)

So parallel mode case will be handled by mapping KeyTreeItem's key onto module from module list that will trigger v11n translation.

@kalemas kalemas linked a pull request Jan 8, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

1 participant