Skip to content
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

Fix around image use backspace key can't delete break line. #1309

Merged
merged 2 commits into from
Jul 16, 2023

Conversation

TheHeBoy
Copy link
Contributor

@TheHeBoy TheHeBoy commented Jul 16, 2023

Fix around image use backspace key can't delete break line. issue #1306

2023-07-16.11-18-21.mp4

I think EnsureEmbedLineRule and InsertEmbedsRule both apply for embed.EnsureEmbedLineRule only work for video, but InsertEmbedsRule for all embed. Must idea control block or inline element by expend of embed.

/// Handles all format operations which manipulate embeds.
/// This rule wraps line breaks around video, not image.
class InsertEmbedsRule extends InsertRule {
  const InsertEmbedsRule();

  @override
  Delta? applyRule(Delta document, int index,
      {int? len, Object? data, Attribute? attribute}) {
    if (data is String) {
      return null;
    }

    assert(data is Map);

    if (!(data as Map).containsKey(BlockEmbed.videoType)) {
      return null;
    }

@singerdmx
Copy link
Owner

This PR contains previous changes

@singerdmx
Copy link
Owner

Can you rebase?

@singerdmx
Copy link
Owner

Is this about image or video?

@TheHeBoy TheHeBoy closed this Jul 16, 2023
@TheHeBoy TheHeBoy force-pushed the master branch 2 times, most recently from 3a01165 to a353ef1 Compare July 16, 2023 04:54
@TheHeBoy TheHeBoy reopened this Jul 16, 2023
@TheHeBoy
Copy link
Contributor Author

About image.

@@ -118,6 +119,12 @@ class EnsureEmbedLineRule extends DeleteRule {
final itr = DeltaIterator(document);

var op = itr.skip(index);
final opAfter = itr.skip(index + 1);

if (!_isVideo(op) || !_isVideo(opAfter)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add comment what this is about?

Copy link
Owner

Choose a reason for hiding this comment

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

It seems that EnsureEmbedLineRule should be renamed to EnsureVideoLineRule?
This rule is only applicable to video?
why is it not applicable to image?

Copy link
Owner

Choose a reason for hiding this comment

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

Does this change work with content that has video?

Copy link
Owner

@singerdmx singerdmx Jul 16, 2023

Choose a reason for hiding this comment

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

If you want to exclude image, you can check:

if (_isImage(op) || _isImage(opAfter)) {
   return null;
}

Copy link
Owner

Choose a reason for hiding this comment

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

You basically want to exclude image from this rule, right?

Copy link
Owner

Choose a reason for hiding this comment

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

Can you add one line to class comment:

/// This rule applies to video, not image.

Copy link
Owner

Choose a reason for hiding this comment

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

Please update changelog.
The change makes sense to me now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't understand when need to update changelog, Every time pr?

Copy link
Owner

Choose a reason for hiding this comment

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

If this change needs to be published as a new version

@singerdmx singerdmx merged commit 0ab6027 into singerdmx:master Jul 16, 2023
1 check passed
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.

2 participants