#371 quickfix

[aaalsaleh]

This is a quickfix for #371. The replacement of markers using regex is generally not good, and it was there before the move to the new undo manager (unlike what is mentioned in the issue). I've only fixed it by adjusting the regex to only remove the markers ems.

[rf-]

Won't this fail if the user has selected any HTML tags when the marker is placed? Also won't it eat any content in the current selection (at least for purposes of tracking whether that content has changed)?

[aaalsaleh]

Placing markers by Scribe is only a temporary state to save the location of the selection when pushing a history item or doing a command. It will always be restored into a real selection before returning to the user. The user will never be able to select or edit while there are markers. Also, Scribe is using two markers to save the start and the end of the selection. It uses two empty em at both ends, rather than one em surrounding the selection range. Can you list the steps to reproduce the issues you are raising?

[rf-]

Thanks for explaining. Sorry if it sounded like I was saying there are bugs here—I haven't tried this fix and was curious about how it works. I didn't realize Scribe uses empty em tags instead of wrapping the selection in a single em tag. I still don't understand why the [^<]*? part of the regex is necessary though.

[aaalsaleh]

Nothing to be sorry about, as I had the same impression in the past. As for the regex, well, I could've just dropped it, or just used \s*?. However, I am worried about any weird cases in the different browsers across the different platforms when adding an empty element. They might use spaces, newlines (\n or \r or both), maybe  , or any other wild case. Therefore, [^<]*? is used to match anything before < in a non-greedy way (to avoid eating further content). Of course, Scribe never places anything inside.

[rrees]

I'd like to see some tests around this because its getting hard to understand what the right behaviour is and what bugs we're fixing here.

[aaalsaleh]

@rrees I eventually decided to place it under the bold command tests. It can be tied to the selection and the undo manager as well. The bug is caused when removing closing em for the markers using regexp, which will also strip closing em for texts surrounded by em. I genuinely don't know how this issue is caused, as the removed content is only used for comparison, and never stored. Nonetheless, the adjusted regexp only removes the markers closing em. This new regexp passes this test, while the current regexp fails.

[rrees]

@aaalsaleh Great, thanks for adding the test, I'll look at merging it tomorrow UK time.

It's not obvious to me either how this fixes the issue but I'm happier to have a test that demonstrates the bug.