Deleted unnecessary code and extended checks for newlines to single item lists. Also added a check if an unordered list is extended.

[CountOrlok]

The first commit deletes empty arrays created just to feed the garbage collection:

var list = [];
list = selected.text.split('\n');

The method split will always return an array, so the variable list will be assigned an array anyway.

The second commit includes checks for newlines for the case of a list containing just one element. Until now, there are no newlines added, which can prevent a list to be rendered.

text
listitem -> mark and push button to add list syntax

Since selections containing newlines and selections without newline are treated within the same conditional branch, we only need a single call to getLeadingNewlines to cover both. I also deleted the variable start, because a variable prefix is declared at the top of each function creating list syntax.

The third commit includes a check whether an unordered list is extending another list.

- listitem
- listitem
listitem
listitem

Consider the list above. If you now mark the last two items and push the button to add bullets, no newlines will be added, so the existing list will just be extended.

I did some testing and it seems to behave as intended, but I recommend to do at least some quick tests by yourself before merging.

[CountOrlok]

Maybe the commit to add newlines for lists containing only one item came a bit too early, if we assume that it is more common to add a single item to an existing list than to start a new list with a single item.

Since I added a check for a preceeding list in the last commit, at least in case of unordered lists this should be no problem. But checking for a preceeding ordered list is a bit more tricky, especially if we want to get the numbers right.

[CountOrlok]

Urgent: Please do not merge!

I used the already existing function __previousLineIsList() to determine whether a selection represents an extension to another list, but this function is seriously broken.

I mean, take a look at the source:

__previousLineIsList: function(content, sel, rx) {
  var i, c;

  for(i = sel.start - 1; i >= 0; --i) {

    c = content.substr(i, 1);
    if(c == "\n") {
      if(content.substr(i + 1, 1).match(rx)) {
        return true;
      }
    }
  }

  return (i === 0);
}

WTF!?

__someLineBeforeExceptTheFirstLineIsList() would've been a better name, wouldn't it? ;-)

I'm going to write another patch tomorrow...

[ckruse]

Indon't think that you are right. It moves backwards through the string and checks if the current character is a newline. If it is it looks if the character following the newline matches the regex. Looks pretty solid to me. The i - 1 is necessary to avoid the check for the current line, defined by the selection.

[CountOrlok]

I don't think that you are right. It moves backwards through the string and checks if the current character is a newline. If it is it looks if the character following the newline matches the regex.

...and if the character doesn't match the regex? :-)

Then let's try again in the previous line, and so on, until we're either out of characters or we've finally got a match. This will not just match a list in the previous line, but in any line preceeding the selection except the first one, because there is no leading newline (which of cause is also the case if the first line actually is the previous line).

It's also interesting to see under which circumstances (i === 0) should resolve to true. :-)

Looks pretty solid to me.

It's already late. ;-)

[ckruse]

That's because a list item may contain newlines. You have to look at more than one line. I can't exactly tell what this function should do since I miss context (e.g. the regex) and I only had a cursory glance at it, but there's definitely more to be aware of.

[CountOrlok]

That's because a list item may contain newlines.

That's true. The point is, that the function contains no logic to determine whether a line that does not match the regex (dash or digit) is plain text or part of a list item. So it will return true even if the selection is not preceeded by another list, but by chunks of plain text, which in turn are preceeded by another list. If we have a posting with more than one list, the function won't produce the correct results.

I can't exactly tell what this function should do

Well, the name __previousLineIsList() is pretty self-explanatory, i'd say. The function is used to check, if a newline should be included or not. So, when I've read the function name and saw how it's used, I was convinced that it can tell whether the previous line is (part of) a list or not. ;-)

I think the real problem is, that I didn't look at the function definition beforehand. I just expected the function to reliably check, if the previous line is a list, as the name of the function implied. I should've been more careful, not blindly taking things for granted.

In fact, the function is not completely broken, so my reaction might seem a bit harsh. In many or even most cases, the function will return the right result. But as I mentioned above, there are very common cases, where the result will be wrong. That's why I voted to not merge the latest commits.

[ckruse]

No, the function is broken. I now had the time to check, it's missing several exit conditions, you are right.

[CountOrlok]

I'll try to refactor, but before I start I need at least two more cups of coffee.