Introduce Table of Content

[ShaopengLin]

Fix #42

Changes:

• Inject to pages a Table of Contents comprised of a nested hierarchy list of hyperlinks to tags in the page.
• Add a button in the corner to toggle the table.
• No button and table are displayed if the table is empty.

[kelson42]

@ShaopengLin The feature work well to my opinion, but we need to change/polish the way how the TOC is displayed, see my comment in the issue.

On the top:

• Please put the TOC icon in the toolbar beside the random icon
• Create an entry in the menu "View > Table of Content"
• Create a keyboard shorcut to toggle the sidebar: ctrl+m

[ShaopengLin]

@kelson42 To clarify, what does the view entry do? I didn't see it from the mockup, but I assume you mean either a button to collapse the entire TOC, or it will do the same thing as the TOC icon. Like a show/hide button?

[kelson42]

@kelson42 To clarify, what does the view entry do? I didn't see it from the mockup, but I assume you mean either a button to collapse the entire TOC, or it will do the same thing as the TOC icon. Like a show/hide button?

Not sure to fully understand you, but "yes", this is about toggling (hide/show) the TOC sidebar.

[ShaopengLin]

@kelson42 Improved the TOC as much as possible requested in this comment #1201 (comment). I will refactor the code again once the UI is ready.

I haven't been able to find a way to inject custom fonts with javascript, so currently it is in Arial. I am looking into some ways to make this work.

[kelson42]

@ShaopengLin Sorry, I don't get it, shortcut does not display the TOC sidebar AND no icon in the toolbar

[ShaopengLin]

@kelson42 I didn't find any issues on my end.

[kelson42]

Works now!

• If in library or no content displayed, then toolbar button should be greyed (like random button I guess)
• I believe that It should not be possible to display the toc sidebar at the same time like the bookmark sidebar. Displaying one should hide the other
• Secure look an feel is the same between bookmark and toc sidebar (The one of the TOC sidebar is better, it should be the reference)
• Loading a new article should not hide the TOC sidebar
• The width of the sidebar is a bit uselessly big, margin left and right should be smaller..
• Strategy should be clear if a section title is longer than the width: ellipsis en full title in the tooltip (when the mouse is over)
• I believe it would be clearer if the full background is highlighed when the mouse goes over the items. For the moment we can not keep the selection I guess as it should then move dynamically when we scroll the article

[ShaopengLin]

@kelson42 Most are doable.

Strategy should be clear if a section title is longer than the width: ellipsis en full title in the tooltip (when the mouse is over)

I will try my best on the ellipsis (Had some trouble with it on the CSS side). The tooltip should be doable.

Secure look an feel is the same between bookmark and toc sidebar (The one of the TOC sidebar is better, it should be the reference)

Out of scope for this PR, I will open an issue. The reading list's UI refactoring is long overdue anyway...

I believe it would be clearer if the full background is highlighted when the mouse goes over the items. For the moment we can not keep the selection I guess as it should then move dynamically when we scroll the article

Do you mean to highlight the items in the TOC when we hover them? Not sure what "full background" means here.

The dynamic scrolling might be too much for this PR... an indicator of localization has proven rather hard for me given the time constraint. I will open an issue afterward of this.

[kelson42]

Do you mean to highlight the items in the TOC when we hover them? Not sure what "full background" means here.

I believe you understand properly. Item background highlighted from the left to the right of the sidebar

The dynamic scrolling might be too much for this PR... an indicator of localization has proven rather hard for me given the time constraint. I will open an issue afterward of this.

Indeed. Thx

[ShaopengLin]

@kelson42 All issues mentioned are resolved:

1. TOC doesn't close on navigations and tab switches.
2. TOC and reading list does not co-exist.
3. grey out button appropriately
4. Added ellipsis and hover highlighting. Tool tips appears on the bottom left since they are links.

Once we finally agree on the UI I can refactor the code. The implementation is hard and due to time constraints, I didn't do this methodically. I will ping veloman when I finish by then.

[kelson42]

Lots of things work fine, but:

• TOC toolbar button is not always activated properly, here just after opening a ZIM file from the local library! I don't believe you have followed the same approach like the random button and seems to have build a new (broken) logic to decide if this button should be actived!
  
• Please remove the "cursor" pointer, keep always the "pointer" mouse pointer.
  
• Item highlighting is OK, but there is a problem with right border not visible! highlighting left and right border should be the same
• 
• Margin are still "wild". "Table of Content" (and then hide) 4 margin should be the same. Left margin of the first TOC item should be the same as "Table of Content" and aligned:
  
• Sidebar width is IMHO too big, should be the same as the bookmard sidebar
• By playing around with ctrl+M, ctrl+B and ctrl+R I still achieved to get the two bars displayed at the same time. The system to avoid duplicate sidebars should be robust and implemented differently
  

[ShaopengLin]

@kelson42 Disable behavior now matches the random button. The rest of the UI request is fixed.

[kelson42]

@ShaopengLin Not all points: On the following screenshot you clear see:

• The width of the sidebar is even bigger than before and not the same as the bookmarking one
• Top margin over "highlighed" text is bigger than left
  

For the rest it looks good and when these two points are sorted out then we can probably start with code review

[ShaopengLin]

@kelson42 Fix size done.

Top margin over "highlighed" text is bigger than left

This isn't intentional. If you highlight any text, that's just how they are highlighted.

[ShaopengLin]

@veloman-yunkan One question, am I allowed to introduce custom URL paths like "zim://kiwix-desktop-toc" ? Since we are using injected javascript, loading style sheet, icon, font, etc. would be much easier if we provide a request API path. It also is useful to pass information from the website to our application from some custom event.

[kelson42]

@kelson42 Fix size done.

Top margin over "highlighed" text is bigger than left

This isn't intentional. If you highlight any text, that's just how they are highlighted.

I hope with what follows, what is left to do is clearer:

• The red part should not exist
• the "hide" bottom should be the same as the "Table of content"
  

STILL A PROBLEM: If I have ctrl+B + ctrl+R + Ctrl+M and then again ctrl+R... every two attempts the TOC disappear after loading a random page.

[ShaopengLin]

@kelson42 disappearance should be fixed.

Unfortunately, the top and bottom padding around the text belong to line height, which the border cannot close in on. Otherwise the distances are consistent.

[veloman-yunkan]

@veloman-yunkan One question, am I allowed to introduce custom URL paths like "zim://kiwix-desktop-toc" ? Since we are using injected javascript, loading style sheet, icon, font, etc. would be much easier if we provide a request API path. It also is useful to pass information from the website to our application from some custom event.

I don't think that introducing custom URL schemes is strictly prohibited but "zim://kiwix-desktop-toc" doesn't make much sense to me, unless it is just a (bad) example of what you mean to do. Please elaborate on the problem and your approach to address it.

[ShaopengLin]

@veloman-yunkan The TOC has a hide button that hides it. Our application in Qt also has a toggle for TOC. However, our application has no way to know the user clicked the hide button inside TOC since its in the webpage. Thus, I am considering sending requests like "zim://kiwix-desktop-toc?close=true" or something like that to update state changes for our application.

[veloman-yunkan]

@veloman-yunkan The TOC has a hide button that hides it. Our application in Qt also has a toggle for TOC. However, our application has no way to know the user clicked the hide button inside TOC since its in the webpage. Thus, I am considering sending requests like "zim://kiwix-desktop-toc?close=true" or something like that to update state changes for our application.

Please let me research what solutions are possible and reply tomorrow.

[kelson42]

Don't abuse the zim:// scheme for anything else as accessing the ZIM file.

[veloman-yunkan]

@ShaopengLin It looks like you can take advantage of QWebChannel (via QWebEnginePage::setWebChannel()).

If for some reason that or some other approach doesn't work and you have to resort to your hack of communicating via UrlSchemeHandler then you can use a new URL scheme for that (e.g. kiwix-desktop://).

[ShaopengLin]

@veloman-yunkan Thanks! This is a lot more intuitive and works well.

@kelson42 I have addressed the issues of UI in this comment. If the UI is ready I can go on to the code refactoring.

[kelson42]

From a user perspective LGTM
@veloman-yunkan Ready for the technical review

[ShaopengLin]

@veloman-yunkan I will tag you when the code is ready.

[kelson42]

@ShaopengLin Still a bug: If section title is shorten, then there is not hint displayed with the full title!

[ShaopengLin]

@kelson42 I could not reproduce this on my own ZIM files. Could you direct me to where your example file is?

[kelson42]

@kelson42 I could not reproduce this on my own ZIM files. Could you direct me to where your example file is?

There is not tooltip on your screenshot either! I'm not talking about the tooltip at the bottom which display the targeted URL, I'm talking about a tooltip displyed at the section title displaying the title of the section (you have it already because you display it in the TOC).

[ShaopengLin]

@kelson42 Done, see if it matches what you are looking for stylistically.

[kelson42]

@kelson42 Done, see if it matches what you are looking for stylistically.

Yes, I would not have positioned the tooltip like this, but I like the way how you have fixed it!

[kelson42]

@ShaopengLin Any news abou the code cleaning you are running? It's important we start soon with code review.

[ShaopengLin]

@kelson42 I have just finished the zim search one today as its a large change. I will start on this shortly. This one should take slightly less time.