Introduce Table of Content Without Intense JS Invasion

[ShaopengLin]

Alternative to #1201. FIx #42

Changes:

• Uses QTreeWidget to handle TOC display
• The TOC now resides in the sidebar along with the Content Manager Side and ReadingList Bar.
• Styling is slightly different since QTreeWidget doesn't perform the same way as HTML and JS.

Benefits:

• More extensible when future features come
• Easier handling inside Qt for styling and parsing.

Loses:

• The TOC requesting is asynchronous and requires some fine-tuned race checking.
• Slow due to extra parsing and passing around of the TOC.

[veloman-yunkan]

Bug report - when the TOC is not empty the attempt to open the TOC sidebar results in the latter growing wider and wider indefinitely.

[ShaopengLin]

@veloman-yunkan I can't seem to replicate this. Is this happening on any zim?

[veloman-yunkan]

@veloman-yunkan I can't seem to replicate this. Is this happening on any zim?

It happens on ray_charles.zim for sure

[veloman-yunkan]

Screencast.from.11-07-2024.08.22.37.PM.webm

[ShaopengLin]

@veloman-yunkan Would like to confirm if you are on Qt6? I have tested Qt5 and Qt6, where Qt6 seems to be the version that this bug happens

[veloman-yunkan]

Here is the initial feedback from a quick and very superficial review

[veloman-yunkan]

@veloman-yunkan Would like to confirm if you are on Qt6? I have tested Qt5 and Qt6, where Qt6 seems to be the version that this bug happens

No, I am a qtsaur. I don't use the most recent version.

[veloman-yunkan]

• Slow due to extra parsing and passing around of the TOC.

Is this assessment based on your understanding of the implementation or a result of some measurement?

[ShaopengLin]

Is this assessment based on your understanding of the implementation or a result of some measurement?

@veloman-yunkan It is more like my own understanding and observation from other Stackoverflowers:

1. QTreeWidget have seen performance issues regarding larger data amounts: 1, 2
2. In Javascript, instead of creating JSON and doing another round of parsing, we can just create a <ul> <li> list on the first round and immediately append this to the sidebar.
3. In Qt, everytime we go to a new page, the TOC needs to reload. While If we display using Javascript, the TOC is already in the DOM and no need to reload.

[ShaopengLin]

Bug report - when the TOC is not empty the attempt to open the TOC sidebar results in the latter growing wider and wider indefinitely.

@veloman-yunkan Should be fixed now. Somehow under the same settings, QListWidget in ReadingListBar doesn't have this problem. This might be due to either styling or underlying widget differences.

[veloman-yunkan]

Is this assessment based on your understanding of the implementation or a result of some measurement?

@veloman-yunkan It is more like my own understanding and observation from other Stackoverflowers:

1. QTreeWidget have seen performance issues regarding larger data amounts: 1, 2

Do we ever expect "large data amounts" for TOC?

2. In Javascript, instead of creating JSON and doing another round of parsing, we can just create a <ul> <li> list on the first round and immediately append this to the sidebar.

Back to the previous comment, if the TOC is not going to be overwhelmingly large this shouldn't matter a lot.

3. In Qt, everytime we go to a new page, the TOC needs to reload. While If we display using Javascript, the TOC is already in the DOM and no need to reload.

How come the TOC is already in the DOM when we go to a new page? Did you mean switching tabs?

[veloman-yunkan]

The entries in the TOC need to be double clicked for the navigation to happen. Was it intended to be like that?

[ShaopengLin]

@veloman-yunkan

1. I don't expect the average TOC to be large.
2. Yes indeed.
3. Yes I meant switching tabs.

I do believe this PR is better than #1201. The loses I mentioned are more for acknowledging some minor inconveniences we face. In the grand scheme of things, it is not closr enough to offset the benefits by TOC to Qt side.

[veloman-yunkan]

Do I get it right that in this implementation TOC subsections are collapsible while in the JS-based implementation they aren't?

[ShaopengLin]

The entries in the TOC need to be double clicked for the navigation to happen. Was it intended to be like that?

@veloman-yunkan Its supposed to be single click. I will change this.

[ShaopengLin]

Do I get it right that in this implementation TOC subsections are collapsible while in the JS-based implementation they aren't?

@veloman-yunkan In JS they weren't collapsible.

[kelson42]

@ShaopengLin @veloman-yunkan Don't try to handle collabsable sections in the content. Focus only on the behaviour of the TOC sidebar.

[veloman-yunkan]

In this iteration my focus was on the injected JS script.

[veloman-yunkan]

I do believe this PR is better than #1201. The loses I mentioned are more for acknowledging some minor inconveniences we face. In the grand scheme of things, it is not closr enough to offset the benefits by TOC to Qt side.

I am now also inclined to consider this solution as a superior one. Hoping that we won't discover a major deficiency in this approach that will make us shift back to the JS-based version.

[veloman-yunkan]

My intuition tells me that this combination of iteration and recursion is redundant (and to my taste a little confusing). I feel that this algorithm can be rewritten in a clearer way with iteration alone (you will need an additional nested loop for ascending up the hierarchy to the QTreeWidgetItem corresponding to the new level). To add more clarity please extract the code responsible for the creation and initialization of a new QTreeWidgetItem into a helper function of its own.

[ShaopengLin]

Done

[veloman-yunkan]

What are the assumptions about level values in the header data? Can there be gaps like in the following sequence: {h1, h3, h4, h4, h2, h4} ? Does your algorithm handle such cases? Otherwise, assuming absence of gaps i.e. all levels on a branch from the root to the leaf are filled, why do you need LevelRole?

[ShaopengLin]

Yes there can be gaps. During parsing, we use their level to determine the correct parent, but the tree is constructed as if there are no gaps ( i.e. { h2, h4 } will be repesented in the tree as if it is { h1, h2 }). Having perfect headers is an unlikely assumption...

[veloman-yunkan]

Well, I see now that probably under such conditions combining recursion with iteration makes more sense. Without recursion you have to store in the tree items intermediate data like level and the number-list (this one was present in your previous implementation too). Here is a cleaner and more comprehensible scheme:

QJsonArray takeDeeperEntries(QJsonArray& headerArr, int level)
{
    QJsonArray result;
    while (!headerArr.isEmpty() ) {
        const auto& nextHeader = headerArr.first().toObject();
        if ( nextHeader["level"].toInt() <= level )
           break;
        result.push_back(nextHeader);
        headerArr.pop_front();
    }
    return result;
}

void createSubTree(QTreeWidgetItem* parent, QString parentNo, QJsonArray headerArr)
{
    while (!headerArr.isEmpty()) {
        const auto& childHeader = headerArr.takeAt(0).toObject();
        const int childLevel = childHeader["level"].toInt();
        const QString childNo = parentNo + "." + QString::number(parent->childCount() + 1);
        QTreeWidgetItem* childItem = createChildItem(parent, childNo, childHeader);
        const QJsonArray deeperStuff = takeDeeperEntries(headerArr, childLevel);
        setupSubTree(childItem, childNo, deeperStuff);
    }
}

[ShaopengLin]

Done

[veloman-yunkan]

LGTM.

Since C++ is not a typo-safe language the compiler didn't detect the contamination of this PR by a subtle spelling error, however I believe that no user will be affected if we merge the PR as is.

A couple of comments regarding functionality:

1. navigation via TOC isn't recorded in the history.
2. when a section in the content is collapsed clicking a TOC entry belonging to that section has no effect

[veloman-yunkan]

recieve -> receive

[ShaopengLin]

Done

[veloman-yunkan]

A couple of comments regarding functionality:

1. navigation via TOC isn't recorded in the history.

2. when a section in the content is collapsed clicking a TOC entry belonging to that section has no effect

I believe those can be addressed in a follow-up PR (if they need to be fixed at all).

[kelson42]

A couple of comments regarding functionality:

1. navigation via TOC isn't recorded in the history.
2. when a section in the content is collapsed clicking a TOC entry belonging to that section has no effect

I believe those can be addressed in a follow-up PR (if they need to be fixed at all).

Don't forget to open tickets!