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

✨ Add "toggle block quote" #811

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

Sean10
Copy link

@Sean10 Sean10 commented Sep 12, 2020

support scene

  • quote/unquote the cursor line
  • quote/unquote the selection lines.
    • if some lines haven't been quote, quote them.
    • if all of the lines have been quoted, unquote them.

#704

Copy link
Owner

@yzhang-gh yzhang-gh left a comment

Choose a reason for hiding this comment

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

Many thanks! Just a few comments.

package.json Outdated Show resolved Hide resolved
@@ -45,6 +46,74 @@ function toggleCodeBlock() {
return editor.insertSnippet(new SnippetString('```$0\n$TM_SELECTED_TEXT\n```'));
}

enum QuoteState {
// State 1: part of lines has been quoted, need to be quoted
PARTIALQUOTED,
Copy link
Owner

Choose a reason for hiding this comment

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

I think it is more natural to quote all the lines (not only the unquoted lines) in this case (thinking about doing multi-line comment).

Copy link
Author

Choose a reason for hiding this comment

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

In consideration of the selected text not always accurate, especially like i use three finder drag, i may select more lines which have been quoted. In this situation, what i need is just to quote the unquoted lines. There is no need to quote which have been quoted which may cause the text format disturbed. So could you share the example which is preferred to quote all selected lines?

Copy link
Owner

Choose a reason for hiding this comment

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

In consideration of the selected text not always accurate, especially like i use three finder drag, i may select more lines which have been quoted. In this situation, what i need is just to quote the unquoted lines.

I agree this is probably a good design (for some people, or maybe the majority). But as a first step, I would like to make it consistent with other similar commands, e.g.

multi-line

Copy link
Author

Choose a reason for hiding this comment

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

Maybe add a switch to decide wthether need to only quote unquoted lines so that default quote behavior is the same as others?

Copy link

@huyz huyz Mar 31, 2024

Choose a reason for hiding this comment

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

Assuming you implement a separate unquoting command, @Sean10 can't the user just solve your case by hitting the unquoting command then hitting the quoting command? Same result.

I agree with @yzhang-gh there are many more instances when it's preferable to nest blockquotes.

src/formatting.ts Outdated Show resolved Hide resolved
Comment on lines 79 to 81
if (line.startsWith("> ")) {
return line.slice(2);
} else {
Copy link
Owner

Choose a reason for hiding this comment

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

line.startsWith('>')?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you're right. I thought adding a space is more beautiful and standard. But actually no such setting in github markdown.I will modify this.

Copy link
Owner

Choose a reason for hiding this comment

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

Well, I think we can definitely advocate adding a space after > (actually I like it). I meant we still need to handle the case if there isn't a space.

Copy link
Author

Choose a reason for hiding this comment

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

Ok~I will modify the space back and skip the line which it hasn't appended space.

src/formatting.ts Outdated Show resolved Hide resolved
src/formatting.ts Outdated Show resolved Hide resolved
@Lemmingh Lemmingh changed the title add quote/unquote line/multiline support ✨ Add "toggle block quote" Sep 12, 2020
… code, use eol to replace \n to fix different eol problem, fixredundant quote syntax
@Lemmingh
Copy link
Collaborator

FYI, VS Code's _analyzeLines() and _executeLineComments():

https://github.com/microsoft/vscode/blob/03adc22ea6ff4c9dd88ec6e4bbd11b1097f0f726/src/vs/editor/contrib/comment/lineCommentCommand.ts


The Spec is very permissive about spaces, which honestly, is often a headache.

And remember leading spaces may have semantic meaning, for example, indentation.

Given that indentation in Markdown is flexible, we cannot use tabSize configuration, but usually have to look behind and ahead to determine it instead.

My proposal:

When a user run this command, the person implies all the lines support it, so we only need to detect indentation.

  1. If there are 4 or more leading spaces in the lines, check if we're in an indented code block.
  2. If there are 3 or more leading spaces in the lines, check if we're in a list.
  3. If there are 2 or more leading spaces in the lines, check if we're in an unordered list.
  4. Otherwise, we're at the root level.

@Lemmingh
Copy link
Collaborator

Lemmingh commented Sep 13, 2020

My previous comment looks pretty confusing.

The actual procedure to determine indentation size I is more likely to be:

  1. Let I = 0.
  2. Let S be the length of leading spaces.
  3. If S >= 2, then:
    1. Look behind. If we're in a list, then:
      1. Let I be ("the length of list marker" + "the length of the marker's leading spaces" + 1).
  4. Return I.

@yzhang-gh
What about extracting some functions from listEditing.ts as utilities?

@yzhang-gh
Copy link
Owner

yzhang-gh commented Sep 13, 2020

Things become more complex now...

I don't want to add too much code only to deal with the corner cases for such a tiny feature. The simplest way is probably to stick with what VSCode does for comment/uncomment (not 100% accurate as VSCode seems to ignore blank lines?)

  1. If not all lines start with > (or are blank lines), then add > to all lines;
  2. else if all lines start with > , remove all leading > ;
  3. else remove leading > of all lines. (As for the possible leading whitespace... I don't care...)

The difference (from the current implementation) is each line is not transformed independently.

@Lemmingh What do you think of (case 1) adding > to 1) all lines or 2) only lines that don't have > ? (IMO, it isn't worth a configuration... Let's simply do a poll and make a decision here.)


Something interesting

Input

> - one
>   - two

Output

  • one
    • two

Input

>- one
>  - two

Output

  • one
  • two

@Lemmingh
Copy link
Collaborator

As far as I can tell, there are 2 major differences between VS Code's "Toggle Line Comment" and our "Toggle block quote":

  • When a user runs the command:
    • VS Code first checks whether it is supported.
    • In our case, the user is determined and implies all the lines support it, so we don't need to check.
  • To calculate insertion points when adding markers:
    • VS Code uses _normalizeInsertionPoint() which depends on tabSize.
    • We cannot due to the flexibility of Markdown, and I raised the proposal. (The first time I thought, I found a great number of cases. Later I realized that only lists count. 😂)

I think "To calculate insertion points" (or "indentation size" or "indentation point") is the challenge now:

  • When the lines really contain a block quote ("Each prefixed by a block quote marker (ignoring leading spaces)" && S <= I + 3), we can remove markers. (Otherwise, the user intends to add markers to create one.)
  • When adding markers, we have to know where to insert them.

VSCode seems to ignore blank lines

I think so.

I believe our command should act on all the lines to produce only one block quote.


If not all lines start with ...

In step 1, I prefer not to op on leading and trailing blank lines. I mean we should exclude leading and trailing blank lines at analyzing stage, since user may select a lot of blank lines around the intended lines.

In step 2, we should match block quote marker by /> ?/. After ignoring leading spaces, the operation should equal to .replace(/^> ?/, '')

Step 3 may break the document and sounds unnecessary. I prefer to drop it.


What do you think of (case 1)

To be robust, I'm stubborn to always add > (U+003E U+0020) to all the lines.


Your interesting case exactly reflects the definition of block quote, and is one of the reasons that I said

The Spec is very permissive about spaces

@Lemmingh Lemmingh marked this pull request as draft September 13, 2020 14:39
@yzhang-gh
Copy link
Owner

  • When adding markers, we have to know where to insert them.

Alright, I didn't really think of this case. Now I know what you mean, for example

- one                   -->      - one
  nested quote 😱                  > nested quote 😱
  • one

    nested quote 😱


@Sean10 As you see, there are quite a few potential issues with the current implementation. I would suggest making a standalone extension for this feature. And if we find there are many users wanting this, we can then make an effort to resolve this properly.

@Sean10
Copy link
Author

Sean10 commented Sep 14, 2020

Yes, I understand this. I will temporarily make a standalone extension for my specific scene.

@Lemmingh Lemmingh added the Help wanted Looking for help. label Dec 13, 2020
@GMNGeoffrey
Copy link

GMNGeoffrey commented Jan 22, 2021

@Sean10 did you end up making this? I came here looking for this feature. I may install https://marketplace.visualstudio.com/items?itemName=jebbs.markdown-extended as well, but not sure about extension interaction

@Sean10
Copy link
Author

Sean10 commented Jan 22, 2021

@Sean10 did you end up making this? I came here looking for this feature. I may install https://marketplace.visualstudio.com/items?itemName=jebbs.markdown-extended as well, but not sure about extension interaction

vscode-markdown-quote - Visual Studio Marketplace this one.

@Lemmingh Lemmingh added Area: Block quote https://spec.commonmark.org/0.30/#block-quotes Area: Input Related to editor input processing (key presses, key bindings). labels Aug 26, 2021
@gmccullo
Copy link

+1. I want this!

@huyz
Copy link

huyz commented Mar 31, 2024

I think all this is too complicated because of the misguided attempt to implement a "toggle".

Not only does this bring up lots of undefined cases, but it's actually not useful IMO. What would be preferable is blockquote increase and decrease. In Markdown, there are many use cases for nested blockquotes. I use them all the time in Obsidian.

For this blockquote functionality, I've been using this Obsidian plugin https://github.com/czottmann/obsidian-blockquote-levels for years and it works beautifully.

Two separate commands are simpler to design and implement and actually give users a way to handle all cases, without ambiguity.

@Sean10
Copy link
Author

Sean10 commented Apr 14, 2024

I think all this is too complicated because of the misguided attempt to implement a "toggle".

Not only does this bring up lots of undefined cases, but it's actually not useful IMO. What would be preferable is blockquote increase and decrease. In Markdown, there are many use cases for nested blockquotes. I use them all the time in Obsidian.

For this blockquote functionality, I've been using this Obsidian plugin https://github.com/czottmann/obsidian-blockquote-levels for years and it works beautifully.

Two separate commands are simpler to design and implement and actually give users a way to handle all cases, without ambiguity.

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Block quote https://spec.commonmark.org/0.30/#block-quotes Area: Input Related to editor input processing (key presses, key bindings). Help wanted Looking for help.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants