-
-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clickable roomId on message #1268
Changes from all commits
b19a79d
76d360f
0bc4a5a
fd081bd
fc3c828
603d278
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
- [enhance] Copy room link instead of roomId | ||
- [fix] Click on room link will perform appropriate action |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,9 +139,11 @@ class RoomProfilePage extends ConsumerWidget { | |
tiles: [ | ||
SettingsTile( | ||
onPressed: (ctx) { | ||
//FIXME : ?via=$serverName data should be handle from rust helper function | ||
final serverName = roomId.split(':').last; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't really how We can keep this for now, but should probably add a helper function on the rust side that compiles the actual correct information including There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok got it. I will check for matrix specification and do required changes accordingly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have read the documentations, and agree with you that repeating the homeserver doesn't make sense. I'm adding FIXME comment and once rust side helper function is ready then will change it accordingly. |
||
Clipboard.setData( | ||
ClipboardData( | ||
text: roomId, | ||
text: 'https://matrix.to/#/$roomId?via=$serverName', | ||
), | ||
); | ||
customMsgSnackbar( | ||
|
@@ -150,7 +152,7 @@ class RoomProfilePage extends ConsumerWidget { | |
); | ||
}, | ||
title: Text( | ||
'Copy Room ID', | ||
'Copy room link', | ||
style: tileTextTheme, | ||
), | ||
leading: const Icon(Atlas.chain_link_thin, size: 18), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import 'package:acter/common/themes/app_theme.dart'; | ||
import 'package:acter/common/themes/chat_theme.dart'; | ||
import 'package:acter/common/utils/utils.dart'; | ||
import 'package:acter/features/chat/chat_utils/chat_utils.dart'; | ||
import 'package:acter/features/chat/providers/chat_providers.dart'; | ||
import 'package:acter/features/home/providers/client_providers.dart'; | ||
import 'package:acter_flutter_sdk/acter_flutter_sdk_ffi.dart'; | ||
|
@@ -56,7 +57,7 @@ class _TextMessageBuilderConsumerState | |
//remove mx-reply tags. | ||
String parsedString = simplifyBody(widget.message.text); | ||
final urlRegexp = RegExp( | ||
r'https://matrix\.to/#/@[A-Za-z0-9\-]+:[A-Za-z0-9\-]+\.[A-Za-z0-9\-]+', | ||
r'https://matrix\.to/#/[@!#][A-Za-z0-9\-]+:[A-Za-z0-9\-]+\.[A-Za-z0-9\-]+', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. matrix:// is also a valid link we need to check for https://spec.matrix.org/v1.9/appendices/#uris |
||
caseSensitive: false, | ||
); | ||
final matches = urlRegexp.allMatches(parsedString); | ||
|
@@ -153,9 +154,7 @@ class _TextWidget extends ConsumerWidget { | |
maxLines: isReply ? 3 : null, | ||
) | ||
: Html( | ||
onLinkTap: (url) async { | ||
await openLink(url.toString(), context); | ||
}, | ||
onLinkTap: (url) => onLinkTap(url, context, ref), | ||
backgroundColor: Colors.transparent, | ||
data: message.text, | ||
shrinkToFit: true, | ||
|
@@ -184,4 +183,19 @@ class _TextWidget extends ConsumerWidget { | |
], | ||
); | ||
} | ||
|
||
Future<void> onLinkTap(Uri uri, BuildContext context, WidgetRef ref) async { | ||
final roomId = getRoomIdFromLink(uri); | ||
|
||
///If link is type of matrix room link | ||
if (roomId != null) { | ||
await navigateToRoomOrAskToJoin(context, ref, roomId); | ||
} | ||
|
||
///If link is other than matrix room link | ||
///Then open it on browser | ||
else { | ||
await openLink(uri.toString(), context); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matrix://
is also a valid link format: https://spec.matrix.org/v1.9/appendices/#urisThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I get link like
matrix://
from Acter or Element general?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure which clients are sending those at the moment... the element-x iOS maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gnunicorn
No, I have check element web and element iOS. Both are sending
matrix.to
url.And
matrix://
is not a link url,I believe It isurl schema
which is generally used in deep linking.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gnunicorn
matrix://
and test it?matrix://
type of url are consider as url schema then still I have to handle condition for it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gnunicorn
Sorry but I'm not agree with that.
Not sure but based on the my knowledge
matrix://
andmatrix.to
are different things and have their own purposes.matrix://
is Matrix URI scheme which ideally suitable in deep-linking for direct intent callsmatrix.to
is Matrix Navigation URL and widely used by all the matrix platforms but general navigations.For better clarification, I'm looking for any platform which is using
matrix://
(Matrix URI Scheme definition).If you have any idea about such matrix platforms then can you please share it so that I can look into the same and get detail understanding.
And if you don't mind then can we proceed with this PR as this PR is mainly for copying RoomID URL from profile page and user should able to handle it with click on that message like element and other platforms doing because
matrix://
is different thing and don't have any use-case at least in Acter App.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a misunderstanding though. Quoting the spec, about the
matrix.to
it says: "This namespacing existed prior to a matrix: scheme." hinting to the fact that they do the same but the existence of both is just an historically artifact to stay backwards compatible whenmatrix:
was added: Both are used for deep-linking (clicking thematrix.to
-link on a smartphone with element on it, you will see it come up), and targeting the same objects. From a purpose perspective they do and are the same. Withmatrix:
(no double//
after the:
by the way) being the one for the future andmatrix.to
being the fallback.Thus if want to only use one, we should do the
matrix:
-format and not the old one but we are only using the legacy with this PR. If you don't want to implement this now, let's open a ticket saying we needmatrix:
-protocol-support.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gnunicorn for details understanding.
If
matrix://
is the new way then for navigating and deep-link then I would like to integrate that way in Acter and have support for both.@gnunicorn Do you have idea of any of the matrix platform which is using this new way? I just wanted to confirm certain use-cases with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as said, if Element-X isn't using it, I wouldn't know if any does. According to the original spec proposal fluffychat and nheko have support for it (don't know if they post use it primarily though).
FYI, when I say the "new way" of doing it ... the Spec was merged 2021 .... and looks like even Element doesn't use it primarily yet, so ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, I see..
Let me create create new ticket for
"matrix:-protocol-support"
and then we can move on with currentmatrix.to
support.