fix(MessageView): unbreak opening URLs when clicking on a text message#20939
Conversation
| wrapMode: root.convertToSingleLine ? Text.NoWrap : Text.Wrap | ||
| readOnly: true | ||
| selectByMouse: true // applies to mouse only, not touch | ||
| enabled: !Utils.isMobile // eats the internal touch events to enable the external context menu on long press |
There was a problem hiding this comment.
This is the main fix; we need the (edit) control always enabled for the URLs to be clickable
| } | ||
| onLinkHovered: (link) => disabledLinkTooltip.visible = d.showDisabledTooltipForAddressEnsName(link) | ||
|
|
||
| // context menu handlers |
There was a problem hiding this comment.
... but we need to handle the context menus differently
| root.messageStore.resendMessage(root.messageId) | ||
| } | ||
|
|
||
| onContextMenuRequested: pos => root.openMessageContextMenu(pos) // for StatusTextMessage which would eat the press events internally |
There was a problem hiding this comment.
w/o this, the context menu wouldn't work when right clicking over the text itself; the TextEdit/TextArea has an intrinsic MouseArea that eats the events
| } | ||
|
|
||
| TextEdit { | ||
| StatusTextArea { |
There was a problem hiding this comment.
This change from a TextEdit -> TextArea is needed as the latter at least exposes the pressAndHold and pressed signals, unlike the simple TextEdit
There was a problem hiding this comment.
I feel like we need to test all platforms before merging to make sure this doesn't have a weird side-effect, since it's so core to the messaging part.
There was a problem hiding this comment.
Yup, needs testing for sure but I don't expect any breakage messaging-wise, (Status)TextArea inherits from the prior TextEdit, just adds background and those additional signals that we need to use here
Jenkins BuildsClick to see older builds (25)
|
|
tested APK build 1: The fix works but it has these two issues:
|
caybro
force-pushed
the
fix/20894-messageview-open-links
branch
from
a3b74a6 to
b8af1ad
Compare
Got it, will fix
Yeah that bug is (being) fixed in the other PR (#20883) |
jrainville
left a comment
jrainville
left a comment
There was a problem hiding this comment.
Code looks good. We just need to test it out to be sure it doesn't have weird side effects
| } | ||
|
|
||
| TextEdit { | ||
| StatusTextArea { |
There was a problem hiding this comment.
I feel like we need to test all platforms before merging to make sure this doesn't have a weird side-effect, since it's so core to the messaging part.
| bottomPadding: 0 | ||
| text: d.text | ||
| selectedTextColor: Theme.palette.directColor1 | ||
| selectionColor: Theme.palette.primaryColor3 |
There was a problem hiding this comment.
Not sure if it matters, but the selectionColor will now be primaryColor2 instead of primaryColor3
- use a StatusTextArea as the internal part (renderer) of the StatusTextMessage and make it always enabled (disabling prevents the URLs from being opened) - by using a (Status)TextArea instead of a TextEdit, we can handle the context menu events correctly w/o disabling the edit control Fixes #20894
- on touch screens, the `button` is often `Qt.NoButton` - also fixup opening the image context menus; on touch screens, the Menu.popup() needs a point position to open at the exact point the user (long)pressed Fixes #20897
caybro
force-pushed
the
fix/20894-messageview-open-links
branch
from
d7d4a7f to
feac112
Compare
| function onImageClicked(image, mouse, imageSource, url = "", pos = undefined) { | ||
| switch (mouse.button) { | ||
| case Qt.LeftButton: | ||
| case Qt.NoButton: // touch event |
There was a problem hiding this comment.
Yeah, the TapHandler simply reports Qt.NoButton if the event is not coming from a mouse, which makes sense
sunleos
left a comment
sunleos
left a comment
There was a problem hiding this comment.
tested APK 4:
re
the android selection modal appears as a side effect. See the video: https://drive.google.com/file/d/1wI253sUKPTiaBc2h1h1r8heiXB5SMs2r/view?usp=sharing
confirming a successful fix for this issue
Great job!!!
Let's merge this PR.
7 participants
What does the PR do
Fixes #20894
Fixes #20897
Affected areas
MessageView/Status(Text)Message
Quality checklist
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
Opening a link:
Context menu (on touch):
Impact on end user
How to test
Risk
low