Fixed Whiteouts of Book Descriptions on Hover #1115

ShaopengLin · 2024-05-22T04:43:56Z

Changes:

For the description row, only use one column to span across.
DescriptionNode's data is now available on column index 0 instead of 1.
Added word wrap to previously unwrapped text
Prevented button actions from the last column to overflow into the description row.

veloman-yunkan

Whiteouts of book description have not been fixed - on the contrary, the name column was included in that undesirable behaviour.

On another note, this PR fixes several issues. Please extract each fix in a commit of its own.

src/contentmanagerdelegate.cpp

src/contentmanagerview.cpp

ShaopengLin · 2024-05-22T13:56:51Z

Whiteouts of book description have not been fixed - on the contrary, the name column was included in that undesirable behaviour.

@veloman-yunkan I can't seem to reproduce what you mean in both 5.15.2 and 6.2.4.

@kelson42 does this fix work on your build?

kelson42 · 2024-05-23T04:33:40Z

@ShaopengLin In the future, please develop your PR in a feature branch in our repo (not in your fork). I didn't had the time to test your branch which somehow seems to be difficult to reconciliate with my (this repo) revision log.

ShaopengLin · 2024-05-23T08:15:02Z

I will try installing 5.12 a bit later and see if I can reproduce it. Will need to download it eventually since it seems compilation issues will continue to present themselves in the future (e.g. #1116) :).

ShaopengLin · 2024-05-24T16:30:22Z

Cannot seem to reproduce it either in 5.12. Will just wait for a response.

veloman-yunkan · 2024-05-29T13:48:30Z

Whiteouts of book description have not been fixed - on the contrary, the name column was included in that undesirable behaviour.

I double checked that it was not a result of an incorrect build:

Screencast.from.05-29-2024.05.44.02.PM.webm

I am using Ubuntu 22.04.

kelson42 · 2024-06-01T15:06:46Z

@ShaopengLin I have been able to reproduce this problem (with both Qt6 and Qt6) but I hard a had time. It's important to:

Deal with the very same ZIM files like in the screencast
Windows size should be the very same
After downloading, go once to "online" and then back "to offline"
Finally uncollapse and the bug should be there

ShaopengLin · 2024-06-01T22:41:15Z

Confirmed fixed in all Qt5 and Qt6. The reason it isn't working is the press on the specific drop-down button will not count as a clicked event, thus our handler for the fix isn't called.

src/contentmanagerdelegate.cpp

veloman-yunkan · 2024-06-03T09:46:31Z

src/contentmanagerdelegate.cpp

-        if (lastColumnClicked)
+        /* The description row does not constitute as part of the open button. */
+        if (lastColumnClicked && !index.parent().isValid())
            handleLastColumnClicked(index, e, option);


I fixed this same problem slightly differently in my so far WIP PR #1118. Do you think that an early return may have undesirable effects?

Users previously can open the book by middle clicking on the description section. An early return would then disable this feature.

@veloman-yunkan I believe fixing this your commit makes more sense. Your can do that check anytime after the middle button click check.

src/contentmanagerdelegate.cpp

veloman-yunkan

Great! The PR is so much better this way. Let's make it close to ideal and we will merge.

src/contentmanagermodel.cpp

src/contentmanagerdelegate.cpp

veloman-yunkan · 2024-06-06T10:00:18Z

src/contentmanagerdelegate.cpp

+    if (index.parent().isValid())
+    {
+        auto view = KiwixApp::instance()->getContentManager()->getView()->getView();
+
+        /* Get the Rectangle from start of column1 to end of column 5. */
+        QRect r = view->visualRect(index.parent().siblingAtColumn(1));
+        QRect lastColRect = view->visualRect(index.parent().siblingAtColumn(5));
+        r.setWidth(lastColRect.x() + lastColRect.width() - r.x());
+
+        /* Based on the rectangle and text, find the best fitting size. */
+        QFontMetrics fm(option.font);
+        QSize textRectSize = fm
+            .boundingRect(r,
+                          Qt::AlignLeft | Qt::AlignVCenter | Qt::TextWordWrap,
+                          index.data().toString())
+            .size();
+
+        if (textRectSize.height() < 70)
+            textRectSize.setHeight(70);
+        return textRectSize;
    }


Nice, but it seems to slightly fall short of its goal:

src/contentmanagermodel.cpp

src/descriptionnode.cpp

veloman-yunkan · 2024-06-07T10:00:28Z

src/contentmanagerdelegate.cpp

+    if (index.parent().isValid())
+    {
+        auto header = KiwixApp::instance()->getContentManager()->getView()->getView()->header();
+
+        /* Get the Rectangle from start of column1 to end of column 5. This is a
+           slight underestimate of the actual width, but it means we will produce 
+           a proper height upper bound from boundingRect and the painting process 
+           will span the width regardless of what we return in sizeHint.
+        */
+        QRect descRect(0, 0, header->length() - header->sectionSize(0), 0);
+
+        /* Based on the rectangle and text, find the best fitting size. */
+        QFontMetrics fm(option.font);
+        const QString text = index.data().toString();
+        const auto format = Qt::AlignLeft | Qt::AlignVCenter | Qt::TextWordWrap;
+        const QRect textRect = fm.boundingRect(descRect, format, text);
+        QSize textSize = textRect.size();
+
+        if (textSize.height() < 70)
+            textSize.setHeight(70);
+        return textSize;
    }


This still doesn't work correctly

Screencast.from.06-07-2024.01.55.59.PM.webm

Maybe you have to account for some padding

Hmm either the header length or the boundingRect function is being inaccurate between systems. I will try to find an appropriate width for the height calculation.

veloman-yunkan · 2024-06-09T06:01:01Z

src/contentmanagermodel.cpp

-    if (index.isValid() && index.parent().isValid()) {
+    if (isDescriptionIndex(index)) {


Does isDescriptionIndex() handle an invalid index correctly?

Yep, if the index is invalid, index.parent() will be invalid as well. Consequently, they have the same effect.

src/contentmanagermodel.cpp

src/contentmanagerdelegate.cpp

src/contentmanagerview.cpp

veloman-yunkan

LGTM

The branch must be rebased and the a conflict resolved. Then we will merge.

Handler is now a slot function instead of a lambda.

index.parent().isValid replaced with static function isDescriptionNode.

Clicking and hovering the description dropdown of zim no longer whites out. Replaced manual drawing with default handling as it is no longer necessary.

The description row height fits the text context when expanded.

Already expanded description row will now adjust with respect to any window size changes to fit the content.

ShaopengLin requested a review from veloman-yunkan May 22, 2024 04:44

veloman-yunkan requested changes May 22, 2024

View reviewed changes

src/contentmanagerdelegate.cpp Outdated Show resolved Hide resolved

src/contentmanagerview.cpp Outdated Show resolved Hide resolved

veloman-yunkan mentioned this pull request May 22, 2024

Book description misbehaves on hover #1112

Closed

ShaopengLin force-pushed the Issue#1112-fix-bookmark-description branch from f3cd032 to f7a71f2 Compare June 1, 2024 22:39

ShaopengLin requested a review from veloman-yunkan June 1, 2024 22:49

veloman-yunkan requested changes Jun 3, 2024

View reviewed changes

veloman-yunkan reviewed Jun 3, 2024

View reviewed changes

src/contentmanagerdelegate.cpp Outdated Show resolved Hide resolved

ShaopengLin force-pushed the Issue#1112-fix-bookmark-description branch from f7a71f2 to 2d62484 Compare June 5, 2024 03:29

ShaopengLin requested a review from veloman-yunkan June 5, 2024 03:34

veloman-yunkan requested changes Jun 6, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#1112-fix-bookmark-description branch from 2d62484 to db53bb2 Compare June 7, 2024 06:24

ShaopengLin requested a review from veloman-yunkan June 7, 2024 06:25

veloman-yunkan requested changes Jun 7, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#1112-fix-bookmark-description branch from db53bb2 to 21df619 Compare June 7, 2024 22:56

ShaopengLin requested a review from veloman-yunkan June 7, 2024 23:10

veloman-yunkan requested changes Jun 9, 2024

View reviewed changes

ShaopengLin force-pushed the Issue#1112-fix-bookmark-description branch from 21df619 to aea74f6 Compare June 9, 2024 08:44

ShaopengLin requested a review from veloman-yunkan June 9, 2024 09:09

veloman-yunkan reviewed Jun 11, 2024

View reviewed changes

ShaopengLin added 4 commits June 11, 2024 14:49

Refactored ContentManager click signal handler

2f4a36f

Handler is now a slot function instead of a lambda.

Introduce method to decide whether index contain description

bc1107b

index.parent().isValid replaced with static function isDescriptionNode.

Fixed hover misbehaviour over zim description

e635611

Clicking and hovering the description dropdown of zim no longer whites out. Replaced manual drawing with default handling as it is no longer necessary.

Description row fits now content on expand

ef7645e

The description row height fits the text context when expanded.

Description row fits to content when screen size change

fa2d425

Already expanded description row will now adjust with respect to any window size changes to fit the content.

ShaopengLin force-pushed the Issue#1112-fix-bookmark-description branch from aea74f6 to fa2d425 Compare June 11, 2024 18:51

kelson42 merged commit b59155a into kiwix:main Jun 11, 2024
2 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Whiteouts of Book Descriptions on Hover #1115

Fixed Whiteouts of Book Descriptions on Hover #1115

ShaopengLin commented May 22, 2024 •

edited

Loading

veloman-yunkan left a comment

ShaopengLin commented May 22, 2024 •

edited

Loading

kelson42 commented May 23, 2024 •

edited

Loading

ShaopengLin commented May 23, 2024

ShaopengLin commented May 24, 2024

veloman-yunkan commented May 29, 2024

kelson42 commented Jun 1, 2024

ShaopengLin commented Jun 1, 2024

veloman-yunkan Jun 3, 2024

ShaopengLin Jun 3, 2024 •

edited

Loading

ShaopengLin Jun 5, 2024

veloman-yunkan left a comment

veloman-yunkan Jun 6, 2024

veloman-yunkan Jun 7, 2024

veloman-yunkan Jun 7, 2024

ShaopengLin Jun 7, 2024 •

edited

Loading

veloman-yunkan Jun 9, 2024

ShaopengLin Jun 9, 2024 •

edited

Loading

veloman-yunkan left a comment

		if (index.isValid() && index.parent().isValid()) {
		if (isDescriptionIndex(index)) {

Fixed Whiteouts of Book Descriptions on Hover #1115

Fixed Whiteouts of Book Descriptions on Hover #1115

Conversation

ShaopengLin commented May 22, 2024 • edited Loading

veloman-yunkan left a comment

Choose a reason for hiding this comment

ShaopengLin commented May 22, 2024 • edited Loading

kelson42 commented May 23, 2024 • edited Loading

ShaopengLin commented May 23, 2024

ShaopengLin commented May 24, 2024

veloman-yunkan commented May 29, 2024

kelson42 commented Jun 1, 2024

ShaopengLin commented Jun 1, 2024

veloman-yunkan Jun 3, 2024

Choose a reason for hiding this comment

ShaopengLin Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

ShaopengLin Jun 5, 2024

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan Jun 6, 2024

Choose a reason for hiding this comment

veloman-yunkan Jun 7, 2024

Choose a reason for hiding this comment

veloman-yunkan Jun 7, 2024

Choose a reason for hiding this comment

ShaopengLin Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

veloman-yunkan Jun 9, 2024

Choose a reason for hiding this comment

ShaopengLin Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

ShaopengLin commented May 22, 2024 •

edited

Loading

ShaopengLin commented May 22, 2024 •

edited

Loading

kelson42 commented May 23, 2024 •

edited

Loading

ShaopengLin Jun 3, 2024 •

edited

Loading

ShaopengLin Jun 7, 2024 •

edited

Loading

ShaopengLin Jun 9, 2024 •

edited

Loading