I have been reviewing and discussing his code all through the process, and in the last few days I have given it a final and extra-thorough review.
My opinion? this is _very_ ready for a merge into CVS HEAD. Of course there will be things to tweak, and the exposure to more webbrowsers will surely be interesting to see.
Also -- I think Mihai would be very deserving of CVS access. All through the reviews he has shown very good taste in his style and technical decisions, and can discuss tradeoffs in different approaches.
He can argue very clearly (and strongly ) for the options he favours, but can yield to other concerns and priorities.
He is still new to the PHP side of Moodle; much of the gsoc project was spent in JS-land. On the other hand, this brings a top-notch JS expert to the group -- with good understanding of what happens inside TinyMCE for example.
What does it do? On modern webbrowsers that support the HTML5 Canvas API, it provides a JS-based UI to "paint/edit/retouch" over an existing image, or on a new image. So TinyMCE has a new button.
"Upstream" project homepage
Docs on how it integrates with TinyMCE (and Moodle):
Git repository showing the well formatted (and commented!) commits for your reviewing pleasure:
Pretty diff -- click on the 'raw' link at the top to see a "real" patch.
There is only one thing that I am aware we might want to change during the merge. Paintweb has a makefile that controls the minifying of the JS (so editing/debugging happens of readable JS!). We may want to merge an already 'made' PW. Or change the scripts that build the release and nightly zipfiles to run 'make'.
(PW does run even if it's not yet "made". It loads the un-minified files, which is good for a hacking session, but very slow for production sites.)
I would also really like to see this merged and agree with Martins comments.
Some trivial comments on the 'diff':
If merging into head presumably it'd be nicer to not include the imagesave19.php or imageview19.php bits (and patches)
Would also be nice if imagesave20.php stuff was called imagesave.php in head
I have no idea how the editor plugins stuff works, if its cleanly implemented and that kind of thing. Does anyone with knowledge of that have comments on this? Is it going to give us tinymce upgrade pain?
Also, thanks to you for your quick reply. ;)
1. While I agree that imagesave19.php and imageview19.php are not needed, it should be taken into consideration that ... /lib/paintweb includes the "upstream" codebase. The "upstream" project handles the Moodle integration which includes support for 1.9 and 2.0. This is different from the TinyMCE integration which is handled by Moodle itself - the code outside of the project.
If we were to remove those two php scripts, I'd have to make a special target for my make script ("make moodle20").
I can do this, but please let me know if this is what you want.
2. Same as above. The makefile can handle this requirement.
3. TinyMCE 3 is nice on this. The PaintWeb plugin I added to TinyMCE does not pose any potential compatibility issues or upgrade pains. You just upgrade TinyMCE as usual - PaintWeb integration has no special needs.
I should note that in general, if you upgrade to a major TinyMCE release, which makes important API changes, they might break external plugins. If and when that happens, each TinyMCE plugin developer needs to update his code - which I will do (I'd just need a notification, hehe).
I hope this answers your concerns.
Thinking about it... yes, it makes your life (as upstream) and our life (as moodle devs) "easier". However...
If you include it "as is", it is a script that receives and saves a file, with no security checks. Not a good start
And we are talking about making a bit of a dirty hack on imagesave19.php to make things work on 1.9. The hack may well be not-super-secure. Moodle 2.0 definitely will not want the file there
To answer Dan's question: I do think that the integration is well done, both at the TinyMCE-to-Pw level and at the PW-to-Moodle level:
- PW uses lang strings from Moodle properly
- only appears if the user is allowed to upload attachments
- it can be disabled with a config variable (do we have a UI for this?)
- PW blocks 'submit' until the user is done, with a reasonable error msg
- UI feedback during img upload
Of course, there may be other bits of polish that are missing.
I already have the "moodle" Makefile target. I could just add "moodle20" (and moodle19 when the time comes). So, is this settled? Should I do it now?
No, there's no UI for disabling PaintWeb. I didn't venture into the Moodle settings/configuration code because that depends on lots of factors. If someone wants to do this, they can - or I can also do it, but I'd need some guiding. ;)
IIRC there's been a revamp of that API, so I am not sure what the right way is...
Regarding settings, I think I will hold off at the moment. Let's see what Petr Skoda has in mind. As I saw today in a dev chat this is planned in a wider approach.
With this occasion I have also made other improvements to the make process, by adding a configuration file. I included instructions on how to build PaintWeb in the INSTALL file - it's much clearer now how to do it all. ;)
We need to make the editors more pluggable and customisable, we should also refactor the JS embedding code together with the repository integration a bit. There are also some other theme related changes planned that will affect the editors.
My current plan is:
* finish my part of Web service related work
* restart my work on themes improvements
* continue with editor refactoring and general text format support
I think that the maths tools and paintweb are perfect test cases - it should be possible to deploy them as real plugins without any hacking required.
Then we should imho concentrate on usability and decide which plugins and default settings should be included in 2.0.
My personal main objective is to allow replacing of text editor, easy extensions and custom configurations of editors together with user preferences.
I have some nice new findings in latest math tool tests http://moodle.org/mod/forum/discuss.php?d=133132#p590927 that may help also your work and can be usable both in moodle 2.0 and previous versions - particularly my latest tests with Google Chart Api Tex and Dragmath can make using of maths much easier than it has been before with plain tex.
And Your pluggable systems change everything much easier also in implementation of any new features to moodle - S U P E R !!!
Sorry for bothering... did you have the chance to work on the new HTML editor embedding API? I'd like to update the integration of PaintWeb in time for Moodle 2.0.
Is there any news about paintweb integration in Moodle 2?