| Yamaha5 | |
| May 29 2012, 10:08 AM |
| F16444150: Capture.PNG | |
| Mar 30 2018, 2:08 AM |
| F13711451: aceFilter.png | |
| Feb 13 2018, 1:13 PM |
As an maintainer of edit filters, I want to have syntax highlight so that I can understand and review the code more easily.
The MW extension SyntaxHighlight GeSHi already provides syntax highlight for CSS, JS and Lua pages, and most code editors provides some form of syntax highlight (e.g. Notepad++ on Windows, Atom and Kile on linux).
See Also:
+1. It would help with filters filled with giant regexes. At the minimum, bracket highlighting like Notepad++ and other text editors' would greatly help.
Since AbuseFilter only has simple constructs, I think a bracket highlight would be a nice compromise. It shouldn't be too hard to implement, while being of great help with nested brackets.
Just reminding after the merge for whoever comes here without seeing the other task: the way syntax highlight should be implemented is to use CodeEditor, which also provides an easier to read edit box, even without syntax highlight. By the way, I was thinking of a possible way to implement Ace editor: AF pages are not "real" pages (like the others where CodeEditor is enabled) and don't have a toolbar, thus CodeEditor sounds like redundant for this task. In order to keep other things as they are right now, should we consider to directly add Ace to AF? Also, if we want a total syntax highlight, we should adapt Ace to the specific syntax of AF.
Little update: I'm unofficially working on this. I made a scratch with a fresh (and reduced) implementation of Ace, which is actually working quite well, though it's really far from a final state. I won't claim this task until I'll get a more concrete result, though I hope to lead good news within a few days. Also, since I'm not that good with PHP and especially with MW, it would be really appreciated to have some thoughts about the proposed implementation.
Now the result is concrete. It is also almost ready to be committed, it just needs some stylistic fixes and minor changes. There will probably be several things to be discussed/reviewed/changed after sending the commit, which I plan to do tomorrow. As a side note, when this task will be resolved we may think to switch syntax validation directly to Ace, which will provide more precise, intuitive and real-time validation, possibly including warning and or performance tips.
I think validation should occur both at the client- and the server-side if possible. Glad to see you are working on this.
@Huji Sure, though we may use as client-side validation the line-by-line one provided by Ace. In my opinion, it would be many, many times better than pushing everytime a "check syntax" button. Anyway, I'll eventually open another task for this, when (and if) this task will be resolved.
Change 410129 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Switch editor to Ace and provide syntax highlight
Well you can at least just load the RL module "ext.codeEditor.ace"
Then if you want to support some of the wiki specific stuff, you can split the jquery.codeEditor.js file in multiple pieces, so that you are able to reuse the toolbar etc, without loading the wiki editor.
@TheDJ Thanks for answering! I'll take a look on how to load the module. However, we don't actually need many tools like the toolbar, at least from my POV: this is how I implemented this, and it should be enough.
"ext.codeEditor.ace": { "group": "ext.codeEditor.ace", "scripts": [ "ace/ace.js", "ace/mode-javascript.js", "ace/mode-json.js", "ace/mode-css.js", "ace/mode-lua.js", "ace/ext-language_tools.js", "ace/ext-modelist.js" ] },
I was thinking maybe a little bit of refactoring to that, so AF doesn't need to load json/css/lua js files too (so split them into another module maybe?). But that's a minor thing
@Daimona ext.codeEditor.ace is just the ace files from CodeEditor:
https://github.com/wikimedia/mediawiki-extensions-CodeEditor/blob/master/extension.json#L78
Avoids having to deal with two sets of JS files to check and handle
Also in order to make autoloading of stuff from ace possible in foundation setups, you probably should check out
https://github.com/wikimedia/mediawiki-extensions-CodeEditor/blob/master/modules/jquery.codeEditor.js#L351
where we do basePath and language configuration for the internal loader of Ace.
https://github.com/wikimedia/mediawiki-extensions-CodeEditor/blob/master/modules/jquery.codeEditor.js#L509
is the statusbar of CodeEditor, showing cursor positions and linting warnings.
Right, having only one set of JS files should be good; however, if I'm right, the "mode-abusefilter.js" file should then be added to CodeEditor (and also ext-modelist should be updated). As for the autoloading and configuration, are they actually needed in a simple context as AbuseFilter? Finally, at the moment we don't need linting warnings since we don't have a built-in syntax validation, and cursor position migh be superfluous too. Those might still be added in the future, if we will write a mode for AF syntax. Probably it's better to implement these things one-by-one, to fix unexpected bugs and maybe try to gather some feedbacks.
So, recapping, is it enough to delete Ace files and use instead codeEditor resource? If so, I'll update the commit and propose the needed modifications to codeEditor.
That sounds about right, yup.
Add CodeEditor as a dependancy https://www.mediawiki.org/wiki/Manual:Extension_registration#Requirements_(dependencies)
Re-use the RL modules in CodeMirror (you still need to "addModule" them to a page). Then simplify your module to just add your abuseFilter mode. Add that to the page at the same time
Change 410173 had a related patch set uploaded (by Reedy; owner: Reedy):
[integration/config@master] Make AbuseFilter depend on CodeEditor
For some reason my gerrit access isn't working, so leaving a comment here. I suspect the current proposed patch might break when a user has JS disabled in his browser. Please double check (we have quite a few admins who disable JS). Looking pretty spiffy otherwise, but i'd have to setup vagrant + AF at home for a proper test.
Delete your cookies :) https://lists.wikimedia.org/pipermail/wikitech-l/2018-February/089501.html
Well, 410173 needs to be merged or jenkins won't test the commit. BTW, I'm testing this latest build and it seems to work, except for a thing: if I Ctrl+f inside the edit box, the searchbox won't show up and instead in JS console I get: Refused to execute script from 'xxxxxxxx/Speciale:FiltroAntiAbusi/ext-searchbox.js' because its MIME type ('text/html') is not executable, and strict MIME type checking is enabled." With the previous build I could fix this by adding ext-searchbox to extension.json, though I don't know how to fix it now :D
@TheDJ : Might be unrelated, but I also can't login on gerrit; the only way is to use private navigation, otherwise it won't enter. As for javascript, every page to edit filters requires it, otherwise some stuff won't work (just tested).
CodeEditor had the ext-searchbox.js file, but it doesn't seem to be loaded into a RL module. A patch to CodeEditor should be easy enough to fix that
@Reedy Sure, that would be an easy fix. However the searchbox works in CodeEditor. The problem might be elsewhere, otherwise I'll make that patch.
{ name: "find", bindKey: bindKey("Ctrl-F", "Command-F"), exec: function(editor) { config.loadModule("ace/ext/searchbox", function(e) {e.Search(editor)}); }, readOnly: true }
Is it just loading it on the fly? And it's not working due to some path difference issue?
No, THIS is why i said you needed the basePath config stuff :) So that the internal loader of ACE knows where to look for it's resources, without having to register everything in resource loader modules :)
Yes, that's the loading. Sincerely I can't tell why it's not working, it should work even without adding it to RL.
@TheDJ Oh! Now it's clear. I'm sorry but I didn't read the whole javascript. So, isn't it enough to import lines 356-363 to ext.abuseFilter.edit? Of course after rearranging them.
Change 410173 merged by jenkins-bot:
[integration/config@master] Make AbuseFilter depend on CodeEditor
Oh, fine, thanks. In the meanwhile I'll try to set the path for searchbox and other stuff.
As noted by @Legoktm on gerrit, we'd better avoid CodeEditor dependency due to distribution issues. As I said there, we may check if codeEditor is installed and if not return to the old textarea, but it sounds like a waste of time from my POV. Given this, shouldn't we reconsider going back to the standalone ace implementation? Then, when codeEditor will be distributed in the tarball we may go back again to the dependency.
I think having two copies of ace deployed is going to be problematic from a maintenance standpoint.
As I said there, we may check if codeEditor is installed and if not return to the old textarea, but it sounds like a waste of time from my POV.
Why do you think it's a waste of time?
I know it might not be a nice solution but how else can we do? As a side note, are there partiular reason that keep codeEditor from being added to tarball?
I think it might be a waste of time since the code should be reworked while we could avoid this by simply providing a dedicated copy of ace. I really don't see any other simple solution, but I'd be glad to hear some thoughts and work on them tomorrow. Finally, I'd like to ask if it would be possible to set the opposite dependency, i.e. moving ace to AF and make CE depends on it.
In theory, yes. In practice, it doesn't make sense. Making extensions depend on random unrelated extensions just signifies poor thought/design.
AbuseFilter has "code" (the rules in it). So having it depend on a "CodeEditor" to provide functionality makes sense.
CodeEditor provides syntax highlighting.. What logical functionality would it expect to get from needing an extension called AbuseFilter? Never mind other functionality from AF that would now be on the wiki, and may not be expected to be there...
The other ways around this would... Is either putting things into MW Core (or vendor. Historically, we might just have had an extension *just* to provide ace... But we try not to do that now), or something more like T107561: MediaWiki support for Composer equivalent for JavaScript packages (when it's finished), where both extensions could require ace, but the resultant MW install would only have one copy to include etc.
So, rather than a hard dependancy... We can have a soft one. See https://www.mediawiki.org/wiki/Manual:Extension_registration#Check,_if_an_extension_is_loaded_without_actually_require_it
T182472: Allow extension registration to suggest optional extension/skin or MediaWiki version was created as a way to add visibility to checks like the above. When Extension B can depend on Extension A, but it doesn't have to have it installed for it to work. But if you do, you get more features etc.
But we can also have the discussion about bundling CodeEditor.... However. We don't bundle AbuseFilter currently, so isn't this a non issue?
I know it doesn't make sense. However, just to be precise, that solution (and also the one with embedding Ace in AF) were temporary, at least until there'll be a better solution. I was also thinking of including Ace directly in the core, I'd say it's the best solution.
I've seen some soft dependency here and there and I partially know how to implement them, my only concern in doing this is that we could avoid such situation in relatively simple (and temporary) ways, e.g. embedding Ace.
I wasn't aware AF isn't included as well, this may actually reduce possible solutions.
So, I see we either add Ace to the core (or wait for T107561), which would also be handy for future extensions requiring ace, or I'll establish a soft dependence.
BTW, I'm still thinking of embedding Ace directly here. We may only need to keep the code for some time (until the solution above is ready), and I don't think we may face that many troubles in such a limited amount of time.
Seems to me like it should be possible to have CodeEditor as an optional dependency.
It's a bit more work, because you need to manually load the RL module, from the ext.abuseFilter.ace module code, and you need to make everything that it touches optional, but since it's pure JS, i don't see why that wouldn't be possible. I mean, the CodeEditor itself can switch between plain text area's and the editor with an enable/disable button in the toolbar, so it should be possible (then you also don't need all those php changes).
@TheDJ In fact it totally is, I was only trying to keep it as the last available option.
I tried to rework my patch by adding checks for codeEditor, making it optional. If we don't have better solutions, I'll upload it on gerrit ASAP. However it'll probably need a review, since I'm not totally sure of what I've done.
I'm having a delay in submitting the patch, since I just found out a strange behaviour: if the rules of the filter contain HTML tags in a string, e.g. "<small>foo</small>" (maybe it also goes for other kinds of tags, I didn't test properly), the filter can be saved, but when it's opened again such tags are not displayed, nor they appear in the source code of both ace editor and dummy textarea. However, they still appear in the diff, even in following edits. This might either be a trouble of sanitization or it may depend on how ace returns values, or might even be some silly mistake that I made in the code. In any case, I probably won't be able to work on it until tomorrow.
I finally managed to do it, plus some little fixes here and there. Apart from following reworkings, my concern now is if we need some other changes to I/C: the dependency to CodeEditor should be removed, and is there a way to test it both with and without CodeEditor?
Change 410129 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Switch editor to Ace and provide syntax highlight
This caused the rule editor in /test and /tools to collapse. Reopening to give @Daimona an opportunity to fix it.
Better. I changed it to max-width though; that way if you resize the window to half the width of the screen, you don't get an unnecessary horizontal scrollbar.