SidOfc / mkdx

A vim plugin that adds some nice extra's for working with markdown documents
https://www.vim.org/scripts/script.php?script_id=5620
MIT License
482 stars 13 forks source link

Toggle bold, italic, and strike styling #101

Closed victorkristof closed 4 years ago

victorkristof commented 4 years ago

Hi!

I really like to be able to toggle quotes and list types.

Would it make sense to also enable <Plug>(mkdx-text-bold-n) (and its italic and strike counterpart) to toggle between **text** and text?

Currently, b performed on **text** gives ****text****, which isn't really useful.

SidOfc commented 4 years ago

Ahh yeah, I figured this one would pop up someday :)

The thing here is that figuring out the boundaries is sometimes quite tough to do, depending on wether or not a certain applied style (bold / italic) spans multiple lines.

That said one can always u to undo a broken move. And you may be the first to want this (and open an issue about it) but I'm sure you're not alone (I've just lived with removing the marks myself manually too hehe).

I think it's worth a shot, cheers for the suggestion :+1: though as it may be more difficult than expected it may take some time for me to implement, with that I thank you for your patience in advance :)

samarulmeu commented 4 years ago

Will it be possible to toggle between selected text maybe? Just a thought...

SidOfc commented 4 years ago

@samarulmeu I think it would be more effort to first have to select the text and then unmark it, as opposed to just unmarking stuff around the cursor directly.

This is only the case for unmarking though, since it does make sense to start a selection and later figure out where you want it to end, and apply stuff around it after you've selected what you want.

In any case I think basic toggling of say bold/italic/bold-italic using normal mode should be possible and will allow me to stay in the realm of sanity while at it :)

As usual thanks for joining in on the convo, please keep sharing your thoughts if you have them, always appreciated :+1:

samarulmeu commented 4 years ago

@SidOfc Thank you for your kind words. I really like the plugin and it replaced some other markdown plugins for me, that is way I am following all the issues trying to help in making it better.

My latest suggestion was based on what I considered to be easier. I think is easy to select text betweend bold or italic mark amd then just toggle them. Having the plugin do the switch in normal mode will ne great, but as I already noted it will be a lot of work.

Anyhow, you have the knife and the knowledge to do it.

victorkristof commented 4 years ago

Thanks @SidOfc and @samarulmeu for the comments! I believe the behavior of this feature could be rather simple (using bold as an example).

  1. Case single word: \b inserts ** around the word under cursor if the word doesn't already have **, and it removes ** if the word is already surrounded by **.
  2. Case multiple words: \b removes ** if the cursor is in a "bold" zone (determined by syntax highlighting I guess?). This is a "destructive" action as it is unclear where to put the ** marks for multiple words. Bolding multiple words would still be performed using visual selection.

What do you think? Does that make sense? Am I forgetting some edge cases or missing anything?

SidOfc commented 4 years ago

@victorkristof indeed that is an amazing suggestion, I never thought about using the syntax highlighting group to detect the boundaries of bold marks! In my mind I was already traversing the file left / right and up / down in order to find boundaries, but this seems like it could make life a whole lot easier and less bug riddled, I will give that a shot hopefully this week as well!

SidOfc commented 4 years ago

Did some more research, while I can actually identify the highlighting group below the cursor, the start / end positions of the highlighting group the cursor is on are not in the results. I've checked :h function-list and it doesn't have any functions for getting them either.

Had some high hopes for that information being given to me by Vim/Nvim but it seems like traversing current line and possibly up/down as well will be the only way to achieve what we want here.

I'll see what I can come up with, but it may take a bit longer because of this :sweat_smile:

SidOfc commented 4 years ago

I've got a crude working implementation of it now, but because of the way vim applies highlighting (it's inconsistent, ugh) there are some edge cases that need to be taken care of...

basically, given: **hello world**, the text **hello world has the highlighting group markdownBold but the last two ** at the end only have markdownBoldDelimiter :facepalm:

Aside from that I'm a bit worried about single-character wrapped texts such as **a**, but I can only really start looking at that once I've patched the inconsistency.

For today though, I think I'll leave it at what it is :)

SidOfc commented 4 years ago

Alright, actually pushed through a bit and got it working, this one is also in master now, you can update mkdx normally. Let me know if anything breaks for you, otherwise I think we can also close this one :)

The only thing it doesn't do is handle bold+italic mappings since these are essentially "composite" of a bold+italic marker and the syntax highlighting mkdx applies doesn't respect them as individual parts but rather as a whole, perhaps changing the way its highlighted will fix it though, but I think this can wait until it pops up as a feature request :)

victorkristof commented 4 years ago

That's really great! I have some errors unfortunately :/

  1. When I press \b to bold the word under my cursor, I get this error:

    Error detected while processing function mkdx#WrapText[6]..44:
    line    4:
    E46: Cannot change read-only variable "count"

    and then **function(data_set,** replaces the word I want to bold.

  2. When trying to toggle some bold text, I get the same error as above (without the function(data_set, thingy). But the text gets unbolded after the error is displayed.

  3. Same behavior as 2 when I add a count to bold some text.

SidOfc commented 4 years ago

@victorkristof hmmn that is quite interesting, can you provide a case to reproduce this issue? It could be because I am safeguarding get(v:, 'count1', 1) in my code...

Just opened vim and indeed marking text there doesn't work at the momoent. in Neovim this does work however... I'm on it!

SidOfc commented 4 years ago

hmmn, regular vim (at least mine) refuses to load mkdx all-together now... interesting indeed

SidOfc commented 4 years ago

@victorkristof I reset master to an earlier point in time, does it restore "old" functionality for you?

SidOfc commented 4 years ago

@victorkristof I've reset master back to before any feature merges (so even further than my last message), seems regular vim now works again but holy freaking hell...

I usually develop in neovim and usually, neovim and vim aren't too different. In this case however, I have no idea what causes this issue at all... Unfortunately the error messages are harder to decipher than chinese. It is complaining about not being able to modify count, which I am never modifying anywhere.

The artefact you are seeing may be an internal vim function since that code does not exist within mkdx source code.

edit: back to the drawing board to make vim also play nice

SidOfc commented 4 years ago

@victorkristof figured it out...

It seems that, if you use let count = in regular vim, it trips because it thinks I'm trying to modify an internal variable v:count:

image

I fixed it and added a note in the code to clarify:

image

I don't like bashing on vim, but that was indeed rediculous :sweat: Anyways, can you retry @victorkristof?

edit: upon further investigation it makes more "sense", apparently using let without l: overwrites internal variables by default (:h let) it's literally just the first time that I used some variable that already exists in vim, keeping that in mind for next time too.

victorkristof commented 4 years ago

Well done! It works smoothly :)

I found another (smaller) bug though...with a potential solution.

If I toggle bold on a snake-cased word, I get two different results:

This is a snake-cased word.    >>>    This is a **snake-cased** word.
          ^

and

This is a snake-cased word.    >>>    This is a snake**-cased** word.
                ^

The second instance is broken I guess.

I think I suggested in #103 to wrap around WORD rather than word. But I realize that since we can now give a count to the mapping, then it is easy to wrap snake-cased words (with 2\b and the cursor in the first part of the word). And it won't fiddle with the punctuation (like you correctly noticed).

So my suggestion would to revert the behavior of bold/italics/... to wrap around words and make it explicit in the documentation.

Unless you can come up with a resilient behavior for these cases? What do you think? :)

SidOfc commented 4 years ago

Ahh nice catch @victorkristof this should be easy to fix actually, I use regular motions to move cursor around so all I had to do is replace w with W to move WORDS, but I also move the cursor back using a likewise motion b, changing this to a B will likely fix it, will check it out right now, stay tuned :P

SidOfc commented 4 years ago

Should be working now actually :)

victorkristof commented 4 years ago

Hmm nope, same problem... No sorry, buffer didn't reload properly.

SidOfc commented 4 years ago

Ahh cool cool!

Does this mean we can close this one? :)

victorkristof commented 4 years ago

Ok one last thing. You were right about punctuation. If there is punctuation right after the snake-cased word, then the punctuation will be included:

This is a composed-word.    >>>    This is a **composed-word.**
          ^ 

Any chance to exclude the last character of the WORD if it is not part of a word or something? :)

SidOfc commented 4 years ago

Yeah was just typing this one out in a message actually, I haven't found a way to "change" the definition of a WORD (:h WORD) but I may be missing something, a WORD is basically any non-whitespace and sentences could also end in comma's, semi-colons etc so this is more difficult than it looks.

I think it'll be easier to move the punctuation out manually in this edge case, because otherwise I'd have to reimplement how wrapping works entirely, is that something you can live with?

SidOfc commented 4 years ago

Alternatively, I could provide a setting which would allow wrapping to occur per WORD or word, since as you mentioned before, wrap mappings may now be prefixed with count, which could solve it that way.

victorkristof commented 4 years ago

Alright I see, I don't have a strong opinion right now, I guess I'll have to use the plugin for a few days in real situation to make my mind. If this turns out to be a problem, I can open a bug issue later so that you can close this one :)

Anyways, thank you for the reactivity and implementing the feature so quickly!

[Edit] Just saw your second comment, yes that could be a potential workaround. But don't want to overload your plugin with rather "small" options though!

SidOfc commented 4 years ago

Alright, that sounds good to me!

As for the option, I believe mkdx already has 40+ settings so another one won't do harm, perhaps the default could be changed back to "word" wrapping etcetera but this is something we can discuss later.

Give it a test drive and let me know what you think if indeed it bothers you, your input has been valuable and useful for other users (though we won't hear about it from them easily hehe).

Thank you for the compliments, I do my best to keep everyone happy while retaining the quality of the plugin, it is getting a bit hairy lately though but still doable :)

With that I'll close this one for now and I may hear from you again quite soon, thanks for all the feedback so far (now onto #100) :+1:

SidOfc commented 4 years ago

just in case you're wondering about the . scenario, I reverted back to using word as you suggested. For me, word actually includes - characters as well so using WORD wasnt needed.

I also found a setting which I can use with word (not WORD though) which is called iskeyword. Testing in both vim and neovim yielded the same results so I think it should support the . scenario properly now in most cases.

One edge case I found is while the cursor is on the punctuation it will not do the right thing currently, not yet sure how to figure that one out either but figured I'd keep you up-to-date here :)

victorkristof commented 4 years ago

Ok, interesting. Actually setting iskeyword is the best option. I now have this in my vimrc:

augroup markdown
    autocmd!
    " Include dash in 'words', which makes more sense for prose.
    autocmd FileType markdown setlocal iskeyword+=-
augroup END

This works majestically! Might be worth mentioning in the doc :)

(I don't why you have it by default. Did you set it explicitly as well? Do you have set lisp?)

Anyway, problem solved - thanks!

SidOfc commented 4 years ago

Actually don't have either lisp or iskeyword modifications at all so it must be something else, or perhaps it depends on version / platform (would be really odd though).

I can add a mention in the README, glad this worked for you as well!

samarulmeu commented 4 years ago

A bug I discovered today, when writing: if the word is the last one, without any space after it and you try to toggle it will also take the space before

Example:

Some random words

Trying to bold words I get

Some random**words**

Instead of

Some random **words**

Italic shows the same behavior.

samarulmeu commented 4 years ago

Please see my edited last comment. I accidentally added a dot at the end of the example.

SidOfc commented 4 years ago

Alright, reopening, may take a few more days since I already planned a bit ahead this week, cheers @samarulmeu :+1:

samarulmeu commented 4 years ago

No problem. Take your time.

SidOfc commented 4 years ago

After some quick checking, it is an issue in vim only, neovim handles it properly (this is just a note to self)

SidOfc commented 4 years ago

Nevermind above comment, I performed a slightly different check in neovim where it did work (having cursor on last char of last word) both are equally broken.

SidOfc commented 4 years ago

Alright, should now also work properly @samarulmeu although I did find a new issue whereby if you try to quote an actual punctuation mark:

hello world.                            >>>                           hello .**world**
           ^ (cursor is on punct)

I doubt anyone really wants to do this though however, if it does pop up, let's not reopen this anymore since this one is getting quite long now :)

samarulmeu commented 4 years ago

Thank you again. You've made my day! I think that the issue with the dot mark is strange and will not be encountered.

How I've stumble upon the issue I've mentioned: I was writing something and then I thought I should emphasize this so Esc (cursor stays on word I was writing) and then <leader>+/ and voila! I can continue writing. And this way also I don't have to insert manually the emphasize or strong marks -- I just write, stop and toggle the last word.

Great job!

You can close this as I am not able to.

SidOfc commented 4 years ago

Cheers for detailing this, there's probably some more edge cases lurking in the dark with text wrapping (especially multi-line which I did end up writing some traversal logic for).

In any case will close this one permanently now, new issues are welcome as usual samarulmeu :+1:

Note: I honestly thought this one would be more difficult, but I keep finding new ways to fix these things lately :sweat_smile: