Skip to content
This repository was archived by the owner on Sep 27, 2024. It is now read-only.

Edit link text via dialog.#841

Open
aringenbach wants to merge 6 commits into
matrix-org:mainfrom
aringenbach:edit-link-with-text
Open

Edit link text via dialog.#841
aringenbach wants to merge 6 commits into
matrix-org:mainfrom
aringenbach:edit-link-with-text

Conversation

@aringenbach

@aringenbach aringenbach commented Oct 10, 2023

Copy link
Copy Markdown
Contributor

Replacement for #622

  • Adds a Rust API to allow editing text and url of a link at the same time via the dialog. Previously only the link could be edited in the dialog.
  • Mobile & WASM bindings
  • Adds an implementation of that new API in the iOS example app
  • Slight API break with the change to the LinkAction for Edit now having both parameters (but it doesn't seem to break Android example app at least, and setting only the URL the old way is still compatible)

What's missing from there in this repo: a bit of boilerplate code to use this in Android applications (and maybe Web ?).

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-10-10.at.17.35.47.mp4

@aringenbach aringenbach changed the title Edit link with text Edit link with url and text Oct 10, 2023
@codecov-commenter

codecov-commenter commented Oct 10, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (450db53) 88.08% compared to head (05f95b4) 86.72%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #841      +/-   ##
============================================
- Coverage     88.08%   86.72%   -1.37%     
- Complexity      369      372       +3     
============================================
  Files           161      161              
  Lines         18167    18269     +102     
  Branches        971      985      +14     
============================================
- Hits          16003    15844     -159     
- Misses         1908     2160     +252     
- Partials        256      265       +9     
Flag Coverage Δ
uitests 66.54% <60.00%> (-6.14%) ⬇️
uitests-android 66.54% <60.00%> (-0.04%) ⬇️
uitests-ios ?
unittests 86.09% <31.70%> (-0.27%) ⬇️
unittests-android 44.69% <0.00%> (-0.54%) ⬇️
unittests-ios 76.48% <40.00%> (-0.20%) ⬇️
unittests-rust 89.69% <36.36%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
crates/wysiwyg/src/link_action.rs 100.00% <ø> (ø)
...ent/android/wysiwyg/compose/internal/ViewAction.kt 100.00% <100.00%> (ø)
...roid/wysiwyg/internal/view/models/LinkActionExt.kt 66.66% <100.00%> (ø)
...id/wysiwyg/internal/viewmodel/EditorInputAction.kt 100.00% <100.00%> (ø)
.../element/android/wysiwyg/view/models/LinkAction.kt 100.00% <100.00%> (ø)
...WysiwygComposerView/WysiwygComposerViewModel.swift 81.73% <100.00%> (-11.63%) ⬇️
crates/wysiwyg/src/composer_model/hyperlinks.rs 98.47% <96.00%> (-0.27%) ⬇️
.../element/android/wysiwyg/compose/RichTextEditor.kt 63.21% <0.00%> (+0.42%) ⬆️
...ent/android/wysiwyg/compose/RichTextEditorState.kt 72.97% <50.00%> (-0.64%) ⬇️
...roid/wysiwyg/internal/viewmodel/EditorViewModel.kt 54.67% <75.00%> (+0.60%) ⬆️
... and 6 more

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonnyandrew jonnyandrew requested a review from a team October 11, 2023 08:19
@jonnyandrew

jonnyandrew commented Oct 11, 2023

Copy link
Copy Markdown
Contributor

@aringenbach

aringenbach commented Oct 11, 2023

Copy link
Copy Markdown
Contributor Author

Does not close Android yet unfortunately.
Might not close Web as well, WASM build is fine but maybe additional typescript code is needed on top of that in platforms/web.

@aringenbach

aringenbach commented Oct 11, 2023

Copy link
Copy Markdown
Contributor Author

Actually updated with Android code.
I'm seeing an issue with the compose variant of the example app, but it does seem to happen with the current API too:

  1. Write text
  2. Select it
  3. Create link
  4. => doesn't work (probably because opening the dialog changes the selection in the field into a cursor)

CC @jonnyandrew

@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jonnyandrew

Copy link
Copy Markdown
Contributor

Actually updated with Android code. I'm seeing an issue with the compose variant of the example app, but it does seem to happen with the current API too:

  1. Write text
  2. Select it
  3. Create link
  4. => doesn't work (probably because opening the dialog changes the selection in the field into a cursor)

@aringenbach I pulled main into this work and tested the above and it seems to be fixed, or at least I couldn't reproduce it. I pushed the branch here if you wanted to take a look.

@aringenbach

Copy link
Copy Markdown
Contributor Author

@jonnyandrew Cool, yeah I'm pretty sure it was not directly related (and it is the demo app only anyway)

As far as this feature go, I think it still requires some action from a Web developer to check whatever should be done for the Web to handle that change properly. Probably not too much work, but it needs to be done before this can be released.

Please feel free to create a new PR and close this one.

@langleyd langleyd changed the title Edit link with url and text Edit link text via dialog. Nov 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants