Skip to content

Conversation

@provokateurin
Copy link
Member

@provokateurin provokateurin commented Apr 13, 2024

The algorithm for splitting the message and replacing the rich objects might not be the best one, but I found this to be relatively simple and I couldn't think of a much better solution.

The first commit basically backports a change that will also come with #1894, so we might just want to wait for 29 to land before merging this.

Not all parameter types are supported yet, because i wasn't able to test the maps integration for geo-location (I think it is broken) and displaying vcards can be done later as it is not that important imo. Polls are also not implemented, but that's because they are a whole different thing we need to support and it would bloat this PR which just gets the basics into place.

@provokateurin provokateurin requested a review from Leptopoda April 13, 2024 07:17
@provokateurin provokateurin force-pushed the feat/neon_talk/inline-message-rendering branch from 939f30f to 68812be Compare April 13, 2024 07:29
@codecov
Copy link

codecov bot commented Apr 13, 2024

Codecov Report

Attention: Patch coverage is 38.71560% with 334 lines in your changes are missing coverage. Please review.

Project coverage is 28.82%. Comparing base (6a55847) to head (b2a6519).
Report is 3 commits behind head on main.

❗ Current head b2a6519 differs from pull request most recent head 3184e37. Consider uploading reports for the commit 3184e37 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1910      +/-   ##
==========================================
+ Coverage   28.75%   28.82%   +0.07%     
==========================================
  Files         198      203       +5     
  Lines       67697    68243     +546     
==========================================
+ Hits        19466    19674     +208     
- Misses      48231    48569     +338     
Flag Coverage Δ *Carryforward flag
dynamite 44.18% <ø> (ø) Carriedforward from 83e6624
dynamite_end_to_end_test 61.38% <ø> (ø) Carriedforward from 83e6624
dynamite_runtime 82.65% <ø> (ø) Carriedforward from 83e6624
neon_dashboard 92.56% <ø> (ø)
neon_framework 37.64% <ø> (ø)
neon_talk 95.77% <85.34%> (-4.23%) ⬇️
nextcloud 25.76% <26.10%> (-0.01%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
...ckages/neon/neon_talk/lib/src/widgets/message.dart 100.00% <100.00%> (ø)
...neon_talk/lib/src/widgets/rich_object/mention.dart 100.00% <100.00%> (ø)
...on_talk/lib/src/widgets/rich_object/deck_card.dart 80.00% <80.00%> (ø)
...eon_talk/lib/src/widgets/rich_object/fallback.dart 81.25% <81.25%> (ø)
...on/neon_talk/lib/src/widgets/rich_object/file.dart 66.66% <66.66%> (ø)
packages/nextcloud/lib/src/api/spreed.openapi.dart 33.71% <45.65%> (+0.10%) ⬆️
...ckages/nextcloud/lib/src/api/spreed.openapi.g.dart 12.30% <23.75%> (+0.20%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm for splitting the message and replacing the rich objects might not be the best one, but I found this to be relatively simple and I couldn't think of a much better solution.

I didn't try to 100% understand the algorithm but if it does the job let's go with it.


Screenshot_20240414_111508

I think that the TalkRichObjectMention has to respect the text scale. Optimally the entire chip should not be larger than the surrounding text. Now the text in the chip is already larger.

@provokateurin provokateurin force-pushed the feat/neon_talk/inline-message-rendering branch from 68812be to fa0b80f Compare April 14, 2024 13:44
@provokateurin provokateurin requested a review from Leptopoda April 14, 2024 13:44
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to propagate the textstyle through the BuildContext? That should allow future mistakes.

I suggest to wrap the entire systemMessage in a DefaultTextStyle widget and let the children build according to the current default style.

@provokateurin
Copy link
Member Author

I suggest to wrap the entire systemMessage in a DefaultTextStyle widget and let the children build according to the current default style.

This doesn't work for the Chips because they rely on the ChipThemeData and do not consider the DefaultTextStyle. I think we will just have to live with it, even though I like your idea.

@provokateurin provokateurin force-pushed the feat/neon_talk/inline-message-rendering branch from fa0b80f to b2a6519 Compare April 14, 2024 16:48
@provokateurin provokateurin requested a review from Leptopoda April 14, 2024 16:48
@provokateurin provokateurin force-pushed the feat/neon_talk/inline-message-rendering branch from b2a6519 to 3184e37 Compare April 15, 2024 16:58
@provokateurin provokateurin enabled auto-merge April 15, 2024 16:58
@provokateurin provokateurin merged commit 761b21c into main Apr 15, 2024
@provokateurin provokateurin deleted the feat/neon_talk/inline-message-rendering branch April 15, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants