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

Add mention insertion methods #698

Merged
merged 120 commits into from
Jun 12, 2023
Merged

Conversation

alunturner
Copy link
Contributor

@alunturner alunturner commented May 30, 2023

This change makes breaking changes to the API. This PR incorporates changes to web, iOS and Android to account for the selection logic changes required and the use of the new lib functions.

This PR:

  • changes name of internal function to set_mention_from_suggestion and updates bindings
  • expands some of the ffi tests for this function
  • exposes a new function in the bindings files
  • creates a new mentions.rs file to handle the renamed function
  • creates a new insert_node_at_cursor.rs file to allow insertion of a node at a cursor with unit tests
  • adds extensive tests for mentions in test_mentions.rs
  • tweaks behaviour of suggestion monitoring to disallow it inside links
  • implements the corresponding selection changes required in web due to mention nodes now having length 1
    - implements iOS changes
    - implements Android changes

alunturner and others added 30 commits May 24, 2023 15:52
* Add Mention container node type and initial to markdown implementation
* Make new container node kind correspond to link dom node
* Add new_mention function
* update all references to ContainerNodeType::Link with new mention type
* add new_mention for dom node, add TODO comments for parsing
* add TODO - app compiling, tests passing
* add TODO for tree display
* change signature for set_link
* update bindings
* update tests
* add TODO for deletion
* remove unused parameters (web lib.rs)
* change set_link and set_link_in_range signatures
* update bindings
* update tests
* fix tests
* rejig arguments for usability
* rename to set_mention_from_suggestion
* fix function calls, do not pass in contenteditable
* fix mobile binding arguments
* refactor to not pass attributes to new_links
Base automatically changed from jonny/distinct-mention-node-wip to main June 5, 2023 14:11
@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2023

Codecov Report

Patch coverage: 89.42% and project coverage change: +0.09 🎉

Comparison is base (cfda115) 88.28% compared to head (a3cadae) 88.38%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #698      +/-   ##
============================================
+ Coverage     88.28%   88.38%   +0.09%     
+ Complexity      274      268       -6     
============================================
  Files           145      146       +1     
  Lines         16652    17003     +351     
  Branches        769      789      +20     
============================================
+ Hits          14702    15028     +326     
- Misses         1765     1785      +20     
- Partials        185      190       +5     
Flag Coverage Δ
uitests 73.63% <75.38%> (-1.59%) ⬇️
uitests-android 64.10% <75.00%> (-1.53%) ⬇️
uitests-ios 82.31% <75.60%> (-1.85%) ⬇️
unittests 88.09% <86.89%> (+0.28%) ⬆️
unittests-android 58.82% <55.81%> (+0.54%) ⬆️
unittests-ios 79.05% <89.02%> (+0.64%) ⬆️
unittests-rust 90.02% <90.02%> (+0.22%) ⬆️

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

Impacted Files Coverage Δ
bindings/wysiwyg-wasm/src/lib.rs 20.10% <0.00%> (-0.44%) ⬇️
crates/wysiwyg/src/composer_model/hyperlinks.rs 98.74% <ø> (-0.09%) ⬇️
...lement/android/wysiwyg/view/spans/CodeBlockSpan.kt 24.00% <ø> (ø)
...ysiwygComposer/Components/WysiwygMentionType.swift 100.00% <ø> (ø)
bindings/wysiwyg-ffi/src/ffi_composer_model.rs 31.08% <8.69%> (+6.35%) ⬆️
...roid/wysiwyg/internal/viewmodel/EditorViewModel.kt 44.88% <50.00%> (ø)
.../java/io/element/android/wysiwyg/EditorEditText.kt 52.55% <66.66%> (-0.41%) ⬇️
...element/android/wysiwyg/utils/HtmlToSpansParser.kt 83.03% <69.69%> (-3.91%) ⬇️
...nsions/DTCoreText/PlaceholderTextHTMLElement.swift 88.88% <84.61%> (-3.97%) ⬇️
...MLParser/Extensions/DTCoreText/DTHTMLElement.swift 98.50% <92.30%> (-1.50%) ⬇️
... and 16 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@jonnyandrew jonnyandrew left a comment

Choose a reason for hiding this comment

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

Looks good to me, but we'll need to update all platform code as part of this PR as it is a breaking change

platforms/web/lib/dom.test.ts Outdated Show resolved Hide resolved
suggestion: SuggestionPattern,
attributes: Vec<(S, S)>,
) -> ComposerUpdate<S> {
// This function removes the text between the suggestion start and end points, updates the
Copy link
Contributor

Choose a reason for hiding this comment

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

We must self.push_state_to_history(); here, before we start making changes to the composer.

self.state.start = Location::from(suggestion.start);
self.state.end = self.state.start;

self.insert_mention(url, text, attributes)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should really rather call the internal do_insert_mention. If the test around being inside a link is required, it should be done before everything else (and be extracted to an util function run before push_state_to_history)

crates/wysiwyg/src/composer_model/mentions.rs Show resolved Hide resolved
crates/wysiwyg/src/composer_model/mentions.rs Show resolved Hide resolved
let new_node = DomNode::new_mention(url, text, attributes);
let new_cursor_index = start + new_node.text_len();

self.push_state_to_history();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably never push_state_to_history inside inner variant do_... in case of reuse somewhere else (since it creates an undo/redo state - it should always be called in the public API function)

self.state.start = Location::from(new_cursor_index);
self.state.end = self.state.start;

// add a trailing space in cases when we do not have a next sibling
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this was already discussed at some point, the conversation lies somewhere in our Room history, but yeah adding a space after the inserted trailing function only is the way to go.

fn test_set_link_suggestion_ffi() {
let model = Arc::new(ComposerModel::new());
fn test_replace_whole_suggestion_with_mention_ffi() {
let mut model = Arc::new(ComposerModel::new());
let update = model.replace_text("@alic".into());
Copy link
Contributor

Choose a reason for hiding this comment

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

update is unused now

@alunturner
Copy link
Contributor Author

I've made those changes @aringenbach . Extracting the logic check to a util is probably a good move too as we're going to change that behaviour when issue #702 gets addressed anyway.

crates/wysiwyg/src/composer_model/mentions.rs Outdated Show resolved Hide resolved
crates/wysiwyg/src/composer_model/mentions.rs Outdated Show resolved Hide resolved
crates/wysiwyg/src/composer_model/mentions.rs Outdated Show resolved Hide resolved
bindings/wysiwyg-ffi/src/wysiwyg_composer.udl Outdated Show resolved Hide resolved
alunturner and others added 4 commits June 8, 2023 10:34
Co-authored-by: aringenbach <[email protected]>
Co-authored-by: aringenbach <[email protected]>
Co-authored-by: aringenbach <[email protected]>
Co-authored-by: aringenbach <[email protected]>
jonnyandrew and others added 2 commits June 12, 2023 09:31
* Update Android to new mention insertion API
* Add issue link to todo
* [iOS] Handle selection offsets for mention node
* [iOS] Rename `PermalinkReplacer` to `MentionReplacer`
* Use insert_mention API and fix tests
@sonarcloud
Copy link

sonarcloud bot commented Jun 12, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@aringenbach aringenbach left a comment

Choose a reason for hiding this comment

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

LGTM ! I also triggered the mobile platforms tests that seem to be all green.

@alunturner alunturner merged commit b431568 into main Jun 12, 2023
@alunturner alunturner deleted the alunturner/add-mention-insertion-methods branch June 12, 2023 11:18
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.

4 participants