Fix double click to select full words #5243

KleberPF · 2024-03-10T03:25:37Z

Leaving this as a draft because I want some opinions if this is a good approach or not. Heavily inspired by this comment from @Nerixyz, but adding an id to each element instead of the prev/next approach. Currently works on basic tests, haven't tested more advanced stuff (RTL text, CJK characters, etc)

Nerixyz

Using IDs for words is a good idea. However, I think the concept shouldn't "leak" beyond MessageLayout, i.e. ChannelView and *MessageBuilder shouldn't know about it.

One bug this implementation has is that it assumes that one TextElement has at most one word. This isn't the case. The EmotePopup for example, adds an element with multiple words:

chatterino2/src/widgets/dialogs/EmotePopup.cpp

Lines 53 to 55 in b9b1b8c

    
           builder.emplace<TextElement>("no emotes available", 
        
                                        MessageElementFlag::Text, 
        
                                        MessageColor::System);

MessageLayoutContainer should generate the IDs. When a TextElement adds its elements to the container, it generates a new ID for each word by calling something like container.nextWordID(). This way, message builders don't need to know about word-IDs. (if you put the next ID after containsRTL, it doesn't consume extra space)

Getting the word bounds should hide the concept of word-IDs too. It should have a single method like layout.getWordBoundsAt(QPoint) which finds the layout element and finds the bounds of all layout elements with that word-ID (or returns the element's bounds if it has an ID of -1).

Nerixyz · 2024-03-10T09:38:02Z

src/messages/layouts/MessageLayoutContainer.cpp

+        wordEnd += element->hasTrailingSpace()
+                       ? element->getSelectionIndexCount() - 1
+                       : element->getSelectionIndexCount();


This works¹ because elements created from one word don't have trailing spaces in between. However, this should be handled here. Only if the last element had a trailing space, the selection index should be reduced by one.

Footnotes

Actually, it doesn't work here, as this can be called with -1 in which case multiple elements can be selected and the selection won't span the entire group. ↩

Now this is done after the loop checking the last element

Nerixyz · 2024-03-10T09:41:59Z

src/widgets/helper/ChannelView.cpp

@@ -1817,7 +1817,7 @@ void ChannelView::mouseMoveEvent(QMouseEvent *event)
    if (this->isDoubleClick_ && hoverLayoutElement)
    {
        auto [wordStart, wordEnd] =
-            getWordBounds(layout.get(), hoverLayoutElement, relativePos);
+            layout->getWordBounds(hoverLayoutElement->getWordId());


This can pass -1 to getWordBounds. Example:

Open emote popup in a channel that that doesn't have BTTV, FFZ or 7TV emotes

Click on Channel, find no emotes available

Double click next to the text (a few pixels)

Quickly move the mouse over the text

Now we choose which logic to use inside layout->getWordBounds

Nerixyz · 2024-03-10T09:52:23Z

src/messages/layouts/MessageLayoutElement.hpp

+    /**
+     * Elements that are part of the same word originally have the same id
+     */


(See top level comment)

Suggested change

/**

* Elements that are part of the same word originally have the same id

*/

/// @brief ID of a word inside its container

///

/// One word has exactly one ID that is used to identify elements created

/// from the same word (due to wrapping).

/// IDs are unique in a MessageLayoutContainer.

Btw, if you put this before text_, it won't consume extra space and fit into the padding.

KleberPF · 2024-03-10T22:24:22Z

Thanks for the feedback. I think I implemented everything you pointed out, and it seems to be working well. There might be some off by one errors because the messages are kinda scuffed on KDE Plasma

I can fix it a bit by shrinking and stretching the window, but it doesn't fix 100%.

KleberPF · 2024-03-13T01:33:13Z

I tested this a bit more and everything seems to be working. Not sure if there is any point in documenting the tests as they would be just screenshots of random selections but let me know if I should. I'll remove the draft as it seems the final version will be something close to this.

pajlada

I pushed some stuff, mostly small things. Could you review the comments I added to make sure I understood the code correctly?

src/messages/layouts/MessageLayout.cpp

pajlada · 2024-03-14T20:54:01Z

src/messages/layouts/MessageLayout.cpp

+    if (element->getWordId() != -1)
+    {
+        return this->container_.getWordBounds(element);
+    }


Could you add a comment here for what this significes? It's not clear to me what it means when the element we're looking at has a word id of -1

-1 basically means that the element can't span across multiple lines (like an emote, for example). I agree the code is not very self explanatory here. Do you want a comment here or should I encapsulate that inside a function like element->canBeMultiline() (name is kinda bad but you get the idea)

I think a comment here is fine

src/messages/layouts/MessageLayoutContainer.cpp

tests/src/MessageLayout.cpp

KleberPF · 2024-03-16T05:46:36Z

tests/src/MessageLayout.cpp

+        this->layout->layout(WIDTH, 1, MessageElementFlag::Text, false);
+    }
+
+    MockApplication mockApplication;


I still don't quite understand how constructing MockApplication here makes it so any all to getIApp gets the MockApplication. Also this should probably only be done once instead of each time a test is created

MockApplication inherits from mocks::EmptyApplicatino, which inherits from IApplication which sets a singleton to point to itself (check mocks/EmptyApplication & src/Application.cpp)

tests/src/MessageLayout.cpp

src/messages/MessageElement.cpp

pajlada · 2024-03-16T13:41:30Z

Testing RTL selections - it's very scuffed. Luckily, it's already scuffed before this PR, which means I don't think it's a blocker.

Draft: Fix double click to select full words

bcad63d

Nerixyz reviewed Mar 10, 2024

View reviewed changes

Add suggestions

20b8e67

KleberPF added 2 commits March 10, 2024 19:37

Removed unused stuff

65640e9

Remove outdated comment

7dd669d

KleberPF marked this pull request as ready for review March 13, 2024 01:33

KleberPF changed the title ~~Draft: Fix double click to select full words~~ Fix double click to select full words Mar 13, 2024

pajlada added 5 commits March 14, 2024 20:56

Merge remote-tracking branch 'origin/master' into fix-word-selection

8f4a717

nit: make MessageLayoutContainer::getWordBounds const

b162f23

constify getWordBounds & getElementAt

a60c06c

Comment getWordBounds

cfa491d

Comment nextWordId

bbbed0c

pajlada reviewed Mar 14, 2024

View reviewed changes

KleberPF added 2 commits March 16, 2024 02:41

Add review suggestions

24b02bc

Kinda add tests

2090d00

KleberPF commented Mar 16, 2024

View reviewed changes

src/messages/MessageElement.cpp Show resolved Hide resolved

KleberPF and others added 6 commits March 16, 2024 02:53

Add missing comment

91060a6

sort includes & add newline

ed2dd85

Run formatter

bf73811

Update tests & comment for getWordBounds

2641b05

Removed unused includes

269e77d

Add changelog entry

0f7a74d

pajlada approved these changes Mar 16, 2024

View reviewed changes

pajlada added this to the Investigate before 2.5.0 milestone Mar 16, 2024

pajlada enabled auto-merge (squash) March 17, 2024 13:17

Merge branch 'master' into fix-word-selection

cd9daf6

pajlada merged commit c10e364 into Chatterino:master Mar 17, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix double click to select full words #5243

Fix double click to select full words #5243

KleberPF commented Mar 10, 2024

Nerixyz left a comment

Nerixyz Mar 10, 2024

KleberPF Mar 10, 2024

Nerixyz Mar 10, 2024

KleberPF Mar 10, 2024

Nerixyz Mar 10, 2024

KleberPF Mar 10, 2024

KleberPF commented Mar 10, 2024 •

edited

KleberPF commented Mar 13, 2024

pajlada left a comment

pajlada Mar 14, 2024

KleberPF Mar 15, 2024

pajlada Mar 15, 2024

KleberPF Mar 16, 2024

KleberPF Mar 16, 2024

pajlada Mar 16, 2024 •

edited

pajlada commented Mar 16, 2024

	builder.emplace<TextElement>("no emotes available",
	MessageElementFlag::Text,
	MessageColor::System);

-    /**
-     * Elements that are part of the same word originally have the same id
-     */
+    /// @brief ID of a word inside its container
+    ///
+    /// One word has exactly one ID that is used to identify elements created
+    /// from the same word (due to wrapping).
+    /// IDs are unique in a MessageLayoutContainer.

Fix double click to select full words #5243

Fix double click to select full words #5243

Conversation

KleberPF commented Mar 10, 2024

Nerixyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KleberPF commented Mar 10, 2024 • edited

KleberPF commented Mar 13, 2024

pajlada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pajlada Mar 16, 2024 • edited

Choose a reason for hiding this comment

pajlada commented Mar 16, 2024

KleberPF commented Mar 10, 2024 •

edited

pajlada Mar 16, 2024 •

edited