Introduce Table of Content Without Intense JS Invasion #1237

ShaopengLin · 2024-11-07T08:08:27Z

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 · 2024-11-07T15:46:35Z

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 · 2024-11-07T16:03:16Z

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

veloman-yunkan · 2024-11-07T16:17:29Z

@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 · 2024-11-07T16:23:34Z

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

ShaopengLin · 2024-11-07T16:25:29Z

@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 · 2024-11-07T15:34:27Z

src/kprofile.cpp

+QWebEngineScript getScript(QString filename,
+    QWebEngineScript::InjectionPoint point = QWebEngineScript::DocumentReady)
+{
+    QWebEngineScript script;
+    script.setInjectionPoint(point);
+    script.setWorldId(QWebEngineScript::UserWorld);
+
+    QFile scriptFile(filename);
+    scriptFile.open(QIODevice::ReadOnly);
+    script.setSourceCode(scriptFile.readAll());
+    return script;
+}


Hide in unnamed namespace

veloman-yunkan · 2024-11-07T15:55:18Z

src/webview.cpp

+    connect(tabbar, &TabBar::currentTitleChanged, this, 
+            [=](){
+                const auto tableObject = m_tableOfContent.object();
+                const auto currentUrl = url().url(QUrl::RemoveFragment);
+                const auto tableValid = tableObject["url"].toString() == currentUrl;
+
+                /* When table invalid, then we are loading and the emit will be
+                   handled by KiwixWebChannelObject::tableOfContentChanged.
+                */
+                if (tabbar->currentWebView() == this && tableValid)
+                    emit tableOfContentChanged(m_tableOfContent);
+            });
+    connect(kiwixChannelObj, &KiwixWebChannelObject::tableOfContentChanged, this, 
+            [=](const QString& tableJson){
+                m_tableOfContent = QJsonDocument::fromJson(tableJson.toUtf8());
+                if (tabbar->currentWebView() == this)
+                    emit tableOfContentChanged(m_tableOfContent);
+            });


Better use named slots instead of anonymous lambdas

veloman-yunkan · 2024-11-07T16:04:57Z

resources/js/tableofcontent.js

+function recurseChild(elem, recurseData)
+{
+    if (elem !== "undefined") 
+    {
+        if(elem.nodeName.match(/^H\d+$/) && elem.textContent)
+        {
+            var headerText = elem.textContent.trim();
+            var prevLevel = recurseData.level;
+            var level = elem.nodeName.substr(1);
+            var anchor = "kiwix-toc-" + recurseData.count;
+            var anchorLink = window.location.href.replace(location.hash,"") + '#' + anchor;
+            recurseData.count += 1;
+
+            var anchorElem = document.createElement("a");
+            anchorElem.id = anchor;
+
+            /* Wrap header content with something we can reference. */
+            elem.insertAdjacentElement("afterbegin", anchorElem);
+
+            if (level < prevLevel)
+            {
+                /* Complete current element and the parent element.*/
+                recurseData.toc += ']}]}, ';
+            }
+            else if (level == prevLevel)
+            {
+                /* Complete current element*/
+                recurseData.toc += ']}, ';
+            }
+
+            recurseData.level = parseInt(level);
+            recurseData.levelSet.add(parseInt(level));
+            recurseData.toc += '{"text" : "' + headerText.replace(/"/g, '\\"') + '", "anchor": "' + anchorLink + '", ' + '"child" : [';
+        }
+
+        var c = elem.children;
+        for (var i = 0; i < c.length; i++)
+            recurseChild(c[i], recurseData);
+    }
+}
+
+function tocJSON()
+{
+    /* level used to track current header level.
+       toc used to store constructed list.
+       count used to uniquely identify each list item in toc.
+       levelSet used to retrieve the levels disregarding header level value.
+    */
+    var recurseData = { level: 0, toc: '{ "url" : "' + window.location.href.replace(location.hash,"") + '", "table" : [', count: 0, levelSet: new Set};
+    recurseChild(document.body, recurseData);
+
+    var levelArray = Array.from(recurseData.levelSet).sort();
+    levelArray.sort(function(a, b){return a - b});
+    var level = levelArray.indexOf(recurseData.level) + 1; 
+
+    /* End un-closed lists */
+    if (level)
+        recurseData.toc += (new Array(level + 1)).join(']}');
+    recurseData.toc += ']}';
+    return recurseData.toc;
+}


I think that instead of constructing the final JSON string it is better to construct a recursive JS object and then call JSON.stringify() on it.

veloman-yunkan · 2024-11-07T16:09:31Z

resources/js/tableofcontent.js

+            var prevLevel = recurseData.level;
+            var level = elem.nodeName.substr(1);
+            var anchor = "kiwix-toc-" + recurseData.count;
+            var anchorLink = window.location.href.replace(location.hash,"") + '#' + anchor;


I think that it doesn't make sense to include the full URL with every link. The URL of the page can be provided only once at the root level of the returned JSON object while each TOC entry will include only the anchor.

veloman-yunkan · 2024-11-07T16:15:40Z

resources/js/tableofcontent.js

+    kiwixObj.navigationRequested.connect(function(anchor) {
+        var anchorIndex = anchor.lastIndexOf('#');
+        var anchorPageUrl = anchor.substr(0, anchorIndex);
+        if (window.location.href.replace(location.hash,"") === anchorPageUrl)
+            window.location.href = anchor;
+    })


This comment is about the full commit - can you elaborate on the scenario that is problematic if the navigation is implemented in C++? Is your solution fully devoid of similar problems or they are just made less embarrassing for the UX?

Ok, I thought I replaced this commit already... I previously forgot the setUrl method and was using this method. Another victim from rebasing.

veloman-yunkan · 2024-11-07T16:27:29Z

@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 · 2024-11-07T16:30:20Z

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 · 2024-11-07T16:45:08Z

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:

QTreeWidget have seen performance issues regarding larger data amounts: 1, 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.
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 · 2024-11-07T16:59:53Z

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.

Setup for TOC Javascript/CSS Injection

Add channel to communicate between web page and Qt

Re-enabled ToggleTOCAction to display the bar.

Only one should be in checked state.

Signal emits the collected header JSON. Parsing delegated to JS due to better DOM manipulations.

Recursively load the header JSON.

Upon signaling, use JS to navigate, as helps avoid wrong url setting given the asynchronism.

Elided item text is expanded here.

ShaopengLin requested a review from veloman-yunkan November 7, 2024 08:08

ShaopengLin mentioned this pull request Nov 7, 2024

New implement search in tabs/history/bookmarks #1194

Open

ShaopengLin force-pushed the Issue#42-toc-qtree-alternative branch from fe978e1 to 331855e Compare November 7, 2024 16:11

veloman-yunkan reviewed Nov 7, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#42-toc-qtree-alternative branch from 331855e to 388441f Compare November 7, 2024 16:56

ShaopengLin added 12 commits November 7, 2024 17:14

Introduce QWebEngine Javascript Infrastructure

09cd9d6

Setup for TOC Javascript/CSS Injection

Introduce KiwixWebChannelObject.{h, cpp}

2143e92

Add channel to communicate between web page and Qt

Enter tableofcontentbar.{h,cpp,ui}

4620a3b

Re-enabled ToggleTOCAction to display the bar.

Ensure ReadingList&TOC display states are consistency

5f74621

Only one should be in checked state.

Signal Current TOC Parsed from JS

a60f676

Signal emits the collected header JSON. Parsing delegated to JS due to better DOM manipulations.

Connect Sidebar to Display Table Of Content

8e45c85

Recursively load the header JSON.

Navigate to Header On Click in TOC

77a5e56

Upon signaling, use JS to navigate, as helps avoid wrong url setting given the asynchronism.

Number TOC Header Entries

bdfbffa

Add Hide Button to TOC

3a07518

Proper Font and Sizes for TOC

df59543

Proper Styling to TOC

f389411

Add Tooltip to TOC

6d4aca1

Elided item text is expanded here.

ShaopengLin force-pushed the Issue#42-toc-qtree-alternative branch from 388441f to 6d4aca1 Compare November 7, 2024 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Table of Content Without Intense JS Invasion #1237

Introduce Table of Content Without Intense JS Invasion #1237

ShaopengLin commented Nov 7, 2024 •

edited

Loading

veloman-yunkan commented Nov 7, 2024

ShaopengLin commented Nov 7, 2024

veloman-yunkan commented Nov 7, 2024

veloman-yunkan commented Nov 7, 2024

ShaopengLin commented Nov 7, 2024

veloman-yunkan left a comment

veloman-yunkan Nov 7, 2024

veloman-yunkan Nov 7, 2024

veloman-yunkan Nov 7, 2024

veloman-yunkan Nov 7, 2024

veloman-yunkan Nov 7, 2024

ShaopengLin Nov 7, 2024

veloman-yunkan commented Nov 7, 2024

veloman-yunkan commented Nov 7, 2024

ShaopengLin commented Nov 7, 2024 •

edited

Loading

ShaopengLin commented Nov 7, 2024

Introduce Table of Content Without Intense JS Invasion #1237

Are you sure you want to change the base?

Introduce Table of Content Without Intense JS Invasion #1237

Conversation

ShaopengLin commented Nov 7, 2024 • edited Loading

veloman-yunkan commented Nov 7, 2024

ShaopengLin commented Nov 7, 2024

veloman-yunkan commented Nov 7, 2024

veloman-yunkan commented Nov 7, 2024

ShaopengLin commented Nov 7, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan Nov 7, 2024

Choose a reason for hiding this comment

veloman-yunkan Nov 7, 2024

Choose a reason for hiding this comment

veloman-yunkan Nov 7, 2024

Choose a reason for hiding this comment

veloman-yunkan Nov 7, 2024

Choose a reason for hiding this comment

veloman-yunkan Nov 7, 2024

Choose a reason for hiding this comment

ShaopengLin Nov 7, 2024

Choose a reason for hiding this comment

veloman-yunkan commented Nov 7, 2024

veloman-yunkan commented Nov 7, 2024

ShaopengLin commented Nov 7, 2024 • edited Loading

ShaopengLin commented Nov 7, 2024

ShaopengLin commented Nov 7, 2024 •

edited

Loading

ShaopengLin commented Nov 7, 2024 •

edited

Loading