Paintweb integration ready for merge...

Paintweb integration ready for merge...

by Martín Langhoff -
Number of replies: 11
Mihai Sucan has done an outstanding job refactoring his Paintweb application, integrating it with Moodle's TinyMCE and working on the bugs and browser quirks.

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 smile ) 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.

Enough preambles!

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
http://code.google.com/p/paintweb/

Docs on how it integrates with TinyMCE (and Moodle):
http://code.google.com/p/paintweb/wiki/UsageInTinyMCE

Git repository showing the well formatted (and commented!) commits for your reviewing pleasure:
http://repo.or.cz/w/moodle/mihaisucan.git?a=shortlog;h=refs/heads/mdl20-paintweb

Pretty diff -- click on the 'raw' link at the top to see a "real" patch.
http://repo.or.cz/w/moodle/mihaisucan.git?a=treediff;h=b99427ae693fff343067de2e82fdc72966e6b83e;hp=92eaeca5c9415eafadaa316fba948f5786f6cf03;hb=b99427ae693fff343067de2e82fdc72966e6b83e;hpb=92eaeca5c9415eafadaa316fba948f5786f6cf03

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.)
Average of ratings: -
In reply to Martín Langhoff

Re: Paintweb integration ready for merge...

by Dan Poltawski -

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?

Great work!

Average of ratings: -
In reply to Dan Poltawski

Re: Paintweb integration ready for merge...

by Mihai Sucan -
First of all, thanks to MartinL for his appreciation.

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.
Average of ratings: -
In reply to Mihai Sucan

Re: Paintweb integration ready for merge...

by Martín Langhoff -
> imagesave19.php

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 mixed

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 smile

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.
Average of ratings: -
In reply to Martín Langhoff

Re: Paintweb integration ready for merge...

by Mihai Sucan -
Yep, you have a point.

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. ;)
Average of ratings: -
In reply to Mihai Sucan

Re: Paintweb integration ready for merge...

by Martín Langhoff -
The makefile target idea sounds good. WRT to settings... you'll probably have to add it for this to be considered complete.

IIRC there's been a revamp of that API, so I am not sure what the right way is...
Average of ratings: -
In reply to Martín Langhoff

Re: Paintweb integration ready for merge...

by Mihai Sucan -
Oky, thanks. I'll see to the implementation of the new Makefile targets.

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.
Average of ratings: -
In reply to Martín Langhoff

Re: Paintweb integration ready for merge...

by Mihai Sucan -
Done. I have added the two makefile targets. Please let me know if this is OK for you.

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. ;)
Average of ratings: -
In reply to Martín Langhoff

Re: Paintweb integration ready for merge...

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
Sorry, the editors framework is not yet ready for merging of any extra tools.

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.

Petr
Average of ratings: Useful (2)
In reply to Petr Skoda

Re: Paintweb integration ready for merge...

by Mauno Korpelainen -
Marvellous, Petr!

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 !!!


Average of ratings: -
In reply to Petr Skoda

Re: Paintweb integration ready for merge...

by Mihai Sucan -
Hello Petr!

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.

Thanks.
Average of ratings: -
In reply to Mihai Sucan

Re: Paintweb integration ready for merge...

by Juan Leyva -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers

Is there any news about paintweb integration in Moodle 2?

Regards

Average of ratings: -