Details

    • Testing Instructions:
      Hide

      This feature adds the ability to drag and drop files into a course while editing it and have then automatically added as activities.
      The following tests are loosely described:

      1. Log in as an admin
      2. Purge your caches (version bump for weekly release will remove the need for this)
      3. Enter a course and attempt to drag a file over a section. Make sure nothing happens.
      4. Turn on editing
      5. Try dragging again and this time make sure you see a notice at the top of the screen and a notice at the bottom of each section you drag over.
      6. Try dragging and dropping the following files over a section to make sure they work. Check the activity type that gets added makes sense:
        • .png image
        • .jpg image
        • .txt document
        • .pdf document
        • .csv file
        • .docx document
        • A file without an extension
        • A zip file (you should get an option about what to do, try both!)
        • An MBZ file
        • A url (highlight the URL in your browser and drag that over a section)
      7. Disable the upload repository and dragging a file, make sure nothing happens (because you can't upload)
      8. Re-enable the upload repository
      9. Disable the file, folder, page, and url activities
      10. Make sure you can't drag anything into the course to create an activity
      11. Renable all of them
      12. Repeat these in all supported browsers.

      Please note there are a couple of known issues, please see the linked issues.

      Show
      This feature adds the ability to drag and drop files into a course while editing it and have then automatically added as activities. The following tests are loosely described: Log in as an admin Purge your caches (version bump for weekly release will remove the need for this) Enter a course and attempt to drag a file over a section. Make sure nothing happens. Turn on editing Try dragging again and this time make sure you see a notice at the top of the screen and a notice at the bottom of each section you drag over. Try dragging and dropping the following files over a section to make sure they work. Check the activity type that gets added makes sense: .png image .jpg image .txt document .pdf document .csv file .docx document A file without an extension A zip file (you should get an option about what to do, try both!) An MBZ file A url (highlight the URL in your browser and drag that over a section) Disable the upload repository and dragging a file, make sure nothing happens (because you can't upload) Re-enable the upload repository Disable the file, folder, page, and url activities Make sure you can't drag anything into the course to create an activity Renable all of them Repeat these in all supported browsers. Please note there are a couple of known issues, please see the linked issues.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-22504_drag_and_drop_upload_final
    • Rank:
      1293

      Description

      Now that gmail has shown that it can be done (for recent versions of Firefox 3.6 / Chrome), it would be great if you could add files directly to a Moodle 2.0 course by dragging & dropping from the user's desktop, using some of the new features of HTML5.

      Even if it can't be done directly, would there be an API that could be left open to allow a 3rd-party plugin to add this functionality?

        Issue Links

          Activity

          Hide
          Martin Dougiamas added a comment -

          I guess the natural place for this would be in the Upload repository plugin inside the file picker.

          Details are going to take some thinking about ... I can't see this being done for 2.0 unfortunately.

          Show
          Martin Dougiamas added a comment - I guess the natural place for this would be in the Upload repository plugin inside the file picker. Details are going to take some thinking about ... I can't see this being done for 2.0 unfortunately.
          Hide
          Davo Smith added a comment -

          I'm thinking it would be nice to be able to drop files directly onto the course (possibly also do the same with URLs) and have a resource automatically created with a default title.

          I'm up for having a go at the code myself (assuming I have some free time at some point), but fine with it being a Moodle 2.1 issue.

          Show
          Davo Smith added a comment - I'm thinking it would be nice to be able to drop files directly onto the course (possibly also do the same with URLs) and have a resource automatically created with a default title. I'm up for having a go at the code myself (assuming I have some free time at some point), but fine with it being a Moodle 2.1 issue.
          Hide
          Dongsheng Cai added a comment -

          Hi Davo

          That will be very handy to add new files to courses, hope you can share your knowledge about dnd in html5, I am interested in implemented this in moodle file picker.

          Thanks

          Show
          Dongsheng Cai added a comment - Hi Davo That will be very handy to add new files to courses, hope you can share your knowledge about dnd in html5, I am interested in implemented this in moodle file picker. Thanks
          Hide
          Davo Smith added a comment -

          At the moment, my knowledge of this is limited to a couple of articles I have read (but not had any time to do anything with).

          The relevant articles are:
          http://www.thecssninja.com/javascript/gmail-upload
          http://www.thecssninja.com/javascript/fileapi

          Show
          Davo Smith added a comment - At the moment, my knowledge of this is limited to a couple of articles I have read (but not had any time to do anything with). The relevant articles are: http://www.thecssninja.com/javascript/gmail-upload http://www.thecssninja.com/javascript/fileapi
          Hide
          Davo Smith added a comment -
          Show
          Davo Smith added a comment - I've recently implemented this as Moodle block - http://moodle.org/mod/data/view.php?d=13&rid=4963 (or https://github.com/davosmith/moodle-block_dndupload )
          Hide
          Davo Smith added a comment -

          I've had an initial attempt at this feature as a core patch, based on my drag & drop upload block.

          It's still a bit rough around the edges, but I'd appreciate any feedback that is available.

          Show
          Davo Smith added a comment - I've had an initial attempt at this feature as a core patch, based on my drag & drop upload block. It's still a bit rough around the edges, but I'd appreciate any feedback that is available.
          Hide
          Dan Poltawski added a comment -

          Hi Davo,

          From the looks of this, its much similar to your excellent block for automatic upload into course sections.

          I think the first step before that would be to great to get drag/drop into the file element of forms (rather than needing to click and and select upload and select file, just drag into that form element).

          With regard to uploading directly into sections. Whilst this is very very useful I think we'd need to think about the best way to 'autocreate' course modules (rather than hardcoding the different resource types). Perhaps this might be a module hook or setting - ideally we'd allow third-party modules to benefit from it.

          Show
          Dan Poltawski added a comment - Hi Davo, From the looks of this, its much similar to your excellent block for automatic upload into course sections. I think the first step before that would be to great to get drag/drop into the file element of forms (rather than needing to click and and select upload and select file, just drag into that form element). With regard to uploading directly into sections. Whilst this is very very useful I think we'd need to think about the best way to 'autocreate' course modules (rather than hardcoding the different resource types). Perhaps this might be a module hook or setting - ideally we'd allow third-party modules to benefit from it.
          Hide
          Davo Smith added a comment -

          I've added a patch to the sub-task MDL-29766 which implements drag and drop onto a Filemanager element in, what I hope, is a fairly sensible and clean way.

          After this, I'll get on with trying to design a sensible way of allowing the 'drag and drop onto a course' code to work.

          Show
          Davo Smith added a comment - I've added a patch to the sub-task MDL-29766 which implements drag and drop onto a Filemanager element in, what I hope, is a fairly sensible and clean way. After this, I'll get on with trying to design a sensible way of allowing the 'drag and drop onto a course' code to work.
          Hide
          Nadav Kavalerchik added a comment -

          If any one is looking for a similar functionality and he/she is using Moodle 1.9 ...
          You can use: http://tracker.moodle.org/browse/CONTRIB-2730

          Show
          Nadav Kavalerchik added a comment - If any one is looking for a similar functionality and he/she is using Moodle 1.9 ... You can use: http://tracker.moodle.org/browse/CONTRIB-2730
          Hide
          Davo Smith added a comment -

          I've put together a rough design document in the Moodle wiki, covering what I am currently implementing on this issue, it can be found here:
          http://docs.moodle.org/dev/Course_drag_and_drop_upload

          Show
          Davo Smith added a comment - I've put together a rough design document in the Moodle wiki, covering what I am currently implementing on this issue, it can be found here: http://docs.moodle.org/dev/Course_drag_and_drop_upload
          Hide
          Davo Smith added a comment -

          This is a pretty big patch, but I think it basically covers everything needed to get drag and drop of files, text and links onto course pages working.

          There are still some definite rough edges - YUI has defeated me when it came to submitting the pop-up dialog box with the 'enter' key, the notification at the top of the page could do with a designer to look at it.

          Things that can be done:

          • select one or more files and drop onto a course to upload
          • select some text in an application and drag onto a course to create a 'page'
          • drag a URL from a browser page or the address bar to create a URL
          • drag a zip file and choose 'unzip' to create a folder resource

          I've hopefully added all the necessary comments, GPL notices and PHP docs.

          Looking forward to some feedback on this.

          Show
          Davo Smith added a comment - This is a pretty big patch, but I think it basically covers everything needed to get drag and drop of files, text and links onto course pages working. There are still some definite rough edges - YUI has defeated me when it came to submitting the pop-up dialog box with the 'enter' key, the notification at the top of the page could do with a designer to look at it. Things that can be done: select one or more files and drop onto a course to upload select some text in an application and drag onto a course to create a 'page' drag a URL from a browser page or the address bar to create a URL drag a zip file and choose 'unzip' to create a folder resource I've hopefully added all the necessary comments, GPL notices and PHP docs. Looking forward to some feedback on this.
          Hide
          Dan Poltawski added a comment -

          Hi Davo,

          I don't think the branch has all the changes on it (I can't see lib/dnduploadlib.php).

          Cheers,

          Dan

          Show
          Dan Poltawski added a comment - Hi Davo, I don't think the branch has all the changes on it (I can't see lib/dnduploadlib.php). Cheers, Dan
          Hide
          Davo Smith added a comment -

          OK - looks like I managed to miss out the new files when committing to git.

          I've pushed a fixed version out.

          Show
          Davo Smith added a comment - OK - looks like I managed to miss out the new files when committing to git. I've pushed a fixed version out.
          Hide
          Dan Poltawski added a comment -

          Adding Dan M & MD here..

          Dan: I wonder if you could find the time to look at what Davo has proposed from a module author point of view. I'm thinking the example of draging and dropping a scorm package onto the course page - could you add support for drag/drop of a scorn package onto the page?

          Martin: Just wondering about your thoughts on seeing this in core.

          Show
          Dan Poltawski added a comment - Adding Dan M & MD here.. Dan: I wonder if you could find the time to look at what Davo has proposed from a module author point of view. I'm thinking the example of draging and dropping a scorm package onto the course page - could you add support for drag/drop of a scorn package onto the page? Martin: Just wondering about your thoughts on seeing this in core.
          Hide
          Dan Marsden added a comment -

          very cool idea - would be nice if you could choose to parse the filename for a title or the imsmanifest.xml for the title to the SCORM (site level setting maybe?)

          if I get time I'll try to look later this week!

          Show
          Dan Marsden added a comment - very cool idea - would be nice if you could choose to parse the filename for a title or the imsmanifest.xml for the title to the SCORM (site level setting maybe?) if I get time I'll try to look later this week!
          Hide
          Davo Smith added a comment -

          You are welcome to pull the name out of the imsmanifest.xml file - the value passed into the mod is only a suggestion (based on the name of the file), if you set something different, then this should be picked up automatically (by 'get_fast_modinfo') and sent back to the browser.

          Either way, a global configuration option within the SCORM module would seem like the best place to control that behaviour.

          Show
          Davo Smith added a comment - You are welcome to pull the name out of the imsmanifest.xml file - the value passed into the mod is only a suggestion (based on the name of the file), if you set something different, then this should be picked up automatically (by 'get_fast_modinfo') and sent back to the browser. Either way, a global configuration option within the SCORM module would seem like the best place to control that behaviour.
          Hide
          Martin Dougiamas added a comment -

          Thanks for all the work, Davo! Will have a look soon.

          Show
          Martin Dougiamas added a comment - Thanks for all the work, Davo! Will have a look soon.
          Hide
          Davo Smith added a comment -

          If you want to see the code in action, I have set up a demo site here: http://coursednd.dev.synergy-learning.com/course/view.php?id=2
          username: teacher
          password: teacherpass

          There are instructions in the first course section.

          If you do have a play with it, please can you delete any uploaded files when you are finished? (I'll take the site down again once it has served its purpose).

          There are certainly still one or two rough edges to sort out, but I'd like confirmation that the basic operation is OK, before I start messing around with smaller details.

          Show
          Davo Smith added a comment - If you want to see the code in action, I have set up a demo site here: http://coursednd.dev.synergy-learning.com/course/view.php?id=2 username: teacher password: teacherpass There are instructions in the first course section. If you do have a play with it, please can you delete any uploaded files when you are finished? (I'll take the site down again once it has served its purpose). There are certainly still one or two rough edges to sort out, but I'd like confirmation that the basic operation is OK, before I start messing around with smaller details.
          Hide
          Dan Marsden added a comment -

          just took a look at this again - any chance we could modify it so that the module can "inspect" the file before deciding if it is supported rather than just looking at the file type? for example it would be nice to be able to check that it is a zip and it contains an imsmanifest.xml file before showing the option to upload as scorm.

          Show
          Dan Marsden added a comment - just took a look at this again - any chance we could modify it so that the module can "inspect" the file before deciding if it is supported rather than just looking at the file type? for example it would be nice to be able to check that it is a zip and it contains an imsmanifest.xml file before showing the option to upload as scorm.
          Hide
          Dan Poltawski added a comment -

          Hi Davo,

          Sorry for the delay in getting this peer review started and as always thanks for all your work on this - its looking great.

          Here are my notes in progress (not complete):

          lib/ajax/dndupload.js

          1. In general I wonder if we can share more between this and the form drag and drop javascript. Abstracting away shared code handling and tracking drag events and handling the uploads in something shared and just dealing with the pecularities of each in their own js file.
          2. Martin asked me to look at how this interacts which potential changes to course formats. I think the best recommendation I can make to reduce the impact of this is define as 'constants' CSS classname/dom identifiers so they are clearly identified in the code and easy to find/change. We might have full pages with only one section and other such things in the future. Example identifiers which are hardcoded within the code:
            this.Y.all('li.section.main').each( function(el) {
                        this.add_preview_element(el);
            
            getElementById('page-content')
            
            test.hasClass('section') && test.hasClass('main');
            
            section.one('ul.section');
            

            On those lines I think this should be abstracted into a function:

            // Work out the number of the section we are on (from its id)
                    var section = this.get_section(e.currentTarget);
                    var sectionid = section.get('id').split('-');
                    if (sectionid.length < 2 || sectionid[0] != 'section') {
                        return false;
                    }
                    var sectionnumber = parseInt(sectionid[1]);
            
          3. I think we need to look at the ajax course editting work that Andrew Nicols has been doing regarding ajax_edit_enabled(), add_resource_element() and add_section() - I think this might tie into the course format work too. That main object is weirdly un-namespaced to me maybe Andrew got rid of it.
          4. A few of the icons are hardcoded to 16x16 in JS - can this not be done with a css class which presumably already exists for this. (Might break in some themes)
          5. I don't think this is a safe id generator - surely it should have a time component too var uploadid = Math.round(Math.random()*100000);
          6. Much be worth a note about using XMLHttpRequest rather than Y.io again.

            dndupload_handler::__construct

          7. The harcoded core stuff (// Add some default types to handle) - can this be done by the plugins which handle them? What happened if someone removes the url resource and nothing handles urls?
          8. I think rather than doing your own callback work you will be able to use component_callback function

            lang/en/dndupload.php

          9. Some of the error strings introduced probably do not need to be introduced if they are not going to be an issue in normal use. (i.e. if only seen by developers) (examples - invalid course id and sesskey)
          Show
          Dan Poltawski added a comment - Hi Davo, Sorry for the delay in getting this peer review started and as always thanks for all your work on this - its looking great. Here are my notes in progress (not complete): lib/ajax/dndupload.js In general I wonder if we can share more between this and the form drag and drop javascript. Abstracting away shared code handling and tracking drag events and handling the uploads in something shared and just dealing with the pecularities of each in their own js file. Martin asked me to look at how this interacts which potential changes to course formats. I think the best recommendation I can make to reduce the impact of this is define as 'constants' CSS classname/dom identifiers so they are clearly identified in the code and easy to find/change. We might have full pages with only one section and other such things in the future. Example identifiers which are hardcoded within the code: this .Y.all('li.section.main').each( function(el) { this .add_preview_element(el); getElementById('page-content') test.hasClass('section') && test.hasClass('main'); section.one('ul.section'); On those lines I think this should be abstracted into a function: // Work out the number of the section we are on (from its id) var section = this .get_section(e.currentTarget); var sectionid = section.get('id').split('-'); if (sectionid.length < 2 || sectionid[0] != 'section') { return false ; } var sectionnumber = parseInt(sectionid[1]); I think we need to look at the ajax course editting work that Andrew Nicols has been doing regarding ajax_edit_enabled(), add_resource_element() and add_section() - I think this might tie into the course format work too. That main object is weirdly un-namespaced to me maybe Andrew got rid of it. A few of the icons are hardcoded to 16x16 in JS - can this not be done with a css class which presumably already exists for this. (Might break in some themes) I don't think this is a safe id generator - surely it should have a time component too var uploadid = Math.round(Math.random()*100000); Much be worth a note about using XMLHttpRequest rather than Y.io again. dndupload_handler::__construct The harcoded core stuff (// Add some default types to handle) - can this be done by the plugins which handle them? What happened if someone removes the url resource and nothing handles urls? I think rather than doing your own callback work you will be able to use component_callback function lang/en/dndupload.php Some of the error strings introduced probably do not need to be introduced if they are not going to be an issue in normal use. (i.e. if only seen by developers) (examples - invalid course id and sesskey)
          Hide
          Davo Smith added a comment -

          @Dan M - I'm not sure if pre-inspection of the files would work. Aside from being more difficult to code, it might produce a slightly more confusing UI, where you drag several files and then, at some random time later (depending on upload speed) boxes start popping up to ask what to do with them. With the current version, the decision box comes up immediately and then the files get on with uploading in the background.

          I think that dropping a zip file on a course and choosing 'SCORM', followed by getting an error later is not that dissimilar to adding a SCORM module and attempting to add an invalid zip file directly - most users aren't going to do it unless they have some expectation that they really have a SCORM file and if they are expecting it to be a SCORM file, but it isn't, then it would be more confusing to hide the 'treat as SCORM' option, than to give an error after it has been selected & uploaded.

          Show
          Davo Smith added a comment - @Dan M - I'm not sure if pre-inspection of the files would work. Aside from being more difficult to code, it might produce a slightly more confusing UI, where you drag several files and then, at some random time later (depending on upload speed) boxes start popping up to ask what to do with them. With the current version, the decision box comes up immediately and then the files get on with uploading in the background. I think that dropping a zip file on a course and choosing 'SCORM', followed by getting an error later is not that dissimilar to adding a SCORM module and attempting to add an invalid zip file directly - most users aren't going to do it unless they have some expectation that they really have a SCORM file and if they are expecting it to be a SCORM file, but it isn't, then it would be more confusing to hide the 'treat as SCORM' option, than to give an error after it has been selected & uploaded.
          Hide
          Davo Smith added a comment -

          @Dan P
          1. I set out with the intention of writing this code in that way (shared base code between form and course drag and drop), but when it came down to it, aside from some very superficial similarities in structure, I found almost nothing that could be usefully abstracted. Comparing the two files side-by-side showed that every function had subtle differences, which suggested that it wasn't worth creating a shared base class to work from.

          2. Good suggestions - I'll get on and implement these.
          3. I had some discussion with Andrew at one point (but I can't remember which tracker issue it was on - I can't find it now); at the time he stated that there would need to be a couple of changes to my code (and a slight change to his as well), but nothing too major. I think that any changes to my code will be fairly localised and can be looked at when the course AJAX changes are complete.
          4. The icons in question are the 'ajax loading' spinner (which gets replaced by the real icon, once the upload is done) and the 'green plus' icon for adding items. I will switch to using classes for the sizing.
          5. I'll add a time component; however, looking again at the code, I don't think there'll ever be more than one dialog on screen at a time (I'd tried popping them all up at once, but that caused horrible problems, so I introduced a queue instead and only popped up one at a time).
          6. I'll add a quick note.
          7. The hardcoded default types are there as it is more of a problem if multiple modules define the same types, but give them different names or different messages. The functionality to add a new type is only really there for future-proofing, if a plugin author comes up with some wacky type that browsers handle and which they want to use. If there is nothing to handle a particular type, then it does not get passed on to the javascript - if you comment out the relevant function in mod/url/lib.php, then any URLs dropped onto a page are treated as text (the fallback type that is provided in most cases) and a page is created. If you also remove the handler in mod/page/lib.php, then (I think) that non-file drags are just ignored altogether.
          8. I'll look into the component_callback function and see if it meets my needs
          9. I'll prune out the dev-only error strings.

          Show
          Davo Smith added a comment - @Dan P 1. I set out with the intention of writing this code in that way (shared base code between form and course drag and drop), but when it came down to it, aside from some very superficial similarities in structure, I found almost nothing that could be usefully abstracted. Comparing the two files side-by-side showed that every function had subtle differences, which suggested that it wasn't worth creating a shared base class to work from. 2. Good suggestions - I'll get on and implement these. 3. I had some discussion with Andrew at one point (but I can't remember which tracker issue it was on - I can't find it now); at the time he stated that there would need to be a couple of changes to my code (and a slight change to his as well), but nothing too major. I think that any changes to my code will be fairly localised and can be looked at when the course AJAX changes are complete. 4. The icons in question are the 'ajax loading' spinner (which gets replaced by the real icon, once the upload is done) and the 'green plus' icon for adding items. I will switch to using classes for the sizing. 5. I'll add a time component; however, looking again at the code, I don't think there'll ever be more than one dialog on screen at a time (I'd tried popping them all up at once, but that caused horrible problems, so I introduced a queue instead and only popped up one at a time). 6. I'll add a quick note. 7. The hardcoded default types are there as it is more of a problem if multiple modules define the same types, but give them different names or different messages. The functionality to add a new type is only really there for future-proofing, if a plugin author comes up with some wacky type that browsers handle and which they want to use. If there is nothing to handle a particular type, then it does not get passed on to the javascript - if you comment out the relevant function in mod/url/lib.php, then any URLs dropped onto a page are treated as text (the fallback type that is provided in most cases) and a page is created. If you also remove the handler in mod/page/lib.php, then (I think) that non-file drags are just ignored altogether. 8. I'll look into the component_callback function and see if it meets my needs 9. I'll prune out the dev-only error strings.
          Hide
          Dan Poltawski added a comment -

          Re: 7. yep I think that makes sense now i've read more, sorry.

          dndupload_handler

          10. I think you should throw a moodle_exception rather than call print_error within the library functions (in practice they are both the same but it reads better).
          11. add_type_handler is needlessly looping around $this->types - you could use in_array() and this would be more efficient and easier to understand. Same for is_known_type, has_type_handler, has_file_handler and get_handled_file_types

          dndupload_processor

          12. I think you could just throw an exception rather than using send_error (catch it in lib/ajax/dndupload.php and format as it needs to be if required) - I think that would also allow you to do things like require_capability and require_sesskey (which throw exceptions)
          13. get_handler_function() - I think can use component_callback
          14. dislay_name_from_file Should probably use textlib::strpost and textlib::substr

          Show
          Dan Poltawski added a comment - Re: 7. yep I think that makes sense now i've read more, sorry. dndupload_handler 10. I think you should throw a moodle_exception rather than call print_error within the library functions (in practice they are both the same but it reads better). 11. add_type_handler is needlessly looping around $this->types - you could use in_array() and this would be more efficient and easier to understand. Same for is_known_type, has_type_handler, has_file_handler and get_handled_file_types dndupload_processor 12. I think you could just throw an exception rather than using send_error (catch it in lib/ajax/dndupload.php and format as it needs to be if required) - I think that would also allow you to do things like require_capability and require_sesskey (which throw exceptions) 13. get_handler_function() - I think can use component_callback 14. dislay_name_from_file Should probably use textlib::strpost and textlib::substr
          Hide
          Davo Smith added a comment -

          @Dan P: I've pushed an update to the same branch (I'll merge the commits together once we're ready for integration, but will keep them separate for clarity at the moment):

          1. not sure if this is possible
          2. done (css classes defined at top of javascript)
          3. will need to coordinate with Andrew
          4. sizes removed
          5. time added
          6. comment added
          7. agreed to leave
          8. plugin_callback used
          9. error strings removed
          10. print_errors replaced with moodle_exception / coding_exception, as appropriate
          11. I don't think in_array will work as I'm comparing with specific member of the object, not the whole object
          12. exceptions added + required_XX + MUST_EXIST
          13. plugin_callback used
          14. textlib used

          Show
          Davo Smith added a comment - @Dan P: I've pushed an update to the same branch (I'll merge the commits together once we're ready for integration, but will keep them separate for clarity at the moment): 1. not sure if this is possible 2. done (css classes defined at top of javascript) 3. will need to coordinate with Andrew 4. sizes removed 5. time added 6. comment added 7. agreed to leave 8. plugin_callback used 9. error strings removed 10. print_errors replaced with moodle_exception / coding_exception, as appropriate 11. I don't think in_array will work as I'm comparing with specific member of the object, not the whole object 12. exceptions added + required_XX + MUST_EXIST 13. plugin_callback used 14. textlib used
          Hide
          Davo Smith added a comment -

          Adding this as a note to self, but will need to cross-check the module creation against the new capabilities that Tim Hunt has added to 2.3 (mod/xxx:addinstance)

          Show
          Davo Smith added a comment - Adding this as a note to self, but will need to cross-check the module creation against the new capabilities that Tim Hunt has added to 2.3 (mod/xxx:addinstance)
          Hide
          Davo Smith added a comment -

          I've just rebased on top of the latest version master and force-pushed this back into github.

          I've confirmed that this fully respects the 'addinstance' capabilities (without any fixes needed in my code - apart from one small typo in one of the exceptions).

          Hopefully this is close to being ready for integration.

          Show
          Davo Smith added a comment - I've just rebased on top of the latest version master and force-pushed this back into github. I've confirmed that this fully respects the 'addinstance' capabilities (without any fixes needed in my code - apart from one small typo in one of the exceptions). Hopefully this is close to being ready for integration.
          Hide
          Davo Smith added a comment -

          I've been thinking a bit more about making this compatible with current / future course formats, and wondered if this was a sensible approach:

          • Create a callback into the format to allow the overriding of the section identifier classes (current assumptions: a section is an 'li' element with classes 'section' and 'main'; it contains a 'ul' element with class 'section', which itself contains 'li' elements to represent each activity)
          • Require that the course format inserts the hidden 'Add file here' element into each section, rather than using javascript to inject it later - 'course/lib.php: print_section()' would be an obvious place to do this; the currently injected HTML is '<li class="dndupload-preview activity resource modtype_resource dndupload-hidden"><div class="mod-indent"><icon src="[$OUTPUT->pix_url('t/addfile')]"> <span class="instancename">[get_string('addfilehere', 'core_dndupload')]</span></div></li>'
          • Add some way for the format to override the generation of the resource element?
          Show
          Davo Smith added a comment - I've been thinking a bit more about making this compatible with current / future course formats, and wondered if this was a sensible approach: Create a callback into the format to allow the overriding of the section identifier classes (current assumptions: a section is an 'li' element with classes 'section' and 'main'; it contains a 'ul' element with class 'section', which itself contains 'li' elements to represent each activity) Require that the course format inserts the hidden 'Add file here' element into each section, rather than using javascript to inject it later - 'course/lib.php: print_section()' would be an obvious place to do this; the currently injected HTML is '<li class="dndupload-preview activity resource modtype_resource dndupload-hidden"><div class="mod-indent"><icon src=" [$OUTPUT->pix_url('t/addfile')] "> <span class="instancename"> [get_string('addfilehere', 'core_dndupload')] </span></div></li>' Add some way for the format to override the generation of the resource element?
          Hide
          Dan Poltawski added a comment -

          Hmm on your last comment, I must confess I am not sure i'm qualified to give a good answer.

          And yep I think this is close to integration, I hope to look at it after i've finished wuth this weeks integration.

          thanks!
          dan

          Show
          Dan Poltawski added a comment - Hmm on your last comment, I must confess I am not sure i'm qualified to give a good answer. And yep I think this is close to integration, I hope to look at it after i've finished wuth this weeks integration. thanks! dan
          Hide
          Nadav Kavalerchik added a comment -

          Is it possible to add support for Folder upload?

          Chrome only (for now, as far as i can tell)

          <input type="file" id="file_input" webkitdirectory="" directory="">

          see more info:
          http://stackoverflow.com/questions/5826286/how-do-i-use-google-chrome-11s-upload-folder-feature-in-my-own-code

          Show
          Nadav Kavalerchik added a comment - Is it possible to add support for Folder upload? Chrome only (for now, as far as i can tell) <input type= "file" id= "file_input" webkitdirectory= "" directory=" "> see more info: http://stackoverflow.com/questions/5826286/how-do-i-use-google-chrome-11s-upload-folder-feature-in-my-own-code
          Hide
          Davo Smith added a comment -

          @Nadav - Without a lot of investigation, the link you have provided allows you to click on a file upload element on a page then select a folder to upload in its entirety. This is very different from the browser supporting the ability to drag and drop folders onto a page and then upload their contents. I've carefully looked through the data presented to the javascript when you attempt to drop a folder onto a course page (in Firefox and Chrome); you get the name of the folder, as if it was a normal file, but the file content is either (depending on the OS) 0 bytes long, or a multiple of 4096 bytes, but with all the data '0'. I dug right down into the Chrome source code, just to double-check, but no further useful information is sent anywhere near the javascript layer (certainly not the contents of the folder, nor, due to security considerations, is there any way to manually look inside the folder).

          (As an aside, it would be an interesting question, it it was actually possible, as to how to handle a folder uploaded along with other files - should the contents of the folder (and all sub-folders) be converted into individual file resources, should a 'folder' resource be created, with the files & sub-folders, or should a 'file' resource be created, with the folder structure stored within it).

          Show
          Davo Smith added a comment - @Nadav - Without a lot of investigation, the link you have provided allows you to click on a file upload element on a page then select a folder to upload in its entirety. This is very different from the browser supporting the ability to drag and drop folders onto a page and then upload their contents. I've carefully looked through the data presented to the javascript when you attempt to drop a folder onto a course page (in Firefox and Chrome); you get the name of the folder, as if it was a normal file, but the file content is either (depending on the OS) 0 bytes long, or a multiple of 4096 bytes, but with all the data '0'. I dug right down into the Chrome source code, just to double-check, but no further useful information is sent anywhere near the javascript layer (certainly not the contents of the folder, nor, due to security considerations, is there any way to manually look inside the folder). (As an aside, it would be an interesting question, it it was actually possible, as to how to handle a folder uploaded along with other files - should the contents of the folder (and all sub-folders) be converted into individual file resources, should a 'folder' resource be created, with the files & sub-folders, or should a 'file' resource be created, with the folder structure stored within it).
          Hide
          Nadav Kavalerchik added a comment -

          @Davo - Thanks you for the quick response! I guess it is all very new and maybe even "work in progress". btw, Did you look into the demo : http://html5-demos.appspot.com/static/html5storage/demos/upload_directory/index.html

          Show
          Nadav Kavalerchik added a comment - @Davo - Thanks you for the quick response! I guess it is all very new and maybe even "work in progress". btw, Did you look into the demo : http://html5-demos.appspot.com/static/html5storage/demos/upload_directory/index.html
          Hide
          Davo Smith added a comment -

          @Nadav thanks for that link - but it still doesn't help. You can drag and drop a folder onto the file element (or click on it and select a folder), but you can't drag and drop a folder onto an arbitrary place on the page.

          Show
          Davo Smith added a comment - @Nadav thanks for that link - but it still doesn't help. You can drag and drop a folder onto the file element (or click on it and select a folder), but you can't drag and drop a folder onto an arbitrary place on the page.
          Hide
          Nadav Kavalerchik added a comment -

          @Davo - Dragging a folder and dropping it over the "browse" element seems very useful. It is a minor compromise when considering the benefits. Don't you think? at least until this technology gets more into main stream and maybe gets "standardized" and adopted by other browsers too.

          btw, Does the Drag and Drop code supports "Copy and Pasting" of files?

          Show
          Nadav Kavalerchik added a comment - @Davo - Dragging a folder and dropping it over the "browse" element seems very useful. It is a minor compromise when considering the benefits. Don't you think? at least until this technology gets more into main stream and maybe gets "standardized" and adopted by other browsers too. btw, Does the Drag and Drop code supports "Copy and Pasting" of files?
          Hide
          Davo Smith added a comment -

          @Nadav Have you tried the course drag and drop demo site I linked above? I can't see any way that the demo you have listed would work with the course drag and drop as it works now. It might be useful as a completely separate feature, in which case, I suggest you open a new tracker issue and explain exactly how you think it would work.

          Copy and paste is not drag and drop. As far as I know, browsers don't support uploading of files from the clipboard when copying and pasting (but I could be wrong about that). In addition, it would be difficult to know which section you were uploading to (if you are pasting via the edit menu, or via the Ctrl+V shortcut).

          Show
          Davo Smith added a comment - @Nadav Have you tried the course drag and drop demo site I linked above? I can't see any way that the demo you have listed would work with the course drag and drop as it works now. It might be useful as a completely separate feature, in which case, I suggest you open a new tracker issue and explain exactly how you think it would work. Copy and paste is not drag and drop. As far as I know, browsers don't support uploading of files from the clipboard when copying and pasting (but I could be wrong about that). In addition, it would be difficult to know which section you were uploading to (if you are pasting via the edit menu, or via the Ctrl+V shortcut).
          Hide
          Nadav Kavalerchik added a comment -

          @Davo - Thanks looking forward to see your beautiful code in Moodle 2.3

          Show
          Nadav Kavalerchik added a comment - @Davo - Thanks looking forward to see your beautiful code in Moodle 2.3
          Hide
          Dan Poltawski added a comment -

          (Note before I forget it) lib/ajax/dndupload.php needs sesskey protection. A malicious user could potential craft a url which adds a file to a course page..

          Show
          Dan Poltawski added a comment - (Note before I forget it) lib/ajax/dndupload.php needs sesskey protection. A malicious user could potential craft a url which adds a file to a course page..
          Hide
          Davo Smith added a comment -

          lib/dnduploadlib.php - function process() (which is the first thing that lib/ajax/dndupload.php calls), 4th line is require_sesskey() (right after two 'require_capability' lines).

          Show
          Davo Smith added a comment - lib/dnduploadlib.php - function process() (which is the first thing that lib/ajax/dndupload.php calls), 4th line is require_sesskey() (right after two 'require_capability' lines).
          Hide
          Dan Poltawski added a comment -

          Hi Davo,

          Thanks for all the updates, apologies for my slow reponses. I've been having a big think about this today and discussed a few points with MD. I'll try and summarise:

          • A concern is accidentally dragging files onto the course page and then them being immediately visible to students. A suggestion to deal with this is to pop up a 'what do you want to name this' dialogue much like dragging a link does. The popup would be pre-populated with the filename. This adds a step, but this might actually be a really useful step as well as being protection from accidental uploads. What do you think?
          • I think that the work in MDL-31052 is going to do some cleanup that is going to impact this and I think we want to try and coordinate these two changes - i'll try and do this. Just to reassure you though (since I have been less responsive in here than I would like), we definitely want this in 2.3
          • Out of interest I tried adding dndupload support to the OU subpage module: (https://gist.github.com/2410790) I don't think it worked because of things relating to this ajax course editting stuff - again relating this with MDL-31052. It might be interesting exercise to try adding dndupload to submodule as I think it would help design this tobe tolerance to changes in course formats (e.g. passing in the elements we need in as arguments to the JS)
          • On course formats flexibility, in MD's recent mockup: http://docs.moodle.org/dev/File:paged_course_formats_sectionpage_mockup.png note the 'drag files here' element. which is always visible when on 'single section on one page' mode. That works there, but on multiple sections on one page we probably want that not to be there. We need to think about how we would elegeantly do this. Ideally the control would be in the calling code.
          • CAUTION: I might be over engineering this thought. I think it would be advantageous to move the return structure of the register callback as a php object like we do in lib/outputcomponents rather than a simple array. The reason I think this is because we'd then enforce the data structure with php syntax checking rather than dynamically.
          Show
          Dan Poltawski added a comment - Hi Davo, Thanks for all the updates, apologies for my slow reponses. I've been having a big think about this today and discussed a few points with MD. I'll try and summarise: A concern is accidentally dragging files onto the course page and then them being immediately visible to students. A suggestion to deal with this is to pop up a 'what do you want to name this' dialogue much like dragging a link does. The popup would be pre-populated with the filename. This adds a step, but this might actually be a really useful step as well as being protection from accidental uploads. What do you think? I think that the work in MDL-31052 is going to do some cleanup that is going to impact this and I think we want to try and coordinate these two changes - i'll try and do this. Just to reassure you though (since I have been less responsive in here than I would like), we definitely want this in 2.3 Out of interest I tried adding dndupload support to the OU subpage module: ( https://gist.github.com/2410790 ) I don't think it worked because of things relating to this ajax course editting stuff - again relating this with MDL-31052 . It might be interesting exercise to try adding dndupload to submodule as I think it would help design this tobe tolerance to changes in course formats (e.g. passing in the elements we need in as arguments to the JS) On course formats flexibility, in MD's recent mockup: http://docs.moodle.org/dev/File:paged_course_formats_sectionpage_mockup.png note the 'drag files here' element. which is always visible when on 'single section on one page' mode. That works there, but on multiple sections on one page we probably want that not to be there. We need to think about how we would elegeantly do this. Ideally the control would be in the calling code. CAUTION: I might be over engineering this thought. I think it would be advantageous to move the return structure of the register callback as a php object like we do in lib/outputcomponents rather than a simple array. The reason I think this is because we'd then enforce the data structure with php syntax checking rather than dynamically.
          Hide
          Martin Dougiamas added a comment -

          I'm combining this with other 2.3 changes in MDL-32476

          Show
          Martin Dougiamas added a comment - I'm combining this with other 2.3 changes in MDL-32476
          Hide
          Davo Smith added a comment -

          1. I'd be reluctant to add an extra step, as that takes away from the immediacy of being able to drag & drop the files and have them immediately available (plus it would be very annoying if you've just uploaded 5 files and then have to click through 'ok' 5 times). Maybe a better solution would be an 'undo' button/link that appears immediately after you have done the upload (and which disappears again after 10 seconds)? The undo option would delete all the files you have just uploaded (along with the activities / resources created from them). I'd reckon that the odds are pretty slim of a student ever refreshing the page, spotting a file they weren't supposed to see and downloading it in the time it takes the teacher to click 'undo'.

          2. I've posted in MDL-31052 to try and coordinate this

          3. I'll take a look at the subpage module

          4. I'd noticed the 'drag files here' element - do we want that always visible? If so, I could adjust the drag and drop code to allow for this (although, from my point of view, it would be easier if we could just have a static element on the page informing users of drag and drop, and then pop up the 'drop here' message when they drag files over the section box - even if there is only one section on the page. I think that would be more consistent with what happens when there are multiple sections on the page).

          5. The current structure looks like this (if a module wanted to do everything):
          ['files' => [['extension' => '.ext1', 'message' => 'Turn ext1 into a XXX'], ['extension' => '.ext2', 'message' => 'Turn ext2 into a XXX']],
          'addtypes' => [['priority' => 200, 'identifier' => 'foo', 'datatransfertypes' => ['text/foo', 'x-encoded/foo'], 'addmessage' => 'Add foo here', 'namemessage' => 'What do you want to call this foo?'],
          ['priority' => 100, 'identifier' => 'bar', 'datatransfertypes' => ['text/bar', 'x-encoded/bar'], 'addmessage' => 'Add bar here', 'namemessage' => 'What do you want to call this bar?']],
          'types' => [['identifier' => 'foo', 'message' => 'Create an XXX from this foo'], ['identifier' => 'text', 'message' => 'Create an XXX from this text'], ['identifier' => 'bar', 'message' => 'Create an XXX from this bar']]]
          I'm not quite sure how you'd encode that into a PHP object, rather than a series of arrays, without it getting very convoluted to create (but I'm happy to be corrected on that front).

          Show
          Davo Smith added a comment - 1. I'd be reluctant to add an extra step, as that takes away from the immediacy of being able to drag & drop the files and have them immediately available (plus it would be very annoying if you've just uploaded 5 files and then have to click through 'ok' 5 times). Maybe a better solution would be an 'undo' button/link that appears immediately after you have done the upload (and which disappears again after 10 seconds)? The undo option would delete all the files you have just uploaded (along with the activities / resources created from them). I'd reckon that the odds are pretty slim of a student ever refreshing the page, spotting a file they weren't supposed to see and downloading it in the time it takes the teacher to click 'undo'. 2. I've posted in MDL-31052 to try and coordinate this 3. I'll take a look at the subpage module 4. I'd noticed the 'drag files here' element - do we want that always visible? If so, I could adjust the drag and drop code to allow for this (although, from my point of view, it would be easier if we could just have a static element on the page informing users of drag and drop, and then pop up the 'drop here' message when they drag files over the section box - even if there is only one section on the page. I think that would be more consistent with what happens when there are multiple sections on the page). 5. The current structure looks like this (if a module wanted to do everything ): ['files' => [ ['extension' => '.ext1', 'message' => 'Turn ext1 into a XXX'] , ['extension' => '.ext2', 'message' => 'Turn ext2 into a XXX'] ], 'addtypes' => [['priority' => 200, 'identifier' => 'foo', 'datatransfertypes' => ['text/foo', 'x-encoded/foo'] , 'addmessage' => 'Add foo here', 'namemessage' => 'What do you want to call this foo?'], ['priority' => 100, 'identifier' => 'bar', 'datatransfertypes' => ['text/bar', 'x-encoded/bar'] , 'addmessage' => 'Add bar here', 'namemessage' => 'What do you want to call this bar?']], 'types' => [ ['identifier' => 'foo', 'message' => 'Create an XXX from this foo'] , ['identifier' => 'text', 'message' => 'Create an XXX from this text'] , ['identifier' => 'bar', 'message' => 'Create an XXX from this bar'] ]] I'm not quite sure how you'd encode that into a PHP object, rather than a series of arrays, without it getting very convoluted to create (but I'm happy to be corrected on that front).
          Hide
          Dan Poltawski added a comment -

          Here is the output of the codechecker.

          Note that line length warnings/ comments without punctuation are not too important, but great if you do have time to fix up.

          The incorrect spacing issues and missing phpdocs are more 'hard rules' for new code.

          Show
          Dan Poltawski added a comment - Here is the output of the codechecker. Note that line length warnings/ comments without punctuation are not too important, but great if you do have time to fix up. The incorrect spacing issues and missing phpdocs are more 'hard rules' for new code.
          Hide
          Martin Dougiamas added a comment -

          Re: 1) the other aspect was giving it a decent name, as file names tend not to be descriptive. The little immediate popup was one solution. But MDL-31215 is the other one.

          Show
          Martin Dougiamas added a comment - Re: 1) the other aspect was giving it a decent name, as file names tend not to be descriptive. The little immediate popup was one solution. But MDL-31215 is the other one.
          Hide
          Dan Poltawski added a comment -
          1. Lets leave it as it is for now then.
          2. Cool
          3. Thanks
          4. I think this is just about making the feature more visible/discoverable so do want it always visible, however as discussed lets focus on having it land as it is.
          5. I think it could be encapsulated (even just by enforcing those array keys), but lets leave it as it is.
            1. BTW. Whilst I was looking in that area, I noticed that the handle_other_upload does not check for the result of the plugin callback (btw with that function and handle_upload, I would return an empty value rather than a string in the case of not found).
          Show
          Dan Poltawski added a comment - Lets leave it as it is for now then. Cool Thanks I think this is just about making the feature more visible/discoverable so do want it always visible, however as discussed lets focus on having it land as it is. I think it could be encapsulated (even just by enforcing those array keys), but lets leave it as it is. BTW. Whilst I was looking in that area, I noticed that the handle_other_upload does not check for the result of the plugin callback (btw with that function and handle_upload, I would return an empty value rather than a string in the case of not found).
          Hide
          Davo Smith added a comment -

          5.1. I've copied the exception from the handle_file_upload function. I return the string for the non-existent function as I want to very clearly distinguish between the coding error of declaring support for drag and drop upload and then not providing a handler function, and the situation where the function is there but processing the uploaded data fails for some other reason.

          Show
          Davo Smith added a comment - 5.1. I've copied the exception from the handle_file_upload function. I return the string for the non-existent function as I want to very clearly distinguish between the coding error of declaring support for drag and drop upload and then not providing a handler function, and the situation where the function is there but processing the uploaded data fails for some other reason.
          Hide
          Davo Smith added a comment -

          I've cleaned up the code to fix the comment / whitespace / phpdocs issues (as well as that missing exception from the last comment on this issue).

          Also rebased onto master whilst I was at it.

          New code is there on github.

          Will look at the subpage issue now.

          Show
          Davo Smith added a comment - I've cleaned up the code to fix the comment / whitespace / phpdocs issues (as well as that missing exception from the last comment on this issue). Also rebased onto master whilst I was at it. New code is there on github. Will look at the subpage issue now.
          Hide
          Davo Smith added a comment -

          Just tried out the subpage module.
          It basically all worked, but with the following issues:

          • The non-standard 'AJAX' editing code meant that I had to disable my 'is ajax enabled' check and the non-ajax editing icons were left at the end of the upload.
          • The section number for the subpage (100) was higher than $course->numsections, so I had to disable that check on the back-end as well.

          I hope that the first of these issues could be fixed by the subpage module adopting the new course AJAX code (when finished). The second issue might just be a case of removing the 'numsections' check and hoping for the best (not sure what other solutions there are to this).

          Show
          Davo Smith added a comment - Just tried out the subpage module. It basically all worked, but with the following issues: The non-standard 'AJAX' editing code meant that I had to disable my 'is ajax enabled' check and the non-ajax editing icons were left at the end of the upload. The section number for the subpage (100) was higher than $course->numsections, so I had to disable that check on the back-end as well. I hope that the first of these issues could be fixed by the subpage module adopting the new course AJAX code (when finished). The second issue might just be a case of removing the 'numsections' check and hoping for the best (not sure what other solutions there are to this).
          Hide
          Dan Poltawski added a comment -

          Thanks, the numsections thing is a hack subpage does so not too bothered, I suggested it just again to see if we can tolerate course format changes.

          Show
          Dan Poltawski added a comment - Thanks, the numsections thing is a hack subpage does so not too bothered, I suggested it just again to see if we can tolerate course format changes.
          Hide
          Dan Poltawski added a comment -

          I've rebased this on top of the (in progress) changes in MDL-22504 and quickly modified it to work. (I wasn't very through with this so may have missed stuff off):

          https://github.com/danpoltawski/moodle/tree/dragdrop-upload-on-top-of-MDL-22504

          (Main changes: https://github.com/danpoltawski/moodle/commit/f52a8a3bc27e55ef3043ad8cb248c03a74645453 )

          Show
          Dan Poltawski added a comment - I've rebased this on top of the (in progress) changes in MDL-22504 and quickly modified it to work. (I wasn't very through with this so may have missed stuff off): https://github.com/danpoltawski/moodle/tree/dragdrop-upload-on-top-of-MDL-22504 (Main changes: https://github.com/danpoltawski/moodle/commit/f52a8a3bc27e55ef3043ad8cb248c03a74645453 )
          Hide
          Andrew Nicols added a comment -

          Hi Dan,

          Quick comment on your changes – to be on the safe side, it's probably worth wrapping the call to invoke_function in a YUI().use():

          add_editing: function(elementid) {
              YUI().use('moodle-course-coursebase', function(Y) {
                  M.course.coursebase.invoke_function('setup_for_resource', '#' + elementid);
              });
          }
          

          If, for some reason, the code is called when coursebase hasn't already been required elsewhere, then this should mean that it's included and doesn't throw an error. This could potentially happen if the drag/drop code is included when course ajax is disabled (e.g. by the theme, course format, user, or site admin).

          Show
          Andrew Nicols added a comment - Hi Dan, Quick comment on your changes – to be on the safe side, it's probably worth wrapping the call to invoke_function in a YUI().use(): add_editing: function(elementid) { YUI().use('moodle-course-coursebase', function(Y) { M.course.coursebase.invoke_function('setup_for_resource', '#' + elementid); }); } If, for some reason, the code is called when coursebase hasn't already been required elsewhere, then this should mean that it's included and doesn't throw an error. This could potentially happen if the drag/drop code is included when course ajax is disabled (e.g. by the theme, course format, user, or site admin).
          Hide
          Davo Smith added a comment -

          Please note, my github repo hasn't yet been rebased to squash all the commits down, nor does it include the changes from Dan / Andrew on the 20th April (which I was waiting on the integration of the new YUI course editing, at the end of last week, before adding).

          As it is now pretty late at night UK time, I'll make these changes first thing in the morning, so could we hold off on integration until then?

          Show
          Davo Smith added a comment - Please note, my github repo hasn't yet been rebased to squash all the commits down, nor does it include the changes from Dan / Andrew on the 20th April (which I was waiting on the integration of the new YUI course editing, at the end of last week, before adding). As it is now pretty late at night UK time, I'll make these changes first thing in the morning, so could we hold off on integration until then?
          Hide
          Davo Smith added a comment -

          The code is now compatible with the new YUI3 AJAX course editing.

          All commits have been squashed together.

          Unless I've missed something, this should be ready to integrate (but I'll be on-hand for the rest of the week for any urgent fixes).

          Show
          Davo Smith added a comment - The code is now compatible with the new YUI3 AJAX course editing. All commits have been squashed together. Unless I've missed something, this should be ready to integrate (but I'll be on-hand for the rest of the week for any urgent fixes).
          Hide
          Dan Poltawski added a comment -

          Fatal error: Call to undefined function dndupload_add_to_course() in /Users/danp/git/moodle/course/lib.php on line 4495

          Show
          Dan Poltawski added a comment - Fatal error: Call to undefined function dndupload_add_to_course() in /Users/danp/git/moodle/course/lib.php on line 4495
          Hide
          Dan Poltawski added a comment -

          That was on the front page Seems like dndlib needs to be included from course/lib

          Show
          Dan Poltawski added a comment - That was on the front page Seems like dndlib needs to be included from course/lib
          Hide
          Davo Smith added a comment -

          I've fixed the front page issue on my git branch (and squashed it into the single commit).

          Show
          Davo Smith added a comment - I've fixed the front page issue on my git branch (and squashed it into the single commit).
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've spend a good chunk of time looking this over.
          Noting I didn't even get as far as testing it, I was purely reading through and understanding the code.
          My appologies if I've misunderstood things, however I think there is enough to send this make to be further looked at.

          What I've noted:

          • theme/base/style/course.css: overflow:clip is invalid css.
          • dndupload_handler __construct. I think the code here could certainly be made more robust.
            1. Rather than use get_plugin_list('mod') consider using get_plugin_list_with_function, it will reduce the module list to just those that have the required callback for we begin iterating (this will tie in with my thoughts below).
            2. Don't use plugin_callback, its likely to be deprecated in favour of component_callback (and just calls component_callback). However I would recommend not using component_callback either. If you switch to get_plugin_list_with_function as suggested above then practically everything that component_callback for you would already be done, all you would have to do it call the function (you know it exists for every module returned by get_plugin_list_with_function).
              By doing this you would be cutting out unneeded component parsing (clean, and normalise), youd be avoiding component directory checks, and youd avoid duplicate function checks.
              Personally I don't think component_callback and plugin_callback are the best of functions. They serve their purpose in some ways, but they're not really a great choice unless your component is likely to be from a number of plugin types in which case calling get_plugin.. methods isn't a practical option.
            3. Perhaps too late in the stage now, but still worth considering I suppose is whether there would be advantage in flipping how that register callback works.
              Rather than calling the callback method and requiring it to produce a complex structure which you then need to parse again, why not just call the callback method and pass $this as the only argument. Then each callback method can call add_type/add_type_handler/add_file_handler as they need directly. Those methods are public, and the response structure is not used for anything but parsing to those functions so I'd think that would be a clear win. It would also make the callback MUCH easier to document as people would be able to refer to thie new dndupload API.
          • Small idea but within dndupload if you use the identifier as the key when adding types (given it has to be unique anyway) you can reduce the need to iterate the entire collection when looking for a known type, simplifying your is_known_type method as well as functions like add_type_handler. A very minor performance win.
          • Leading on from that I see that add_file_handler doesn't have a unique property, however by the looks of it module could be made unique given its used for locating specific handlers. Again if done you could make that the key to simplify those checks.
          • dndupload_processor::send_response why is it concatenating an empty string to the result of $mod->get_icon_url() (just curious is all).
          • Should dndupload_processor::process check that $content is null if is_file_upload was true.
          • Looking at dndupload_processor::handle_file_upload I think that the module should be checked for the handle callback well before the draft file area has been created, and certainly before the upload has been processed into it and a new module created. Perhaps that should be checked within process() before calling handle_file_upload.
          • dndupload_processor::finish_setup_course_module could probably be using delete_course_module to make that clean up more future proof.
          • I'm not sure about the new string file lang/en/dndupload.php, to be truthful I think those strings should be moved into an existing file and the new file removed. But I'm not 100% sure so we can check with Eloy/Petr/Martin at some point.
          • While I havn't tested to be sure the module callbacks should all start with mod_ I think. That is our perferred way of adding them to modules I believe to avoid any further chance of collisions not that there is much. Debugging doesn't get shown with AJAX requests obviously so we'd have to check system logs I think.
          • Ideally I think modulename_dndupload_handle should be using modulename_add_instance rather than manually inserting the record. While our core modules are all reasonably simple this will help reduce duplication for those who are doing more with their modules when adding an instance and this ensures things won't be inadverningly missed. Also when cleaning up modulename_delete_instance should be used to clean up anything additional as well. Where possible for both of these anyway, it helps reduce duplication and provides a good example to other developers.
          • Just a note in regards to the javascript Y.Node.create('<div class="someclass">Blah</div>'); could be used to simply much of the node creation code there.

          There is one larger issue that I have noted as well.
          The naming of class and file names and the location of files should reflect what this code is doing and its scope, presently I'm not sure about the naming and location of things. It all looks very clean but I wonder whether things could be moved and renamed to be made clearer.
          dnduploadlib.php contains two classes and a function, the function dndupload_add_to_course is only used within course/lib.php, dndupload_processor is only used within lib/ajax/dndupload.php, and finally dndupload_handler is used internally by dndupload_processor, and by dndupload_add_to_course. Essentially this code is being used for viewing courses, and for AJAX calls to make things happen when viewing a course.
          Because of this I wonder whether this code should be moved from the lib directory to the course directory as thats where its implementation would suggest it be located.
          I also think dndupload_processor needs to be renamed to reflect that it can only be used for AJAX callbacks. This is of course governed by its only real public function process leading eventually to send_response which returns JSON.
          I think that alone is a very strong reason to rename this class to reflect its limited use case.
          Also just noting so that others are aware I've looked I'm always very dubious of any class that in its process calls require_login and handles basic page validation, everything appears to be fine here, the class is essentially designed specifically to handle an AJAX page request.
          As well as the change in naming I think including a check of AJAX_SCRIPT within the constructor may be wise and help to ensure that the correct output initialisation has occured.
          Actually that leads onto another point, before echoing out the JSON response $OUTPUT->header should be used to ensure that the correct headers are returned for the response (further enforcing the AJAX requirement I suppose).
          Anyway open to thoughts there.

          Cheers all
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've spend a good chunk of time looking this over. Noting I didn't even get as far as testing it, I was purely reading through and understanding the code. My appologies if I've misunderstood things, however I think there is enough to send this make to be further looked at. What I've noted: theme/base/style/course.css: overflow:clip is invalid css. dndupload_handler __construct. I think the code here could certainly be made more robust. Rather than use get_plugin_list('mod') consider using get_plugin_list_with_function, it will reduce the module list to just those that have the required callback for we begin iterating (this will tie in with my thoughts below). Don't use plugin_callback, its likely to be deprecated in favour of component_callback (and just calls component_callback). However I would recommend not using component_callback either. If you switch to get_plugin_list_with_function as suggested above then practically everything that component_callback for you would already be done, all you would have to do it call the function (you know it exists for every module returned by get_plugin_list_with_function). By doing this you would be cutting out unneeded component parsing (clean, and normalise), youd be avoiding component directory checks, and youd avoid duplicate function checks. Personally I don't think component_callback and plugin_callback are the best of functions. They serve their purpose in some ways, but they're not really a great choice unless your component is likely to be from a number of plugin types in which case calling get_plugin.. methods isn't a practical option. Perhaps too late in the stage now, but still worth considering I suppose is whether there would be advantage in flipping how that register callback works. Rather than calling the callback method and requiring it to produce a complex structure which you then need to parse again, why not just call the callback method and pass $this as the only argument. Then each callback method can call add_type/add_type_handler/add_file_handler as they need directly. Those methods are public, and the response structure is not used for anything but parsing to those functions so I'd think that would be a clear win. It would also make the callback MUCH easier to document as people would be able to refer to thie new dndupload API. Small idea but within dndupload if you use the identifier as the key when adding types (given it has to be unique anyway) you can reduce the need to iterate the entire collection when looking for a known type, simplifying your is_known_type method as well as functions like add_type_handler. A very minor performance win. Leading on from that I see that add_file_handler doesn't have a unique property, however by the looks of it module could be made unique given its used for locating specific handlers. Again if done you could make that the key to simplify those checks. dndupload_processor::send_response why is it concatenating an empty string to the result of $mod->get_icon_url() (just curious is all). Should dndupload_processor::process check that $content is null if is_file_upload was true. Looking at dndupload_processor::handle_file_upload I think that the module should be checked for the handle callback well before the draft file area has been created, and certainly before the upload has been processed into it and a new module created. Perhaps that should be checked within process() before calling handle_file_upload. dndupload_processor::finish_setup_course_module could probably be using delete_course_module to make that clean up more future proof. I'm not sure about the new string file lang/en/dndupload.php, to be truthful I think those strings should be moved into an existing file and the new file removed. But I'm not 100% sure so we can check with Eloy/Petr/Martin at some point. While I havn't tested to be sure the module callbacks should all start with mod_ I think. That is our perferred way of adding them to modules I believe to avoid any further chance of collisions not that there is much. Debugging doesn't get shown with AJAX requests obviously so we'd have to check system logs I think. Ideally I think modulename_dndupload_handle should be using modulename_add_instance rather than manually inserting the record. While our core modules are all reasonably simple this will help reduce duplication for those who are doing more with their modules when adding an instance and this ensures things won't be inadverningly missed. Also when cleaning up modulename_delete_instance should be used to clean up anything additional as well. Where possible for both of these anyway, it helps reduce duplication and provides a good example to other developers. Just a note in regards to the javascript Y.Node.create('<div class="someclass">Blah</div>'); could be used to simply much of the node creation code there. There is one larger issue that I have noted as well. The naming of class and file names and the location of files should reflect what this code is doing and its scope, presently I'm not sure about the naming and location of things. It all looks very clean but I wonder whether things could be moved and renamed to be made clearer. dnduploadlib.php contains two classes and a function, the function dndupload_add_to_course is only used within course/lib.php, dndupload_processor is only used within lib/ajax/dndupload.php, and finally dndupload_handler is used internally by dndupload_processor, and by dndupload_add_to_course. Essentially this code is being used for viewing courses, and for AJAX calls to make things happen when viewing a course. Because of this I wonder whether this code should be moved from the lib directory to the course directory as thats where its implementation would suggest it be located. I also think dndupload_processor needs to be renamed to reflect that it can only be used for AJAX callbacks. This is of course governed by its only real public function process leading eventually to send_response which returns JSON. I think that alone is a very strong reason to rename this class to reflect its limited use case. Also just noting so that others are aware I've looked I'm always very dubious of any class that in its process calls require_login and handles basic page validation, everything appears to be fine here, the class is essentially designed specifically to handle an AJAX page request. As well as the change in naming I think including a check of AJAX_SCRIPT within the constructor may be wise and help to ensure that the correct output initialisation has occured. Actually that leads onto another point, before echoing out the JSON response $OUTPUT->header should be used to ensure that the correct headers are returned for the response (further enforcing the AJAX requirement I suppose). Anyway open to thoughts there. Cheers all Sam
          Hide
          Davo Smith added a comment -

          I've added a new commit to my repo. Please could this be checked, before I squash the commits?

          • overflow:clip changed to overflow:hidden
          • dndupload_handler __construct:
            1. switched to get_plugin_list_with_function (I didn't know about this function, otherwise I'd have used it in the first place!)
            2. done
            3. That was my original design, Dan P suggested otherwise here: http://docs.moodle.org/dev/Talk:Course_drag_and_drop_upload
          • The types array is now indexed by the identifier and the relevant functions have been simplified
          • I've thought through doing something similar with the file handlers array, but I'm holding back as this makes the javascript more difficult (looping through associative arrays in javascript can pull in extra member variables added by the JS framework, if you're not careful - leaving the code as it is allows a simple 0..length loop in the javascript).
          • The empty string concatenation was there to convert the moodle_url object into a string - but looking back at it again, I'm not sure why I didn't just call '->out()' instead (probably an innate fear of chaining function calls). Fixed.
          • Check added in case a file upload ever contains a 'content' parameter
          • I've added a check to 'handle_other_upload' to ensure the module has registered that it supports this type of content. 'handle_file_upload' will already fail when creating the draft area if the module is not registered to handle the file type being uploaded (or if it is not registered to handle any file types). If the module has declared support for handling file types (via mod_modname_dndupload_register), but does not include a 'mod_modname_dndupload_handle' function, then that is a coding error on behalf of the module creator. I'm not convinced that the extra effort to check for the existence of the function earlier in the processing of the upload is worth it for something that will get caught very quickly during the development process (especially as there only seems to be a built-in Moodle function for calling a callback, not one for separately checking it's existence first).
          • switched to 'delete_course_module'
          • lang file - awaiting further clarification
          • renamed all callbacks to start with 'mod_' (although that seems to contradict every other function in the mod/XX/lib.php files)
          • Switched all mod_XX_dndupload_handle functions to call XX_add_instance
          • Left the node creation code as it is, as, in many cases, I later update individual elements, based on the AJAX response; the current way leaves references to each of those elements, without having to re-parse the generated HTML.

          I've renamed class dndupload_processor => dndupload_ajax_processor, but not moved any of the files (to try and make sure my other changes are still visible in the diff). I will add an extra commit to move the files, if you would prefer that.
          Extra check for defined('AJAX_SCRIPT') added to the constructor for this class.

          echo $OUTPUT->header() added to send_response function.

          Show
          Davo Smith added a comment - I've added a new commit to my repo. Please could this be checked, before I squash the commits? overflow:clip changed to overflow:hidden dndupload_handler __construct: 1. switched to get_plugin_list_with_function (I didn't know about this function, otherwise I'd have used it in the first place!) 2. done 3. That was my original design, Dan P suggested otherwise here: http://docs.moodle.org/dev/Talk:Course_drag_and_drop_upload The types array is now indexed by the identifier and the relevant functions have been simplified I've thought through doing something similar with the file handlers array, but I'm holding back as this makes the javascript more difficult (looping through associative arrays in javascript can pull in extra member variables added by the JS framework, if you're not careful - leaving the code as it is allows a simple 0..length loop in the javascript). The empty string concatenation was there to convert the moodle_url object into a string - but looking back at it again, I'm not sure why I didn't just call '->out()' instead (probably an innate fear of chaining function calls). Fixed. Check added in case a file upload ever contains a 'content' parameter I've added a check to 'handle_other_upload' to ensure the module has registered that it supports this type of content. 'handle_file_upload' will already fail when creating the draft area if the module is not registered to handle the file type being uploaded (or if it is not registered to handle any file types). If the module has declared support for handling file types (via mod_modname_dndupload_register), but does not include a 'mod_modname_dndupload_handle' function, then that is a coding error on behalf of the module creator. I'm not convinced that the extra effort to check for the existence of the function earlier in the processing of the upload is worth it for something that will get caught very quickly during the development process (especially as there only seems to be a built-in Moodle function for calling a callback, not one for separately checking it's existence first). switched to 'delete_course_module' lang file - awaiting further clarification renamed all callbacks to start with 'mod_' (although that seems to contradict every other function in the mod/XX/lib.php files) Switched all mod_XX_dndupload_handle functions to call XX_add_instance Left the node creation code as it is, as, in many cases, I later update individual elements, based on the AJAX response; the current way leaves references to each of those elements, without having to re-parse the generated HTML. I've renamed class dndupload_processor => dndupload_ajax_processor, but not moved any of the files (to try and make sure my other changes are still visible in the diff). I will add an extra commit to move the files, if you would prefer that. Extra check for defined('AJAX_SCRIPT') added to the constructor for this class. echo $OUTPUT->header() added to send_response function.
          Hide
          Dan Poltawski added a comment -

          I'm pushing this through for integration despite not having a chance to peer review it for Davo. Sorry..

          Show
          Dan Poltawski added a comment - I'm pushing this through for integration despite not having a chance to peer review it for Davo. Sorry..
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Davo,

          I have to get going for the night. I've made good progress into the review of these changes and have made a few notes which I will share now.
          I'll continue this review tomorrow morning however feel free to address any of these points already now but please ping me if you do so that I know to get the new changes.
          Otherwise more feedback tomorrow.
          Just before I get to my points I do want to make something clear. This is one awesome feature set, and I can see some very cool directions that this could be taken in the future. But for now before it lands I really think we need to polish this as much as we can before it lands because it is a feature that is going to be touted I'm sure and certainly it is a feature that people are going to be using (can you imagine anything easier than "just drag your file over the section you want it in and Moodle will do the rest").
          So my notes so far:

          In regards to the interface I really don't like the text that reads 'Add file(s)|link|page here'.
          At first I thought it was a link or a button of some kind which says something (lol perhaps just about me).
          However I think it is rather confusing/annoying that it is always there.
          I really think that we would be much better having something that was displayed when the user was dragging something over either the section or over the course.
          Perhaps all we need to do is make the note at the top of the editing page more prominent (its styling needs improving btw) although personally I think we do need a visible drop target in the section, and I think it should only be shown when dragging something over the page (highlighted further when the user is over top of the actual target/section perhaps in some way). Anyway perhaps get MD's thoughts on this, he can make an executive decision to keep this moving at pace.

          Also to be mentioned I really do think we need to move files and lang strings for this change as I suggested above. I will discuss this with others when I get a chance.

          A couple of suggestions, perhaps for down the track:

          1. Each module that supports a drag+drop creation could have a setting to enable/disable it. That was installations can control it a little more.
          2. There should probably be a setting to enable drag/drop creation of activities across the board.

          Bugs:

          1. If you have a section that contains orphaned activities (create an activity in the last section and then reduce the number of sections by 1) you still see the add files here text and if you try to drag them over you get an error about invalid section.
          2. Things arn't very in tune if you disable file, url, and page modules and then head to a course. You still see the text saying you can add content and you can still drag things over. When you do things just keep spinning. Likely just needing a check somewhere before calling module callbacks to make sure they are enabled right?
          3. When addinstance cap has been removed for editing user on all activities the add file(s) here link is still shown despite them not actually being able to do it. (note it doesn't allow them to create new instances which is good)

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Davo, I have to get going for the night. I've made good progress into the review of these changes and have made a few notes which I will share now. I'll continue this review tomorrow morning however feel free to address any of these points already now but please ping me if you do so that I know to get the new changes. Otherwise more feedback tomorrow. Just before I get to my points I do want to make something clear. This is one awesome feature set, and I can see some very cool directions that this could be taken in the future. But for now before it lands I really think we need to polish this as much as we can before it lands because it is a feature that is going to be touted I'm sure and certainly it is a feature that people are going to be using (can you imagine anything easier than "just drag your file over the section you want it in and Moodle will do the rest"). So my notes so far: In regards to the interface I really don't like the text that reads 'Add file(s)|link|page here'. At first I thought it was a link or a button of some kind which says something (lol perhaps just about me). However I think it is rather confusing/annoying that it is always there. I really think that we would be much better having something that was displayed when the user was dragging something over either the section or over the course. Perhaps all we need to do is make the note at the top of the editing page more prominent (its styling needs improving btw) although personally I think we do need a visible drop target in the section, and I think it should only be shown when dragging something over the page (highlighted further when the user is over top of the actual target/section perhaps in some way). Anyway perhaps get MD's thoughts on this, he can make an executive decision to keep this moving at pace. Also to be mentioned I really do think we need to move files and lang strings for this change as I suggested above. I will discuss this with others when I get a chance. A couple of suggestions, perhaps for down the track: Each module that supports a drag+drop creation could have a setting to enable/disable it. That was installations can control it a little more. There should probably be a setting to enable drag/drop creation of activities across the board. Bugs: If you have a section that contains orphaned activities (create an activity in the last section and then reduce the number of sections by 1) you still see the add files here text and if you try to drag them over you get an error about invalid section. Things arn't very in tune if you disable file, url, and page modules and then head to a course. You still see the text saying you can add content and you can still drag things over. When you do things just keep spinning. Likely just needing a check somewhere before calling module callbacks to make sure they are enabled right? When addinstance cap has been removed for editing user on all activities the add file(s) here link is still shown despite them not actually being able to do it. (note it doesn't allow them to create new instances which is good) Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          IMO this feature absolutely must land in 2.3. Just going to have another look at the latest interface to follow up on the interface things Sam raised.

          Show
          Martin Dougiamas added a comment - IMO this feature absolutely must land in 2.3. Just going to have another look at the latest interface to follow up on the interface things Sam raised.
          Hide
          Martin Dougiamas added a comment - - edited

          Sam I think you may have had some issue in the theme you used or bad cache or something. I used the latest code and the standard theme and the "Add files..." thing in each section only shows when you are over it. It works rather well.

          I'm not a huge fan of the yellow message at the top, but I recognise the difficult choices here.

          Here's my suggestion for something that promotes discoverability but would look slick.

          a) Remove that yellow message from the top.
          b) When we first switch to editing mode, show all these drop zones for a few seconds, for all sections, with the text "Drag and drop files, text or links here to upload them", and then fade them out.
          c) When a user is dragging something over a section we show a message there (as now) of:

          • Drop file(s) here to add them
          • Drop text here to add it
          • Drop link here to add it

          Sam's 3-bug list looks valid and needs fixing.

          Show
          Martin Dougiamas added a comment - - edited Sam I think you may have had some issue in the theme you used or bad cache or something. I used the latest code and the standard theme and the "Add files..." thing in each section only shows when you are over it. It works rather well. I'm not a huge fan of the yellow message at the top, but I recognise the difficult choices here. Here's my suggestion for something that promotes discoverability but would look slick. a) Remove that yellow message from the top. b) When we first switch to editing mode, show all these drop zones for a few seconds, for all sections, with the text "Drag and drop files, text or links here to upload them", and then fade them out. c) When a user is dragging something over a section we show a message there (as now) of: Drop file(s) here to add them Drop text here to add it Drop link here to add it Sam's 3-bug list looks valid and needs fixing.
          Hide
          Martin Dougiamas added a comment - - edited

          After some discussion I'm also happy to keep the yellow box as-is for the time being until more people get to test it in 2.3 beta and we might get some more suggestions.

          Show
          Martin Dougiamas added a comment - - edited After some discussion I'm also happy to keep the yellow box as-is for the time being until more people get to test it in 2.3 beta and we might get some more suggestions.
          Hide
          Davo Smith added a comment - - edited

          To echo what Martin has said, the 'add' messages should only appear to when something is being dragged. I sometimes see them appearing in the way you describe if I've switched branches and not cleared my browser / Moodle cache.

          Being able to enable / disable drag and drop for each module sounds like a reasonable idea, but maybe not essential right away. I'm not sure that being able to disable drag and drop upload across the board has any real benefit (which is why the setting, which was in earlier versions of the code, has been removed); if someone does not want it, they can just not drag things into their browser Window - it does not get in the way of any other functionality.

          1. Should it prevent dragging onto orphaned sections, or should I remove the rule about adding to 'invalid' section numbers? (This rule was also causing problems with the subpage module).
          2 / 3. I'll add a check that the relevant modules are enabled and the user has the 'addinstance' capability, before adding the modules to the 'handlers' list; I'll also not enable drag and drop at all, if the handlers list is empty.

          a) I had hoped that a designer could look at the yellow message and make it look a bit nicer - maybe having it appear when you switch editing on and then fade out would be better?
          b)/c) Changing the wording of the drop zones would be a pity, as it would lose the reminder of the type of data you are dragging (file / text / url) - maybe a compromise would be to use 'Drag and drop files, text or links here to upload them' as the initial wording, then replace with the 'Add file | text | link' here message when something is dragged? With that in place, maybe fading-out text in each section would work.

          (Update: Martin has edited his message whilst I was posting, in case my message does not quite match with what is written above)

          Show
          Davo Smith added a comment - - edited To echo what Martin has said, the 'add' messages should only appear to when something is being dragged. I sometimes see them appearing in the way you describe if I've switched branches and not cleared my browser / Moodle cache. Being able to enable / disable drag and drop for each module sounds like a reasonable idea, but maybe not essential right away. I'm not sure that being able to disable drag and drop upload across the board has any real benefit (which is why the setting, which was in earlier versions of the code, has been removed); if someone does not want it, they can just not drag things into their browser Window - it does not get in the way of any other functionality. 1. Should it prevent dragging onto orphaned sections, or should I remove the rule about adding to 'invalid' section numbers? (This rule was also causing problems with the subpage module). 2 / 3. I'll add a check that the relevant modules are enabled and the user has the 'addinstance' capability, before adding the modules to the 'handlers' list; I'll also not enable drag and drop at all, if the handlers list is empty. a) I had hoped that a designer could look at the yellow message and make it look a bit nicer - maybe having it appear when you switch editing on and then fade out would be better? b)/c) Changing the wording of the drop zones would be a pity, as it would lose the reminder of the type of data you are dragging (file / text / url) - maybe a compromise would be to use 'Drag and drop files, text or links here to upload them' as the initial wording, then replace with the 'Add file | text | link' here message when something is dragged? With that in place, maybe fading-out text in each section would work. (Update: Martin has edited his message whilst I was posting, in case my message does not quite match with what is written above)
          Hide
          Davo Smith added a comment -

          I've pushed an extra commit into my repo, that does the following:

          1. Checks that the module is visible/enabled, before adding it as an available handler
          2. Ignores any files that are dragged, if there are no 'file' handlers available (unfortunately, I can only filter by file type once the file has been dropped, as browser security prevents me from finding out the file type before then).
          3. Completely disable drag and drop if there are no available handlers
          4. Allow any positive 'section number' (as this check seemed to be causing more problems than it solved).

          Just to reiterate, if there is a designer available who could spend a few minutes tidying up the CSS for the 'Drag files ...' yellow message, then that would be great.

          Show
          Davo Smith added a comment - I've pushed an extra commit into my repo, that does the following: 1. Checks that the module is visible/enabled, before adding it as an available handler 2. Ignores any files that are dragged, if there are no 'file' handlers available (unfortunately, I can only filter by file type once the file has been dropped, as browser security prevents me from finding out the file type before then). 3. Completely disable drag and drop if there are no available handlers 4. Allow any positive 'section number' (as this check seemed to be causing more problems than it solved). Just to reiterate, if there is a designer available who could spend a few minutes tidying up the CSS for the 'Drag files ...' yellow message, then that would be great.
          Hide
          Martin Dougiamas added a comment -

          Handballing back to Sam

          Show
          Martin Dougiamas added a comment - Handballing back to Sam
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Its very likely by the sounds it was my cache sorry about that.
          I'm about to start looking at this again now, thanks for clarifying things, fixing the noted bugs, and making a decision on interfaces.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Its very likely by the sounds it was my cache sorry about that. I'm about to start looking at this again now, thanks for clarifying things, fixing the noted bugs, and making a decision on interfaces. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Alrighty, well I've looked this over and I'm now happy with that.

          I've discussed the files and lang strings with Eloy and we are both in agreement that given this is specifically applicable to a courses and not a global dnd solution the lib file should be moved to a course directory and the strings to where ever the course ajax strings are coming from.
          If you like I can do this as part of integration, otherwise please ping me when they are moved so that this can be integrated.

          One more thing although I am quite happy for it to happen after, I think it would be swell if the dndupload.js could be converted to a JS module given all of the rest of the course ajax stuff has now been converted. Of course things work now so perhaps best to do this later.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Alrighty, well I've looked this over and I'm now happy with that. I've discussed the files and lang strings with Eloy and we are both in agreement that given this is specifically applicable to a courses and not a global dnd solution the lib file should be moved to a course directory and the strings to where ever the course ajax strings are coming from. If you like I can do this as part of integration, otherwise please ping me when they are moved so that this can be integrated. One more thing although I am quite happy for it to happen after, I think it would be swell if the dndupload.js could be converted to a JS module given all of the rest of the course ajax stuff has now been converted. Of course things work now so perhaps best to do this later. Cheers Sam
          Hide
          Dan Poltawski added a comment -

          Just a note that I don't think course strings are in a sensibel place at all at the moment (they are in moodle langfile I think)

          Show
          Dan Poltawski added a comment - Just a note that I don't think course strings are in a sensibel place at all at the moment (they are in moodle langfile I think)
          Hide
          Davo Smith added a comment -

          Moving the files and strings now - should be done in a few minutes.
          Should I squash all the commits once done?

          Show
          Davo Smith added a comment - Moving the files and strings now - should be done in a few minutes. Should I squash all the commits once done?
          Hide
          Davo Smith added a comment -

          All files renamed, as an extra commit on my branch.

          For a rebased and 'squashed' version, please use the branch 'MDL-22504_drag_and_drop_upload_final2'

          Show
          Davo Smith added a comment - All files renamed, as an extra commit on my branch. For a rebased and 'squashed' version, please use the branch ' MDL-22504 _drag_and_drop_upload_final2'
          Hide
          Sam Hemelryk added a comment -

          Alrighty this has been integrated, congratulations.

          During integration I did test this in Chrome and Firefox against instructions I've written. Certainly deserves independent testing again as well.
          Could you please read over the testing instructions Davo and add anything I've missed.

          Also noting I did make an additional commit (a0a06d0) to fix up a couple of minor bugs I found during my testing:

          • Wrong require_once for dndupload.php
          • Incorrect lang strings for upload and cancel buttons when selecting how to handle a file
          • Muted a JS error I was getting if Firefox

          I will create a couple more issues now and link them here.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Alrighty this has been integrated, congratulations. During integration I did test this in Chrome and Firefox against instructions I've written. Certainly deserves independent testing again as well. Could you please read over the testing instructions Davo and add anything I've missed. Also noting I did make an additional commit (a0a06d0) to fix up a couple of minor bugs I found during my testing: Wrong require_once for dndupload.php Incorrect lang strings for upload and cancel buttons when selecting how to handle a file Muted a JS error I was getting if Firefox I will create a couple more issues now and link them here. Cheers Sam
          Hide
          Sam Hemelryk added a comment - - edited

          Linking issue MDL-33054 to bug when dragging URLs in Firefox. Cross broswer compatability needs testing.

          Show
          Sam Hemelryk added a comment - - edited Linking issue MDL-33054 to bug when dragging URLs in Firefox. Cross broswer compatability needs testing.
          Hide
          Sam Hemelryk added a comment -

          Linked issue MDL-33056 to move dndupload-status bar to avoid regressions in themes.

          Show
          Sam Hemelryk added a comment - Linked issue MDL-33056 to move dndupload-status bar to avoid regressions in themes.
          Hide
          Sam Hemelryk added a comment -

          Linked to MDL-33057 to convert dndupload.js into a YUI module like the rest of the course AJAX JS

          Show
          Sam Hemelryk added a comment - Linked to MDL-33057 to convert dndupload.js into a YUI module like the rest of the course AJAX JS
          Hide
          Dan Poltawski added a comment -

          I've noticed that drag/drop and 'click to edit title aren't collaborating that well (seems like the course ajax thing isn't picking it up, creating a bug for andrew).

          Show
          Dan Poltawski added a comment - I've noticed that drag/drop and 'click to edit title aren't collaborating that well (seems like the course ajax thing isn't picking it up, creating a bug for andrew).
          Hide
          Dan Poltawski added a comment -

          Created MDL-22504 for that

          Show
          Dan Poltawski added a comment - Created MDL-22504 for that
          Hide
          Frédéric Massart added a comment - - edited

          Hi guys, while testing I noticed these:

          On Chromium 18.0

          • Drag & drop of plain text or formatted text from a Firefox window causes a JS error because of a notice present in the JSON. Notice: Undefined property: stdClass::$content in /home/fred/www/repositories/testing_master/moodle/mod/page/lib.php on line 501. This works from Chromium to Chromium.
          • Drag & drop of URL from Firefox (toolbar, textfield or search field) creates a page instead of a link activity
          • The sentence 'Drag and drop files, text or links onto course sections to upload them' appears regardless of disabled activities/repositories (except if all)
          • The icon for a MBZ file is 32x32 pixels instead of 16x16

          Is it expected?

          • The MBZ file acts as a normal file and is downloaded

          On Firefox 11.0

          • Drag & drop of text or URLs does not work. The notice shown is 'Add files(s) here'. When dropping the text nothing happens, nothing in JS console.
          • The sentence 'Drag and drop files, text or links onto course sections to upload them' appears regardless of disabled activities/repositories (except if all)

          Is it expected?

          • The MBZ file acts as a normal file and is downloaded

          On IE9

          • All good as in nothing works but nothing shows that it is supposed to work.

          Edit: The notice comment applies to Firefox too

          Show
          Frédéric Massart added a comment - - edited Hi guys, while testing I noticed these: On Chromium 18.0 Drag & drop of plain text or formatted text from a Firefox window causes a JS error because of a notice present in the JSON. Notice: Undefined property: stdClass::$content in /home/fred/www/repositories/testing_master/moodle/mod/page/lib.php on line 501. This works from Chromium to Chromium. Drag & drop of URL from Firefox (toolbar, textfield or search field) creates a page instead of a link activity The sentence 'Drag and drop files, text or links onto course sections to upload them' appears regardless of disabled activities/repositories (except if all) The icon for a MBZ file is 32x32 pixels instead of 16x16 Is it expected? The MBZ file acts as a normal file and is downloaded On Firefox 11.0 Drag & drop of text or URLs does not work. The notice shown is 'Add files(s) here'. When dropping the text nothing happens, nothing in JS console. The sentence 'Drag and drop files, text or links onto course sections to upload them' appears regardless of disabled activities/repositories (except if all) Is it expected? The MBZ file acts as a normal file and is downloaded On IE9 All good as in nothing works but nothing shows that it is supposed to work. Edit: The notice comment applies to Firefox too
          Hide
          Davo Smith added a comment -

          I'll try and address these issues this afternoon. Some of them may be difficult or impossible to solve as I am dependent on the browser reporting the right content type so I can process it (certainly during the drag I cannot access the actual content to double-check the type, but I may be able to do something once the drop has happened).

          To note, there is (currently) no special handling of mbz files, so the observed behaviour is expected..

          Show
          Davo Smith added a comment - I'll try and address these issues this afternoon. Some of them may be difficult or impossible to solve as I am dependent on the browser reporting the right content type so I can process it (certainly during the drag I cannot access the actual content to double-check the type, but I may be able to do something once the drop has happened). To note, there is (currently) no special handling of mbz files, so the observed behaviour is expected..
          Hide
          Dan Poltawski added a comment -

          Davo: your comment concerns me - if we can't accurately tell what the content type is in all browsers - then perhaps it is not wise to support difference content types at all?

          Show
          Dan Poltawski added a comment - Davo: your comment concerns me - if we can't accurately tell what the content type is in all browsers - then perhaps it is not wise to support difference content types at all?
          Hide
          Davo Smith added a comment -

          I've added a new branch called 'MDL-22504_drag_and_drop_upload_integration' that addresses as many of the issues as I've been able to fix:

          • Text dragged FF => Chrom(ium|e) - looks like this was providing the text with the unicode bytes reversed. I've fixed this by sending off both the raw contents and a version with each pair of bytes reversed to the server. By experimentation (tested with a range of languages, including Persian and Chinese), the first one arrives empty with text dragged from firefox, but the second gets through OK. In all other situations, the original text gets through fine and is used. I have not come up with a sensible way to detect this within the javascript before posting to the server.
          • The yellow drag and drop message now reflects the types of handlers available (this will not reflect any 3rd party types registered, but I've taken this approach to avoid trying to construct language strings).
          • The MBZ icon (and any other large icon sizes) is now displayed correctly.

          Not fixed:

          • MBZ files do not have any special handlers - just treated as a normal file.
          • Dragging links across browsers

          I have investigated carefully the drag types declared by different browsers, with the results shown below:
          Destination browser: Firefox (Iceweasel 10.04, Linux)

          • Drag file - application/x-moz-file; text/x-moz-url; text/plain; Files => file uploaded
          • Drag url from address bar - text/x-moz-url; text/uri-list; text/plain; text/html => url uploaded
          • Drag url from page - text/x-moz-url; text/x-moz-url-data; text/x-moz-url-desc; text/uri-list; text/_moz_htmlcontext; text/_moz_htmlinfo; text/html; text/plain => url uploaded
          • Drag url from Chrome address bar - application/x-moz-file; text/x-moz-url; text/plain; Files => attempts to upload file and fails (indistinguishable from file upload until the drop event occurs; once the drop has occurred, I can see that there are no files and fall-back to the application/x-moz-url type - will add another commit in a moment that does this)
          • Drag url from Chrome page - application/x-moz-file; text/html; text/x-moz-url; text/plain; Files => attempts to upload file and fails (same comment as last item; I don't want to rely on the extra 'text/html' to distinguish from files)
          • Drag text from firefox - text/_moz_htmlcontext; text/_moz_htmlinfo; text/html; text/plain => upload html
          • Drag text from chrome - application/x-moz-file; text/html; text/plain; Files => tries to upload file and fails (will try same fix as above)
          • Drag text from Libre Office / Kate text editor - empty list of types; nothing I can do about it

          Destination browser Chrome (19, Linux):

          • Drag file - Files => uploads file
          • Drag url from firefox (page or address bar) - text/html; text/uri-list; text/plain => uploads url
          • Drag url from chrome page - text/html; text/uri-list; text/plain => uploads url
          • Drag url from chrome address bar - text/uri-list; text/plain => uploads url
          • Drag text from firefox - text/html; text/plain => uploads html (with byte order fix)
          • Drag text from chrome - text/html; text/plain => uploads html (also with byte order fix, but ignored)
          • Drag text from libre office - text/html; text/plain => uploads html
          • Drag text from kate (text editor) - types is null
          Show
          Davo Smith added a comment - I've added a new branch called ' MDL-22504 _drag_and_drop_upload_integration' that addresses as many of the issues as I've been able to fix: Text dragged FF => Chrom(ium|e) - looks like this was providing the text with the unicode bytes reversed. I've fixed this by sending off both the raw contents and a version with each pair of bytes reversed to the server. By experimentation (tested with a range of languages, including Persian and Chinese), the first one arrives empty with text dragged from firefox, but the second gets through OK. In all other situations, the original text gets through fine and is used. I have not come up with a sensible way to detect this within the javascript before posting to the server. The yellow drag and drop message now reflects the types of handlers available (this will not reflect any 3rd party types registered, but I've taken this approach to avoid trying to construct language strings). The MBZ icon (and any other large icon sizes) is now displayed correctly. Not fixed: MBZ files do not have any special handlers - just treated as a normal file. Dragging links across browsers I have investigated carefully the drag types declared by different browsers, with the results shown below: Destination browser: Firefox (Iceweasel 10.04, Linux) Drag file - application/x-moz-file; text/x-moz-url; text/plain; Files => file uploaded Drag url from address bar - text/x-moz-url; text/uri-list; text/plain; text/html => url uploaded Drag url from page - text/x-moz-url; text/x-moz-url-data; text/x-moz-url-desc; text/uri-list; text/_moz_htmlcontext; text/_moz_htmlinfo; text/html; text/plain => url uploaded Drag url from Chrome address bar - application/x-moz-file; text/x-moz-url; text/plain; Files => attempts to upload file and fails (indistinguishable from file upload until the drop event occurs; once the drop has occurred, I can see that there are no files and fall-back to the application/x-moz-url type - will add another commit in a moment that does this) Drag url from Chrome page - application/x-moz-file; text/html; text/x-moz-url; text/plain; Files => attempts to upload file and fails (same comment as last item; I don't want to rely on the extra 'text/html' to distinguish from files) Drag text from firefox - text/_moz_htmlcontext; text/_moz_htmlinfo; text/html; text/plain => upload html Drag text from chrome - application/x-moz-file; text/html; text/plain; Files => tries to upload file and fails (will try same fix as above) Drag text from Libre Office / Kate text editor - empty list of types; nothing I can do about it Destination browser Chrome (19, Linux): Drag file - Files => uploads file Drag url from firefox (page or address bar) - text/html; text/uri-list; text/plain => uploads url Drag url from chrome page - text/html; text/uri-list; text/plain => uploads url Drag url from chrome address bar - text/uri-list; text/plain => uploads url Drag text from firefox - text/html; text/plain => uploads html (with byte order fix) Drag text from chrome - text/html; text/plain => uploads html (also with byte order fix, but ignored) Drag text from libre office - text/html; text/plain => uploads html Drag text from kate (text editor) - types is null
          Hide
          Davo Smith added a comment -

          I've added a further commit (after many hours of messing around with this), which fixes the following issues:

          • Links can now be dragged from chrome => firefox
          • Text from chrome => firefox works reasonably well for Latin characters, not well for non-Latin characters (I've spent a long time trying to fix this, but I've run out of ideas - I'm hoping this is a fairly unusual use-case); text from firefox => firefox still works fine
          • An annoying Firefox bug whereby the URLs of the inserted activities had '~' characters encoded as '%7e', which then logged the user out when they clicked on it

          The commit can be found in my 'MDL-22504_drag_and_drop_upload_integration' branch.

          Show
          Davo Smith added a comment - I've added a further commit (after many hours of messing around with this), which fixes the following issues: Links can now be dragged from chrome => firefox Text from chrome => firefox works reasonably well for Latin characters, not well for non-Latin characters (I've spent a long time trying to fix this, but I've run out of ideas - I'm hoping this is a fairly unusual use-case); text from firefox => firefox still works fine An annoying Firefox bug whereby the URLs of the inserted activities had '~' characters encoded as '%7e', which then logged the user out when they clicked on it The commit can be found in my ' MDL-22504 _drag_and_drop_upload_integration' branch.
          Hide
          Dan Poltawski added a comment -

          Hi,

          I've just been looking at this and really looking at these hacks we now have to do to sort out the content, and varying results on different browsers it makes me think that we should go the simpler for users and developers alike solution of avoiding non-file type drag drop all together.

          I know thats radical, but it was my initial instinct too some months ago. Do non-tech people really understand the concept of dragging texts and links around like this anyway? Especially non-mac users?

          So, leaving this for discussion and sam to consider.

          Show
          Dan Poltawski added a comment - Hi, I've just been looking at this and really looking at these hacks we now have to do to sort out the content, and varying results on different browsers it makes me think that we should go the simpler for users and developers alike solution of avoiding non-file type drag drop all together. I know thats radical, but it was my initial instinct too some months ago. Do non-tech people really understand the concept of dragging texts and links around like this anyway? Especially non-mac users? So, leaving this for discussion and sam to consider.
          Hide
          Davo Smith added a comment - - edited

          To summarise the issues:

          • Drag and drop of files - no issues
          • Drag and drop of urls/text into chrome - no issues (but one bit of content hacking for text dragged from FF=>chrome under Linux, which appears to be reliable)
          • Drag and drop of urls into firefox - no issues
          • Drag and drop of text into firefox - unreliable (works FF=>FF, corrupts non-ASCII characters chrome=>FF, does nothing for other applications => FF under Linux)

          If we wanted to leave file drag and drop in and make text/url dragging an optional system setting (default to files only), then the obvious place, to my mind, would be to edit 'get_js_data' and add an 'if ($CFG->dndfilesonly) {' around the 'foreach' loop (as well as the settings code).

          Show
          Davo Smith added a comment - - edited To summarise the issues: Drag and drop of files - no issues Drag and drop of urls/text into chrome - no issues (but one bit of content hacking for text dragged from FF=>chrome under Linux, which appears to be reliable) Drag and drop of urls into firefox - no issues Drag and drop of text into firefox - unreliable (works FF=>FF, corrupts non-ASCII characters chrome=>FF, does nothing for other applications => FF under Linux) If we wanted to leave file drag and drop in and make text/url dragging an optional system setting (default to files only), then the obvious place, to my mind, would be to edit 'get_js_data' and add an 'if ($CFG->dndfilesonly) {' around the 'foreach' loop (as well as the settings code).
          Hide
          Sam Hemelryk added a comment -

          Sorry. I've completely overlooked this while working on other reviews.
          Having had a quick look now I really don't like/trust the JS hacks for FF issues. To be truthful I think we are certainly best to remove those.
          I think the idea of having a setting to allow drag and drop of content (not just files) is a good idea and I think perhaps the solution is to make it a setting under experimental headings, disabled by default, noted as not working if firefox presently and then remove the FF hacks a leave a todo to fix it once issue within Firefox have themselves being fixed.
          That way those who still want to enable it can (and I do think it is a cool feature and should be possible) and there won't be any false pretenses about it working as the experimental setting will say it all.
          Whats your thoughts guys?

          Show
          Sam Hemelryk added a comment - Sorry. I've completely overlooked this while working on other reviews. Having had a quick look now I really don't like/trust the JS hacks for FF issues. To be truthful I think we are certainly best to remove those. I think the idea of having a setting to allow drag and drop of content (not just files) is a good idea and I think perhaps the solution is to make it a setting under experimental headings, disabled by default, noted as not working if firefox presently and then remove the FF hacks a leave a todo to fix it once issue within Firefox have themselves being fixed. That way those who still want to enable it can (and I do think it is a cool feature and should be possible) and there won't be any false pretenses about it working as the experimental setting will say it all. Whats your thoughts guys?
          Hide
          Martin Dougiamas added a comment -

          My +1 for a $CFG->dndallowtextandlinks to enable those two, with a description explaining the issues.

          Show
          Martin Dougiamas added a comment - My +1 for a $CFG->dndallowtextandlinks to enable those two, with a description explaining the issues.
          Hide
          Davo Smith added a comment -

          The 'MDL-22504_drag_and_drop_upload_integration' branch now includes the $CFG->dndallowtextandlinks setting on the experimental settings page and removes the content hacks (and adds an extra check to catch the 'empty contents' upload a bit earlier and give a more helpful error message about it).

          Show
          Davo Smith added a comment - The ' MDL-22504 _drag_and_drop_upload_integration' branch now includes the $CFG->dndallowtextandlinks setting on the experimental settings page and removes the content hacks (and adds an extra check to catch the 'empty contents' upload a bit earlier and give a more helpful error message about it).
          Hide
          Dan Poltawski added a comment -

          Hi Davo,

          I'm afraid I don't like the look of that one sorry, because:

          1. The experimental feature isn't matching our style of just being a 'flag' on or off. It should just be an on or off tickbox like the other ones in experimental settings. In fact your description implies that too. So if that could just be a tickbox it'd be better.
          2. Use if (!empty($CFG->dndallowtextandlinks)) { to prevent warnings when not set
          3. My gut feeling about that combination of strings for different variations of files/links being enable seems over the top. However I realise it might be the best solution from a translation point of view. My thoughts for now is that we just ignore the multiple file types in that string. However, i'm still thinking if that really is best.

          Dan

          Show
          Dan Poltawski added a comment - Hi Davo, I'm afraid I don't like the look of that one sorry, because: The experimental feature isn't matching our style of just being a 'flag' on or off. It should just be an on or off tickbox like the other ones in experimental settings. In fact your description implies that too. So if that could just be a tickbox it'd be better. Use if (!empty($CFG->dndallowtextandlinks)) { to prevent warnings when not set My gut feeling about that combination of strings for different variations of files/links being enable seems over the top. However I realise it might be the best solution from a translation point of view. My thoughts for now is that we just ignore the multiple file types in that string. However, i'm still thinking if that really is best. Dan
          Hide
          Davo Smith added a comment -

          1. I went for a drop-down, as I thought that was clearer about exactly what you were enabling, but I've changed to a tickbox
          2. I thought about that, but assumed that after the upgrade the setting would always be present, but have adjusted the code
          3. I don't really like the multiple strings either, but the alternatives of constructing language strings or not explaining what you can drag and drop ('drag and drop things' or just 'drag and drop files') would seem to me to be even less desirable (they would make it hard for teachers to discover the extra features - assuming they were enabled by the site admin; it would also make it hard for teachers already aware of the feature to know, if moving between Moodle sites, whether the feature was available).

          Show
          Davo Smith added a comment - 1. I went for a drop-down, as I thought that was clearer about exactly what you were enabling, but I've changed to a tickbox 2. I thought about that, but assumed that after the upgrade the setting would always be present, but have adjusted the code 3. I don't really like the multiple strings either, but the alternatives of constructing language strings or not explaining what you can drag and drop ('drag and drop things' or just 'drag and drop files') would seem to me to be even less desirable (they would make it hard for teachers to discover the extra features - assuming they were enabled by the site admin; it would also make it hard for teachers already aware of the feature to know, if moving between Moodle sites, whether the feature was available).
          Hide
          Dan Poltawski added a comment -

          OK thanks a lot for that Davo.

          Discussed a bit between integrators and couldn't think fo a better way to do the lang strings so have pulled that in now.

          Show
          Dan Poltawski added a comment - OK thanks a lot for that Davo. Discussed a bit between integrators and couldn't think fo a better way to do the lang strings so have pulled that in now.
          Hide
          Dan Poltawski added a comment -

          Fred: are you able to do another quick test on this?

          Show
          Dan Poltawski added a comment - Fred: are you able to do another quick test on this?
          Hide
          Frédéric Massart added a comment -

          Test successful.

          I just noticed that the icons for xls, ods files are dmg icons. And the icons for docx files are zip icons.

          Good job!

          Show
          Frédéric Massart added a comment - Test successful. I just noticed that the icons for xls, ods files are dmg icons. And the icons for docx files are zip icons. Good job!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!
          Hide
          David Mudrak added a comment -

          Hi guys. Just for your information - seems that strings were MOVed from lang/en/dndupload.php without an appropriate AMOScript. I know it is a bit annoying but AMOS replays the whole history of commits even on the master branch. So it did register the new dndupload.php file and folks started to translate it. Nothing serious happened this time so no stress. I just wanted to mention

          Show
          David Mudrak added a comment - Hi guys. Just for your information - seems that strings were MOVed from lang/en/dndupload.php without an appropriate AMOScript. I know it is a bit annoying but AMOS replays the whole history of commits even on the master branch. So it did register the new dndupload.php file and folks started to translate it. Nothing serious happened this time so no stress. I just wanted to mention
          Hide
          Geoffrey Rowland added a comment -

          Similar to Frédéric Massart's comment
          Testing latest Moodle 2.3 alpha with Firefox 12
          Drag and Drop upload generally working well, but some Microsoft file types are uploaded with wrong icon
          e.g. pptx and xls show as docx
          some docx show as docx others (created by Open Office?) as some form of zipped format (mbz?)
          xlsx also show as zipped

          I have not investigated further, but wondering if there is an issue with mime-type detection by OS/Browser combinations?

          Show
          Geoffrey Rowland added a comment - Similar to Frédéric Massart's comment Testing latest Moodle 2.3 alpha with Firefox 12 Drag and Drop upload generally working well, but some Microsoft file types are uploaded with wrong icon e.g. pptx and xls show as docx some docx show as docx others (created by Open Office?) as some form of zipped format (mbz?) xlsx also show as zipped I have not investigated further, but wondering if there is an issue with mime-type detection by OS/Browser combinations?
          Hide
          Davo Smith added a comment -

          Are the icons correct after a page refresh? If they are still wrong after the page refresh, then this is probably unrelated to drag and drop upload.

          Show
          Davo Smith added a comment - Are the icons correct after a page refresh? If they are still wrong after the page refresh, then this is probably unrelated to drag and drop upload.
          Hide
          Geoffrey Rowland added a comment -

          Get same icon issue if files are uploaded 'conventionally' through the file picker. So, presumably an issue with core file/mime-type recognition rather than drag-n-drop per se.

          Show
          Geoffrey Rowland added a comment - Get same icon issue if files are uploaded 'conventionally' through the file picker. So, presumably an issue with core file/mime-type recognition rather than drag-n-drop per se.
          Hide
          Geoffrey Rowland added a comment -

          And wrong icon remains after a page refresh

          Show
          Geoffrey Rowland added a comment - And wrong icon remains after a page refresh
          Hide
          Martin Dougiamas added a comment -

          If you are using moodle.git, then it might be some days behind, which means you are seeing old bugs. eg MDL-33144

          Show
          Martin Dougiamas added a comment - If you are using moodle.git, then it might be some days behind, which means you are seeing old bugs. eg MDL-33144
          Hide
          Geoffrey Rowland added a comment -

          Yes, had installed a few days ago, Moodle 2.3dev (Build: 20120522). So, I'll update and check if fixed.

          Show
          Geoffrey Rowland added a comment - Yes, had installed a few days ago, Moodle 2.3dev (Build: 20120522). So, I'll update and check if fixed.
          Hide
          Geoffrey Rowland added a comment -

          Just to confirm the recent patch for MDL-33144 fixed file icon display for us
          Thanks Martin!

          Show
          Geoffrey Rowland added a comment - Just to confirm the recent patch for MDL-33144 fixed file icon display for us Thanks Martin!
          Hide
          Helen Foster added a comment -
          Show
          Helen Foster added a comment - Davo, thanks a lot for this very cool new feature in Moodle 2.3, which is now documented in the 2.3 wiki: http://docs.moodle.org/23/en/File_module_settings http://docs.moodle.org/23/en/Folder_module_settings http://docs.moodle.org/23/en/Course_homepage#Drag_and_drop_method

            People

            • Votes:
              30 Vote for this issue
              Watchers:
              28 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: