Desktop: Fixes #4877: Incorrect list renumbering

[austindoupnik]

These changes are related to/have a minor conflict with PR #4832. Please let me know how I can help get that PR merged, then I can rebase and add some additional test cases. I would also be happy to port the testing changes over to my branch and add test cases.

Here is what I found through manual testing (the expected behavior is the behavior after this patch):

Note: Case C and Case D were working and continue to work, just wanted to test all the code paths.

Case A

Steps to Reproduce

1. Create new note
2. Enter text

   1. item1
   2. item2
   3. item3
   

3. Drag select all 3 lines
4. Press numbered list

Actual

4. 1. item1
5. 2. item2
6. 3. item3

Expected

item1
item2
item3

Case B

Steps to Reproduce

1. Create new note
2. Enter text

   1. item1
   2. item2
   3. item3
   

3. Ctrl+A
4. Press numbered list

Actual

2. 1. item1
item2
3. 3. item3

Expected

item1
item2
item3

Case C

Steps to Reproduce

1. Create new note
2. Enter text

3. Press numbered list

Actual

1. List item

Expected

1. List item

Case D

Steps to Reproduce

1. Create new note
2. Enter text

   foobar
   

3. Place cursor at the end of the line
4. Press numbered list

Actual

foobar
1. 

Expected

foobar
1. 

Case E

Steps to Reproduce

1. Create new note
2. Enter text

   1. item1
   1. item2
   1. item3
   

3. Drag select all lines or Ctrl+A
4. Press numbered list

Actual

2. 1. item1
3. 1. item2
4. 1. item3

Expected

1. item1
1. item2
1. item3

[laurent22]

The pull request should indeed be based on #4832 once it has been merged. That other pull request also adds test units for this part of the code, so you'll be able to add yours too for the manual tests you mentioned above.

[austindoupnik]

@laurent22 sounds good, I will rebase and wait for that PR to be merged. It seems it is close. Thank you!

[laurent22]

For information the other pull request has been merged now.

[austindoupnik]

@laurent22 thanks! I updated the PR and ran into one more test case where the behavior is unclear to me what is desired. I also think fixing it requires touching more code than what is covered by modifyListLines. The case is somewhat captured in this test case.

The following

1. item1
item2
2. item3

becomes

item1
2. item2
item3

or

item1
3. item2
item3

Depending on if it is selected with Ctrl+A or click and drag. Testing it quickly with the GH MD editor it seems that GH would do

1. 1. item1
2. item2
3. 2. item3

Let me know how I should handle this case, and if it should be this PR or another.

[laurent22]

Your tests aren't passing at the moment: https://travis-ci.org/github/laurent22/joplin/jobs/769582849#L912

[austindoupnik]

@laurent22 sorry about that! Haven't quite worked out the workflow yet, forgot to recompile typescript before running the tests.

Updated the test cases. I also removed the JSON.stringify I wasn't sure the purpose of that and it made the diffs on assertion failure harder to read as it was diffing JSON instead of the objects directly. I can add it back if it has some purpose. Thanks!

[adarsh-sgh]

I also removed the JSON.stringify I wasn't sure the purpose of

That was just for checking deep equality; using toStrictEqual as you did is better imo.

[austindoupnik]

@lkiThakur ah thanks for the clarification! I thought it might be something like that

[laurent22]

Thanks for the update, I think it's nearly there. Also for some reason I see that there was no tests added to handle lists with gaps in it. Could you add one please, just to make sure we're preserving this behaviour?

So this:

one
two

three

should be come this:

1. one
2. two
3.
4. three

[laurent22]

I don't think this is the best behaviour because if the user selects a block, they want either all the numbers to be gone, or add numbers to each lines, but I don't think they want a mix of both. Perhaps you could check the first item - if it's a number, remove all numbers, and if there's no number, add a number to each line.

[austindoupnik]

Sure that makes sense. Should it be just the first line or any line?

[laurent22]

First line I guess?

[adarsh-sgh]

Also for some reason I see that there was no tests added to handle lists with gaps in it.

I wanted to minimise the efforts if switching to other discussed behaviours in future, however if we are preserving this behaviour then someone could add these lines to the useCursorUtils.test.ts file(in the callback of describe)

const numberedListWithEmptyLines=[
	
	'1. item1',
	'2. item2',
        '3. '     ,
	'4. item3',
];
const noPrefixListWithEmptyLines = [
'item1',
'item2',
''     ,
'item3',
];

	test('should add numbers to each line including empty one', () => {
		expect(modifyListLines(noPrefixListWithEmptyLines,1,'1. ')).toStrictEqual(numberedListWithEmptyLines)
	})

[laurent22]

@austindoupnik, are you still ok with completing this pull request? There are a few tweaks to make, but I think it's nearly there.

[austindoupnik]

@laurent22 yep! I was out for a few days, but will work on the changes now. thanks!

[austindoupnik]

@laurent22 FYI

[laurent22]

Ok looks good I think, so let's merge

[austindoupnik]

@laurent22 thank you!