Page MenuHomePhabricator

Investigation: scope bracket matching and tag matching add-ons
Closed, ResolvedPublic3 Estimated Story PointsSpike

Description

Use the test instance to check the scope and difficulty of installing the code mirror addons for bracket and tag matching, without any alteration. What would this look like in the two editors? Does it cause any unforeseen problems?

See the initial investigation for more detail: T254976: Investigation: Bracket matching .

Status

We can already outline some of the potential work for these features:

Bracket matching

  • Easy, done: Load addon scripts.
  • Easy, have a POC: Change highlighting styles to fit our existing color scheme. Matched and unmatched brackets should have normal text color. Matched bracket character should have background color Base70 #c8ccd1, unmatched brackets should have a red background. Needs some cleanup, an upstream style is commented out when it should be overridden.
  • Easy: port our customizations to CodeMirror 6.x
  • Medium: Fix bracket-matching logic to consistently detect all types of brackets, in each configuration. Currently (, {, [ each highlight but not in every situation and it's not yet clear what is causing this. Possibly add < as well. Must include tests.
  • Medium: VisualEditor "new wikitext" integration breaks the cursorActivity event.
  • Medium, optional: Fix bracket-matching to detect and highlight "{{" and "[[" as pairs.

Tag matching

  • Easy, done: Recognize all MediaWiki parser tags and give the same highlighting as basic HTML tags.
  • Medium: Rely on above cursorActivity fix to integrate with VisualEditor.

Event Timeline

ECohen_WMDE removed the point value for this task.
ECohen_WMDE removed a project: Spike.

Change 618536 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/CodeMirror@master] [POC] Enable the matchbrackets add-on

https://gerrit.wikimedia.org/r/618536

Addons can be loaded using the $wgCodeMirrorPluginModules configuration, which pulls in additional ResourceLoader modules at the right moment to extend CodeMirror. I've created a proof-of-concept that loads the matchbrackets.js addon, and have deployed to the test instance.

Surprisingly, brace matching is already working! Problems experienced on my local were mysteriously not present on the test instance.

The brace-matching regex is configurable, and built-in examples demonstrate that two-character delimiters are supported.

The tag-matching add-on is currently broken, I think it's mostly unused and calls a long-deprecated plugin API. Probably not a big effort to update this add-on, it's a short file and the changes are straightforward.

... built-in examples demonstrate that two-character delimiters are supported.

I was wrong about this, built-in examples are all one-character.

The bracket matching code would have to be rewritten for multiple-character delimiters. Highlighting just one character at a time might be good enough, though.

Bracket matching doesn't show up in VE, I've tried a few combinations of config and modules so far.

Very exciting! A few notes after trying it out:

  1. Seems like it's only working in the 2010 wikitext editor and not 2017 VE wikitext editor. Have you looked at why this might be yet?
  2. It seems to find some pairs and not others. I wasn't able to find a clear pattern either, sometimes it could find pairs that were far away from each other and sometimes it couldn't find a pair closeby, on the same line.
  3. Is it easy to change the styling? I think that the green color change is a bit too subtle. Is it possible to instead style it so that the color remains the same but the character is given a background color of Base70 #c8ccd1.

I think if the first two seem to be too large of problems or issues to fix, then that hints towards this integration only after v6. I think even if it can only be implemented in 2010 then that could be worth it, but not if it's having the issues from number 2.

Oops was writing and didn't refresh/ see your most recent comment.

Glad to have the extra confirmation on highlighting bracket pairs and triples. I think we can definitely conclude then that is out of scope, especially before the switch.

I think we can definitely conclude then that is out of scope, especially before the switch.

We should discuss as a team, maybe. Doing work twice isn't ideal of course, but after seeing these add-on modules and what a small and simple codebase they present, it isn't that scary to (potentially!) port our work to the newer version. There's a danger of letting the perfect be the enemy of the good... The logic we would be implementing is almost certainly reusable, and we would simply splice the changes into updated addons, it might not even be necessary to figure out any of the new glue ourselves.

Good points! Yes, let's definitely talk about it then - didn't mean to cut off discussion. Just feels difficult to balance the potential work here, which might have to be repeated vs the work in the other two areas. And if we can get the rest of bracket matching working, getting the doubles/triples feels less essential (though of course nice to have). But I guess that's why we're collecting as much information as possible so when we have that balancing conversation it can be more informed.

Something else we should keep in mind, is how to deal with highlighting groups of brackets when there is a missing bracket. I'm not sure we could do it without including something like the example below. I think it could be very helpful for catching errors but also increase the amount of work. Just wanted to document here for the future.

Screen Shot 2020-08-06 at 09.47.08.png (81×304 px, 9 KB)

Something else we should keep in mind, is how to deal with highlighting groups of brackets when there is a missing bracket. I'm not sure we could do it without including something like the example below. I think it could be very helpful for catching errors but also increase the amount of work. Just wanted to document here for the future.

Screen Shot 2020-08-06 at 09.47.08.png (81×304 px, 9 KB)

The add-on already highlights unmatched brackets in red, but it's not terribly useful. In your example, it would highlight the first left-brace in red, if the cursor were next to it. In any other cursor position, the mismatch wouldn't appear. For example:

image.png (18×69 px, 959 B)

Ah I noticed the red highlight and actually didn't realize what it meant. Makes sense now, thanks for pointing it out! Maybe just changing the styling will be clear enough then, without needing to change the behavior. Similar to my other comment, I would have it stay the same color but have a red background highlight instead.

As far as I know, you cannot reliably determine the intended edges of {{ and {{{ and { in wikicode. If you want "{hi", then {{{templatewhichsayshi}} is perfectly valid. It's one of the reasons why making a syntax model for wikicode is so hard.

P.S. I mentioned this, because i think for that reason we should be careful to indicate to a user that something is 'wrong', when there is no match. Bracket matching, and being able to see which bracket 'terminates' which other bracket is good. But indicating if a bracket is 'missing' is impossible to do reliably, and thus telling a user that it is wrong, when potentially it is something that the user (or the PREVIOUS editor) wanted, can be 'dangerous'.

So I advocate for insight over judgement.

I would have it stay the same color but have a red background highlight instead.

+1

I'll make notes about remaining work and potential scope, in the task description.

I would have it stay the same color but have a red background highlight instead.

I've added these styles on the test instance.

Nice to have: debounce and timeout. Many sequential requests are simplified to a maximum number of matches per time period. Regexes are cancelled by a timeout of e.g. 0.5s.

The tag-matching add-on is currently broken, I think it's mostly unused and calls a long-deprecated plugin API.

I was totally wrong about this, the error I saw is fixed by including another dependency, addon/fold/xml-fold.js. However, the tag matching still isn't working, something inside of the logic is subtly failing to pick up our xml tags. The demo works fine, and I've ported over the only patch committed upstream since our version (5.35.0) was released.

For reference, here are commands to pull in additional distro files:

git checkout 5.35.0
EXT_ADDON_DIR=~/ext-CodeMirror/resources/lib/codemirror
EDIT_ADDONS=$EXT_ADDON_DIR/addon/edit
mkdir -p $EDIT_ADDONS
CodeMirror/addon$ cp -r edit/matchbrackets.js $EDIT_ADDONS
CodeMirror/addon$ cp -r edit/matchtags.js $EDIT_ADDONS

Thanks for updating the task description! Looks like a great summary and will be very helpful for discussion. Only note is re: the button to toggle on and off. This wasn't the intention, though if you think there is a strong reason for doing I'd be curious to discuss. The idea was that it would just be turned on and off with the same syntax highlighting button. This was because it shouldn't have negative impacts/change existing workflows in negative ways and the editor interface is already quite overloaded.

Also took a quick video of the current behavior and I think it's already great! And it already convinces me that highlighting pairs and triples is not really necessary for bracket matching to provide clarity. Also given the comment from theDJ about the complexity of determining edges, we should discuss the work vs payoff for this feature. Main thing that's clear is that given the number of colors we have, I'll need to do some testing to find the right shade of red for the background which keeps everything readable.

Only note is re: the button to toggle on and off. This wasn't the intention, though if you think there is a strong reason for doing I'd be curious to discuss. The idea was that it would just be turned on and off with the same syntax highlighting button. This was because it shouldn't have negative impacts/change existing workflows in negative ways and the editor interface is already quite overloaded.

I'm totally in support. I've updated the task description. The only reason we might reconsider is if bracket matching slows down some pages, but if this happens we can learn about it through bug reports.

awight renamed this task from Check bracket matching and tag matching add-on scope to Investigation: scope bracket matching and tag matching add-ons.Aug 7 2020, 9:23 AM
awight updated the task description. (Show Details)

Change 618956 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/CodeMirror@master] Import upstream addons: matchbrackets and matchtags

https://gerrit.wikimedia.org/r/618956

Tag matching *is* working in VE, but it seems that the cursor update event isn't triggering a refresh. See for example how my cursor is moved to the bottom of the document:

image.png (219×623 px, 16 KB)

Tag matching is deployed to the test instance.

One nice upgrade is that I was able to add all MediaWiki parser tags such as "<ref>" to the list of recognized tags.

Btw forgot to mention - thanks! Works great for me when I'm in the source code editor but don't see anything in VE. I don't fully understand the cursor update event, so I wasn't sure if this was the expected behavior at the moment or not.

awight set the point value for this task to 3.Aug 11 2020, 9:59 AM

Change 618956 abandoned by Thiemo Kreuz (WMDE):
[mediawiki/extensions/CodeMirror@master] Import upstream addons: matchbrackets and matchtags

Reason:
Now part of Ib01d991.

https://gerrit.wikimedia.org/r/618956

Change 618536 abandoned by Thiemo Kreuz (WMDE):
[mediawiki/extensions/CodeMirror@master] [POC] Enable the matchbrackets and matchtags add-ons

Reason:
Now part of Ib01d991.

https://gerrit.wikimedia.org/r/618536