Moodle

+1 cool! Is there anything in the filepicker that is not in YUI3 ?

The most important one is panel component, it creates filepicker interface, treeview and dialog too. I found a few third-part YUI3 dialog, but they don't look as good as YUI2 dialog.

hi, I have some committed some filepicker changes related to YUI3 loader - works fine for me now in editor for images unfortunately there is some regresion in filemanager (did not work for me before) and filepicker form element. Sorry for any potential problems, but I needed to clean up the includes in ajaxlib.php
please review my changes, thanks

Hi, Petr,
have got a few conflict when updating code, now fixed and commited, renamed filepicker to core_filepicker, I will move filepicker from Y to M after I complete filemanager yui3 upgrade.

Thanks for your work.

Notes on core_filemanager:
1. add key binds
2. main file support

core_filepicker:
1. add key binds
2. max files limitation.
3. Move to M global variable.

Petr,
Currenly, we need to add yui3 module manually on in js_module funciton, it is a bit hacky. I already added filemanager & filepicker there, and will add comments and form modules later.

Can we have a directory store all core modules, like

core/
        filepicker.js
        filemanager.js
        form.js
        comment.js

So js_module can find them automatically.

hi, I do not think core/something.js is a good idea because the JS would be far away from PHP code. We already have similar poblems with the render subsystems, I was proposing to make a list of subsystem locations and then use general name module.js, for example:

functio nget_core_subsystems() {
  return array('webservice'=>'/webservice/', 'comment'=>'comment', 'group'=>'/group/');
}

the code would then look for renderer.php, module.js, etc. in those specified locations.

The filepicker and form stuff is not a real subsystem, the mforms itself does not have full plugin architecture, so theme might be hardcoded in js_module() for now, anyway thanks for the ideas and your new code.

OK, thanks for that, I temporarily hardcorded core_comment module in js_module until you introduce new code.

I just reviewed your new comments code, nice! I think I found a few improvements:
1/ I did not like the new function in js-static, because it was again in global scope, so instead I improved the ->js_function_call() to include 'module' parameter, so that it is automatically load modules, this should simplify initialisation of M. subsystems I think
2/ I did not like the fact that the M.core_comment hold class, this should be general namespace imho mostly with functions, so so instead I reshuffled the stuff a bit there and created new init function useful for 1/
3/ found some unused data_for_js() code there

Please review my changes, feel free to propose any changes/improvements there. I personally think that the comments subsystem is a good example of a really complex usage of JS in Moodle 2.0

Hi, Petr
The reason I created that function in js-static is sam's $PAGE->requires->js_object haven't loaded on CVS yet (it is able to create instance of a js class).

The javascript anonymous function works better imho, for example:
var comment = new M.core_comment();
comment.post();

for your change, we need call a static function to do it:
var comment = M.core_comment.init();
comment.post();

the anonymous function trick looks more OOP.

All I need here is initializing M.core_comment, it is not quite necessary to have a static method to do it.

I was expecting the M object to have plugins as properties, then functions in those plugins. The M.core_comment was supposed to be a namespace, not the actual class. I would really like to make it something like packages in Java, not a brand new concept unique for Moodle again So no, my -100 for "new M.core_comment();", because it looks really ugly. This reminds me, we should use CamelCase for class names at least in javascript to differentiate them from variable_names.

Why "var comment = M.core_comment.init(); comment.post();" ? It is not necessary in current comments code imo, it already works after my latest change that included imporved $PAGE->requires->js_function_call() - everything nevessary is done in one line without any pesky init function in global scope in javascript-static which is a big nono, please put everything under M. now. Please, plugins MUST NOT add stuff into javascript-static, it really is not acceptable, the filepicker init has to go to M.core_filepicker. namespace.

"the anonymous function trick looks more OOP." - my main criteria is it has to be easy to understand, unfortunately OOP often makes things much, much harder to understand.

"All I need here is initializing M.core_comment, it is not quite necessary to have a static method to do it." Nope, static init method looks fine to me here and will be probably used in very many other places. Please let's do things consistently and as simple as possible

The init function usually looks at the DOM of page, modifies page UI and sets up event handlers. This way we may implement the progressive enhancements through Javascript. The actual implementation does not have to be exposed in the M. namespace because events handlers do not have to be exposed there, it is more like a public API.

I do not think the $PAGE->requires->js_object() actually solved our problems because we needed to do the Y.use('modulename', function(Y)) { somehow, the global Y was a hack trying to work around this, I still d not think it should exist and instead we should properly do YUI() or relay on current Y from higher scope.

Hi, Petr,

If you don't like js_object and want to keep everything out of js-static, please review my patch. The change you made changed the way how I think: there maybe several commenting boxes on the same page, each commenting box should be an instance.

The example I showed you, it is not just about comment system, your changes works well here, but for the others, I need to keep the instance of the class, so I can control the widgets on screen later on.

No, you should not do this: "new M.core_comment(options);" and {M.core_component.init == function()} at the same time, it is even worse than before. There are two ways to make these API:

1/ everything method static + some shared storage - this is how we did it actually in 1.9, and many areas will still do that in 2.0; it may not seem nice but at least it is very easy to understand for ppl without extensive knowledge of OOP and ppl that know more strict languages like Java. Please note that M.util.* will be all "static" sharing M.cfg

2/ the second way is to add instances of objects to M hierarchy on the fly. It is possible only at lower level, I do not like it at all at the second level, in your case the M.core_component.

The there is the documentation problem, in order to create nice JS Docs from our code we need so code in some "parse-able" way, these anonymous classes and instances might not help much.

Looking at your last c.js it looks to me like self modifying machine code:
1/ M.core_comment is a function that defines new class
2/ then it creates instance of that class and assigns it to self
3/ then you somehow inject static init method in in between 1/ and 2/
Is this really what you want to encourage all over the moodle codebase? I am personally having problems understanding that. I would definitely be amazed when I would saw this when I was starting with JS.

Why not simply do:
1/ M.core_comment.CommentManager - class definition
2/ M.core_comment.init(options) - does "new M.core_comment.ComponentManager(options)", each instance handles one block with comments
3/ from PHP use $PAGE->requires->js_funtion_call('M.core_comment.init', array($options1), 'core_comment'); $PAGE->requires->js_funtion_call('M.core_comment.init', array($options2), 'core_comment');

(My code in cvs now does this with exception that CommentManager class is created on the fly in local function scope only (can not be documented automatically), hmm, I am wondering how the garbage collection works in browsers, the instance is hanging in the air, only event handlers make it accessible from the outside, but it looks like it is enough).

Other plugins that just need to init page layout will also use $PAGE->requires->js_funtion_call('M.mod_forum.init_view', array(), 'mod_forum');, of course we could also use some $PAGE->requires->js_new_object($classname, $args, $modulename), but I would personally prefer the unified js_function_call() everywhere

hmm, I have found one bug in comment implementation - I can not delete just submitted comment, because the delete event is not registered on the newly created delete link. Tried adding scope.register_delete_buttons() into post(), but it looks like it is missing some id because the db update fails.

The rest of the comments seems to work fine for me here, tried glossary commenting together with comment block. IMHO I did not change how the code actually works, I just removed the self modification and shuffle the location of classes+methods.

In any case, lots of thanks for your feedback and your patience with me, this is exactly what we need now in this phase - find out all possible coding styles/patterns and choose one (or two) and use them consistently all over moodle codebase. I am learning a lot from your code too.

Another related issue is the /comment/admin.js script, it should also use the same core_comment module, it needs a room for living somewhere in M., I think it should be:
1/ M.core_comment.init_admin
2/ M.core_comment.admin_delete

In theory it could have separate module with fewer dependencies, but I think it is not going to be a problem, because the comment admin page is not used much - so no performance problems there imho.

Then maybe all the js needed for comments could go into /comment/modules.js to match other plugins

Yes, Petr, it looks strange to me as well when I started to write js in oop way, but after I read some js code in jquery/prototype/yui, I finally accepted the way they initialize js object, to write js in this way it is not trying to make the code prettier, it aims to make easier to reuse the code and avoid global namespace pollution. Javascript is not a completely OO language, we can make the interface looks like OO in many different ways, but it is hard to implement object/class in traditional OOP, at least it doesn't look like PHP/C++.

Static Method + some shared storage works very well for dom/event operation, but not very well with filepicker/filemanger/comment, you could find my previous code in CVS, take fllemanger for example, I need a object to store all instance data of filemanger, and to pass the client_id to every static method, it is hard to maintain it even for the people who wrote it, that's why I give up the static methods and using YUI3's new modules system.

Reading through your comments, I now understand the OO Javascript may not suitable for all developers, it may cause more problems than the benefits it gives. But for filepicker/filemanager/comment, it is more used by others instead of developing it, so the most important thing is to make the interfaces as simple as possible, I think we should make it looks like the way YUI3 creating widgets, for example slider & overlay.

Just my personal opinion, I think we should discuss on Monday, ask how the others think about this issue, alright?

Hi, I discovered Tim already started using interesting JS OOP code with .prototype., this might be another alternative for comments. I am also dealing with global Y which makes things a bit nasty, it kind of affects the OOP structure itself by the need of scoping when loading extra libraries through the .use()

Thanks for the info, I agree we need to find some suitable compromise tomorrow

Please have a look at lib/javascript-static.js, class M.util.CollapsibleRegion and M.uil.init_collapsible_region(), I think this could be another possible way for Comments too.

About prototype, I have an example:

var oo1 = function() {
    funciton privatemethod() {
    }
    this.id = 'test1',
    this.msg = function() {
        alert(this.id);
    }
}
// static field
oo1.classname ="oo1";
// class static method
oo1.staticmethod = function() {
}
var i1 = new oo1();
i1.msg();


function oo2() {
}
oo2.prototype.id = 'test2';
oo2.prototype.msg = function() {
    alert(this.id);
}

// class static method
oo2.staticmethod = function() {
}

var i2 = new oo2();
i2.msg();

oo1 and oo2 work the same way, from my point of view, oo1 is more 'like' a class, so I use this way more, prototype is more used to create instance methods outside of the class.

But both of this code cannot create private static fields and methods, if you want to use private static things, anonymous function is the only choice.

Hmm, there seems to be some misunderstanding, my main concern with your original code was that it seemed to be storing class definition and instance in the same place and moreover it was directly in "M.", instead all comments related stuff should go into M.core_comment.*.

I personally like all static or all instance approach, because JS makes it really hard to distinguish these on the fly. I studied current code and did my own JS OOP research, it all seems quite messy compared to good old Java, C#, etc.

I started thinking about some strict rules for our JS code:
1/ all prototyped classes use CamelCase with first Capital letter - this would help tremendously with identification of self contained classes in the M scope, for example M.util.CollapsibleRegion is a class that needs to be instantiated via init function
2/ do not create new instances of our classes manually - always use helper methods with "init_*" method - for example M.uil.init_collapsible_region(), this allows us to load extra dependencies properly via Y.use('xxx', function(Y) { new M.SomeThing()}); without the "this" scoping problems
3/ do not create any static methods in our prototyped classes that are instantiated
4/ use $PAGE->requires->js_init_call() instead of js_function_call(), because it adds automatically proper initialised Y as first param with basic dependencies loaded
5/ do not use $PAGE->requires->data_for_js(), always use init_* parameters instead
6/ if the init code for module is really small use ->js_init_code()

Hi guys,
DS pointed me towards this issue, my preference is as follows:

// Namespace + namespaced function definitions at the top, rolled into one object definition
// in Object Notation (JSON)
var M.namespace = { // Namespace declaration
    // A store for classes internally within the namespace
    ns_class : {},
    // A store for class instances likely more complex than this VERY simple example
    ns_instances : [],
    // A couple of static namespaced methods
    ns_method_one : function() {},
    ns_method_two : function() {},
    // An init function NOT CALLED INIT ! A namespace most likely uses more than one class and
    // init functionality where possible should be pushed in the class constructor
    ns_add_new_class : function(domid) {
        M.namespace.ns_instances[domid] = new M.namespace.class.some_class(domid);
    }
}

// A class definition stored within the namespaced class store
M.namespace.ns_class.some_class = function(domid) {
    // All internal properties should be defined in the constructor to make
    // the code obvious, yes you can do it on the fly but please avoid it.
    this.propertyone = null;
    this.propertytwo = null;
    // Init functionality should go here within the class constructor
    // not in an static init method.
    this.node = Y.one('#'+domid);
}
// Prototype is the standardised method for attaching class functionality
// and I think is very easy to read, most JSOOP tutorials/documentation will
// use prototype over inline declaration.
M.namesapce.ns_class.prototype.classfuncone = function() {}
M.namesapce.ns_class.prototype.classfunctwo = function() {}

As for the points raised above:

• We need a good naming convention that covers the intricacies of JS, needs to cover variables, properties, methods, functions, class definitions and namespaces and be suited for an environment where they will all be used/defined in a very similar fashion
• I don't agree with your point #2 Petr, set and forget class instances make good sense in some situations, total self control without the need for interaction with anything other than the DOM they are hooked to.... can be hard to achieve due to JavaScript scoping limitations in some cases (such as timers)
• The global Y is a hack, but I think it is a hack that GREATLY reduces code complexity, if we choose to go with requiring functions/classes to fetch their own Y instance I am happy with that but I prefer the global Y hack to it.

Petr
Just found a strange problem, the filemanager in resource module is broken, but worked elsewhere, I tracked down, found out it is because Y.Base is missing (but I can see the file in firebug), not sure what cause the problem, but if I use YUI.add('core_filemanager') define this module, problem solved, because it will automatically require dependences for module in javascript side, can you please consider my proposal to use YUI way to create our module again? It is safer imho.

I need to commit a temporary fix for this problem, feel free to comment other ways to fix this.

well, the filepicker embedding is incorrect, that is why it can not work:
$str .= html_writer::script(js_writer::function_call('destroy_item', array("nonjs-filepicker-{$client_id}")));
$str .= html_writer::script(js_writer::function_call('show_item', array("filepicker-wrapper-{$client_id}")));
$PAGE->requires->js_function_call('fp_init_filepicker', array('filepicker-button-'.$client_id, $options))->on_dom_ready();
$PAGE->requires->js_module('filepicker');

instead create one init function M.core_filepicker.init_filepicker_element(Y, client_id, options) and use $PAGE->requires->js_init_call(M.core_filepicker.init_filepicker_element($client_id, $options)

global Y is a horrible stuff that is not going to work, sorry:
1/ the Y is loaded asynchronously, the Y gets fully initialised a random later time, not when you assign it, the callback function(Y) solves exactly that
2/ if we ever use module versioning it will break horribly because each instance of Y may store only one version of each module

So please, let's kill all uses of global Y asap.

aaah, the YUI.add() does not know our M.yui.loader which specifies all the module details, the requires there can include only core YUI3 modules, not moodle modules or yui2