-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
[Chat-ng]: Appflowy Editor editor integration #2332
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2332 +/- ##
==========================================
- Coverage 27.05% 26.91% -0.15%
==========================================
Files 596 615 +19
Lines 41433 42146 +713
==========================================
+ Hits 11211 11342 +131
- Misses 30222 30804 +582
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
b3fd900
to
f0ab803
Compare
@gtalha07 the video doesn't load here. secondly, I tried to start reviewing but my focus is a bit scattered this week and I couldn't get through... so you might want to ping someone else to do it if you want this in earlier... |
Let me do a review although I don't understand much details about |
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 will basic review of this PR. However I don't much understand mentioned and editor related work.
Also can I have video reference for Room/User mention related flow?
app/lib/common/widgets/html_editor/components/user_mention_block.dart
Outdated
Show resolved
Hide resolved
app/lib/common/widgets/html_editor/components/room_mention_block.dart
Outdated
Show resolved
Hide resolved
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.
It is quite difficulty to judge mentioned related code review without it's visual reference. More over seeing issues in basic code standards during review.
And I see that after first round of PR review, there are additional thing gets added to this PR with below commit which makes PR review process quite difficulty and need to review whole things again carefully to track entire changes.
3c5a6af
I am not feeling confident to approve this PR, I would like to have once round of review from @gnunicorn whenever he gets some time.
app/lib/common/widgets/html_editor/components/mention_block.dart
Outdated
Show resolved
Hide resolved
app/lib/common/widgets/html_editor/components/mention_block.dart
Outdated
Show resolved
Hide resolved
app/lib/common/widgets/html_editor/components/mention_content.dart
Outdated
Show resolved
Hide resolved
app/lib/common/widgets/html_editor/components/mention_content.dart
Outdated
Show resolved
Hide resolved
app/lib/common/widgets/html_editor/components/mention_content.dart
Outdated
Show resolved
Hide resolved
app/lib/common/widgets/html_editor/components/mention_handler.dart
Outdated
Show resolved
Hide resolved
app/lib/common/widgets/html_editor/components/mention_handler.dart
Outdated
Show resolved
Hide resolved
|
||
final node = widget.editorState.getNodeAtPath(selection.end.path); | ||
if (node == null) return; | ||
|
||
final text = node.delta?.toPlainText() ?? ''; | ||
if (text.length < startOffset) return; | ||
|
||
setState(() { | ||
_search = text.substring(startOffset); | ||
}); | ||
} | ||
|
||
void _selectItem(String id, String displayName) { | ||
final selection = widget.editorState.selection; | ||
if (selection == null) return; | ||
|
||
final transaction = widget.editorState.transaction; | ||
final node = widget.editorState.getNodeAtPath(selection.end.path); | ||
if (node == null) return; | ||
|
||
// Delete the search text and trigger character | ||
transaction.deleteText( | ||
node, | ||
startOffset - 1, | ||
selection.end.offset - startOffset + 1, | ||
); | ||
|
||
// Insert the mention | ||
transaction.insertText( | ||
node, | ||
startOffset - 1, | ||
displayName.isNotEmpty ? displayName : id, | ||
attributes: { | ||
MentionBlockKeys.mention: { | ||
MentionBlockKeys.type: widget.mentionType.name, | ||
if (widget.mentionType == MentionType.user) | ||
MentionBlockKeys.userId: id | ||
else | ||
MentionBlockKeys.roomId: id, | ||
}, | ||
}, | ||
); | ||
|
||
widget.editorState.apply(transaction); | ||
widget.onDismiss(); | ||
} | ||
|
||
void _scrollToSelected() { | ||
final itemPosition = _selectedIndex * kItemHeight; | ||
if (itemPosition < _scrollController.offset) { | ||
_scrollController.animateTo( | ||
itemPosition, | ||
duration: const Duration(milliseconds: 200), | ||
curve: Curves.easeOut, | ||
); | ||
} else if (itemPosition + kItemHeight > | ||
_scrollController.offset + | ||
_scrollController.position.viewportDimension) { | ||
_scrollController.animateTo( | ||
itemPosition + | ||
kItemHeight - | ||
_scrollController.position.viewportDimension, | ||
duration: const Duration(milliseconds: 200), | ||
curve: Curves.easeOut, | ||
); | ||
} | ||
} | ||
|
||
void _deleteCharacterAtSelection() { | ||
final selection = widget.editorState.selection; | ||
if (selection == null || !selection.isCollapsed) { | ||
return; | ||
} | ||
|
||
final node = widget.editorState.getNodeAtPath(selection.end.path); | ||
final delta = node?.delta; | ||
if (node == null || delta == null) { | ||
return; | ||
} | ||
|
||
_search = delta.toPlainText().substring( | ||
startOffset, | ||
startOffset - 1 + _search.length, | ||
); | ||
} | ||
|
||
bool _canDeleteLastCharacter() { | ||
final selection = widget.editorState.selection; | ||
if (selection == null || !selection.isCollapsed) { | ||
return false; | ||
} | ||
|
||
final delta = widget.editorState.getNodeAtPath(selection.start.path)?.delta; | ||
if (delta == null) { | ||
return false; | ||
} | ||
|
||
return delta.isNotEmpty; | ||
} | ||
} |
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.
Ignoring code review of this part as I am not quite understanding logic here.
app/lib/common/widgets/html_editor/components/mention_menu.dart
Outdated
Show resolved
Hide resolved
app/lib/common/widgets/html_editor/components/mention_menu.dart
Outdated
Show resolved
Hide resolved
import 'package:flutter/material.dart'; | ||
import 'package:flutter_riverpod/flutter_riverpod.dart'; | ||
|
||
class MentionBlock extends ConsumerWidget { |
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.
The UI component of the inline mentions in editor (of how it should be displayed when triggered)
import 'package:acter_avatar/acter_avatar.dart'; | ||
import 'package:flutter/material.dart'; | ||
|
||
class MentionItem extends StatelessWidget { |
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.
Mention list UI item component which gets displayed as part of mention menu list.
} | ||
} | ||
|
||
void _selectItem(String id, String displayName) { |
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.
handler when user selects mention item from list and processes how the editor should execute mention block inline
widget.onDismiss(); | ||
} | ||
|
||
void _scrollToSelected() { |
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.
useful for navigating mention list through keyboard navigation arrows (up, down) to scroll accordingly using custom scroll controller
} | ||
} | ||
|
||
bool _canDeleteLastCharacter() { |
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.
checks whether editor state selection is at mention block.
); | ||
} | ||
|
||
InlineSpan customizeAttributeDecorator( |
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.
For customizing the styling of custom component block, here we can extend it beyond mentions (for future reference).
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 have not done a full review and I don't pretend that I have understood this whole, but I'd like to see two major suggestions applied before going forward, as it will make it easier to understand what is even meant to be going on:
- using typed content for the Attributes and everywhere really, rather than a bunch of keyed-strings of keys for a
Map
... that isn't necessary and makes it harder for the reader and the computer to confirm this is correct - remove the
isDesktop
-checks. They are almost always wrong and they are for sure here ... if we have a hardware keyboard or a mouse isn't platform specific and pretending it is makes the code more complex than just dealing with that being the case...
@@ -0,0 +1 @@ | |||
- Chat-NG now supports Appflowy editor with proper markdown support. Some/Other features might be disabled or limited for now. |
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.
- Chat-NG now supports Appflowy editor with proper markdown support. Some/Other features might be disabled or limited for now. | |
- [Labs] Chat-NG now offers a fresh new WYSIWYG editor. More features upcoming. |
final Map<String, dynamic> mention; | ||
final String userRoomId; | ||
final Node node; | ||
final int index; |
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.
don't we usually keep the list above the constructor?
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.
Hmm, yes. I can change it as I didn't really seem to thought much about it before.
final int index; | ||
@override | ||
Widget build(BuildContext context, WidgetRef ref) { |
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.
final int index; | |
@override | |
Widget build(BuildContext context, WidgetRef ref) { | |
final int index; | |
@override | |
Widget build(BuildContext context, WidgetRef ref) { |
) { | ||
return [ | ||
CharacterShortcutEvent( | ||
character: '@', |
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.
let's use a global static for these!?!
final isDesktop = desktopPlatforms.contains(Theme.of(context).platform); | ||
|
||
return Container( | ||
height: 60, | ||
// selection color is only for desktop with keyboard navigation | ||
color: (isSelected && isDesktop) | ||
? Theme.of(context).colorScheme.primary | ||
: null, |
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.
isDesktop
is an incorrect check here: pads can also have keyboards and laptops might also have touch devices. what I think you want to detect for here is a hardware keyboard, no?
but then also, how would isSelected
even come about if not through a hardware-keyboard? I think this check is pointless and should be removed and the rest left to the outer part.
final isDesktop = desktopPlatforms.contains(Theme.of(context).platform); | |
return Container( | |
height: 60, | |
// selection color is only for desktop with keyboard navigation | |
color: (isSelected && isDesktop) | |
? Theme.of(context).colorScheme.primary | |
: null, | |
return Container( | |
height: 60, | |
color: isSelected ? Theme.of(context).colorScheme.primary : null, |
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 see, good point. I'll change the conditional 👍🏻 !
attributes: { | ||
MentionBlockKeys.mention: { | ||
MentionBlockKeys.type: widget.mentionType.name, | ||
if (widget.mentionType == MentionType.user) | ||
MentionBlockKeys.userId: id | ||
else | ||
MentionBlockKeys.roomId: id, | ||
MentionBlockKeys.displayName: displayName, | ||
}, | ||
}, |
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.
so ... attributes
Attributes = Map<String, dynamic
>, right? so, why not have the "mention"-string-key point to a typed object. all those MentionBlockKeys
-with strings make it really hard to follow and easy to mess up. I'd rather have a typed:
attributes: { | |
MentionBlockKeys.mention: { | |
MentionBlockKeys.type: widget.mentionType.name, | |
if (widget.mentionType == MentionType.user) | |
MentionBlockKeys.userId: id | |
else | |
MentionBlockKeys.roomId: id, | |
MentionBlockKeys.displayName: displayName, | |
}, | |
}, | |
attributes: { | |
MENTIONS_KEY: widget.mentionType == MentionType.user ? | |
MentionAttributes.user(userId: id, displayName: displayName) | |
MentionAttributes.room(roomId: id, displayName: displayName) | |
}, |
For a type MentionAttributes
, along the lines of (pseudocode):
class MentionAttributes {
final String? userId;
final String? roomId;
final String? displayName;
final MentionType type;
const MentionAttributes({this.userId, this.roomId, required this.type, this.displayName});
factory MentionAtrributes.user(required this.userId, {this.displayName}) {
type = MentionType.user;
}
}
if (isDesktop) { | ||
// we only want to listen keyboard events on desktop | ||
return Focus( | ||
focusNode: _focusNode, | ||
onKeyEvent: (node, event) => _handleKeyEvent(node, event, suggestions), | ||
child: menuWidget, | ||
); | ||
} |
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.
We can disregard this flaky desktop check (iPads and Mobiles can have hardware keyboards attached to it) by directly interacting with HardwareKeyboardDetector
. Which we should be doing here rather than doing some flaky check and focus grabbing ...
|
||
@override | ||
Widget build(BuildContext context) { | ||
final isDesktop = desktopPlatforms.contains(Theme.of(context).platform); |
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.
reminder: this is almost always bad. checking for the specific feature you want is probably the better idea. In this case: check for/directly use the actual hardware keyboard..
required WidgetRef ref, | ||
required AvatarOptions avatarOptions, | ||
}) { | ||
final desktopPlatforms = [ |
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.
mouse can also be plugged/connected to phones. don't do a check like that.
The first set iterative change is to integrate Appflowy Editor integration (with built-in set of mention components block) as a chat-ng input component which will support markdown formatting. This'll also fix the bugs we've seen with input format which we have to accustom with various handlers for ensuring that proper markdown is passed and that won't be needed since the editor would emit correct format and ensure the renderer catches it correctly and display.
As this section of the codebase is pretty complex and time-consuming to follow, the input component isn't entirely migrated. As it accounts numerous issues being appearing when trying to do so. So, having it started from the ground is the best course to take but that also means it's limited in its functionality compared to current state of
CustomInput
.This only contains functional wise:
editorState
current state.editorState
.But also improves UI of the input:
Desktop Demo with Mentions UI:
Screen.Recording.2024-11-10.at.9.43.30.PM.mov
Mobile UI:
ScreenRecording_11-10-2024.21-39-27_1.mp4