Moodle
  1. Moodle
  2. MDL-14589 META: Develop new File API
  3. MDL-22548

the filemanager element MUST support the main file selection natively in ajax UI

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Files API
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32796

      Description

      I just tested the mod/resource mod edit form - the file manager with main file selection is absolutely horrible, working on a temporary workaround,
      but the main file selection really needs to be implemented natively in the file manager - only when JS off it should be a separate text box

        Issue Links

          Activity

          Hide
          Martin Dougiamas added a comment -

          Yes, no arguments here about that.

          The main file selection actually has been in the main file manager for a long time, but it was hard to find. I think the new menu from yesterday improves that at least. Click on the menu icon after any file to see it.

          However, I think we need to improve this a lot more.

          Javascript version:

          1) Hide the Main file element COMPLETELY.
          2) If only one file is copied into the file area, then select that by default (this can be done after submit in server-side validation).
          3) If more than one file is uploaded and a main file isn't specified, then server-side validation should reload the form and tell the user what to do. This will happen so infrequently that I think it's worth not confusing everyone with the main file element.

          Non-Javascript version (completely broken at the moment!)

          1) I think we again need to hide the main file element with CSS
          2) Add a "set as main file" link after each file, but only if there are more than one.

          Show
          Martin Dougiamas added a comment - Yes, no arguments here about that. The main file selection actually has been in the main file manager for a long time, but it was hard to find. I think the new menu from yesterday improves that at least. Click on the menu icon after any file to see it. However, I think we need to improve this a lot more. Javascript version: 1) Hide the Main file element COMPLETELY. 2) If only one file is copied into the file area, then select that by default (this can be done after submit in server-side validation). 3) If more than one file is uploaded and a main file isn't specified, then server-side validation should reload the form and tell the user what to do. This will happen so infrequently that I think it's worth not confusing everyone with the main file element. Non-Javascript version (completely broken at the moment!) 1) I think we again need to hide the main file element with CSS 2) Add a "set as main file" link after each file, but only if there are more than one.
          Hide
          Martin Dougiamas added a comment - - edited

          After some more talking and looking at this, the main technical difficulty with non-Javascript is the separation of the "Main file" concept from the file API. It's difficult to have a clean interface inside the filemanager to update a field belonging to a module.

          Why don't we add a small boolean field to the "file" table called "mainfile" and add some File API call to set the main file for a filearea? Right now this would mostly only be useful for the resource module, but I think it could be very useful for other modules in future. I can imagine students submitting a web site in an assignment, for example (yeah yeah XSS risks etc, that is a problem for the module and permissions). All that can be discussed later, but the File API should support the concept.

          I propose we do this:

          1) GET JS INTERFACE FIXED FOR TESTERS NOW

          • hide the main file element completely
          • implement server form validation to auto-select mainfile when there is one, and flag error on form when 2 or more, telling them how to use the context menu to select the main file.

          2) IF WE AGREE ON THE MAINFILE FIELD IN FILE TABLE

          • add the flag field, and any new API call to set the flag for any file in a given filearea
          • add a flag to filemanager element to enable/disable the mainfile selection in the menus (so that it will normally not be seen in generic file areas)
          • add upgrade code to make sure mainfile selections in the resource table are copied to the file table
          • drop the varchar mainfile field from the resource table
          • update the resource code to use the new API for server form validation

          3) FIX THE NON-JS INTERFACE

          • We need to load the OBJECT in the page by default and hide/replace it with the JS version. It should be possible to make this pretty seamless with some work (Sam is doing a lot of this with the custom menus and docked blocks etc)
          • Add [Set as main file] links after every file whenever mainfile selection is enabled AND there is more than one file.

          Petr, let me know if you have a strong objection to this new field in the file table. The other option would be a new table like file_mainfile but that seems overkill for such a small boolean value.

          Show
          Martin Dougiamas added a comment - - edited After some more talking and looking at this, the main technical difficulty with non-Javascript is the separation of the "Main file" concept from the file API. It's difficult to have a clean interface inside the filemanager to update a field belonging to a module. Why don't we add a small boolean field to the "file" table called "mainfile" and add some File API call to set the main file for a filearea? Right now this would mostly only be useful for the resource module, but I think it could be very useful for other modules in future. I can imagine students submitting a web site in an assignment, for example (yeah yeah XSS risks etc, that is a problem for the module and permissions). All that can be discussed later, but the File API should support the concept. I propose we do this: 1) GET JS INTERFACE FIXED FOR TESTERS NOW hide the main file element completely implement server form validation to auto-select mainfile when there is one, and flag error on form when 2 or more, telling them how to use the context menu to select the main file. 2) IF WE AGREE ON THE MAINFILE FIELD IN FILE TABLE add the flag field, and any new API call to set the flag for any file in a given filearea add a flag to filemanager element to enable/disable the mainfile selection in the menus (so that it will normally not be seen in generic file areas) add upgrade code to make sure mainfile selections in the resource table are copied to the file table drop the varchar mainfile field from the resource table update the resource code to use the new API for server form validation 3) FIX THE NON-JS INTERFACE We need to load the OBJECT in the page by default and hide/replace it with the JS version. It should be possible to make this pretty seamless with some work (Sam is doing a lot of this with the custom menus and docked blocks etc) Add [Set as main file] links after every file whenever mainfile selection is enabled AND there is more than one file. Petr, let me know if you have a strong objection to this new field in the file table. The other option would be a new table like file_mainfile but that seems overkill for such a small boolean value.
          Hide
          Martin Dougiamas added a comment -

          I think we reached general agreement in chat this afternoon to call the new field "sortorder" and to make it an int with a default value of 0.

          The resource module can use it to make the "main file" the "first file". I guess the value would be 1 for this file. (Higher numbers mean higher in importance).

          Other modules may choose to use this field to sort files in a particular way, or use it to store version numbers or whatever. It's up to the module.

          Show
          Martin Dougiamas added a comment - I think we reached general agreement in chat this afternoon to call the new field "sortorder" and to make it an int with a default value of 0. The resource module can use it to make the "main file" the "first file". I guess the value would be 1 for this file. (Higher numbers mean higher in importance). Other modules may choose to use this field to sort files in a particular way, or use it to store version numbers or whatever. It's up to the module.
          Hide
          Dongsheng Cai added a comment -

          This is the patch to add sortorder field including related changes to resource module.

          I wasn't sure if we should remove the mainfile field in resource module table.

          Show
          Dongsheng Cai added a comment - This is the patch to add sortorder field including related changes to resource module. I wasn't sure if we should remove the mainfile field in resource module table.
          Hide
          Dongsheng Cai added a comment -

          I forgot the main file migrating part, will add another patch of mod/resource/db/upgrade.php

          Show
          Dongsheng Cai added a comment - I forgot the main file migrating part, will add another patch of mod/resource/db/upgrade.php
          Hide
          Petr Škoda added a comment -

          1/ function file_set_sortorder($contextid, $filearea, $itemid, $filepath, $filename, $sortorder=0) {
          why default 0 there?

          2/ the lib/db/upgrade.php should be updated also when it creates the files table

          3/ the mod/resource initial migration should be updated too to not use the mainfile at all + there should be some upgrade path for PR2 users

          Otherwise it looks ok, please upload the the patch here again, thanks!

          Show
          Petr Škoda added a comment - 1/ function file_set_sortorder($contextid, $filearea, $itemid, $filepath, $filename, $sortorder=0) { why default 0 there? 2/ the lib/db/upgrade.php should be updated also when it creates the files table 3/ the mod/resource initial migration should be updated too to not use the mainfile at all + there should be some upgrade path for PR2 users Otherwise it looks ok, please upload the the patch here again, thanks!
          Hide
          Dongsheng Cai added a comment -
          1. ok, it doesn't make sense, will make it required.
          2. working on it, will attach another patch soon.
          Show
          Dongsheng Cai added a comment - ok, it doesn't make sense, will make it required. working on it, will attach another patch soon.
          Hide
          Dongsheng Cai added a comment -

          new patch with resource module upgrade

          Show
          Dongsheng Cai added a comment - new patch with resource module upgrade
          Hide
          Petr Škoda added a comment -

          looks ok, +1 for commit

          Show
          Petr Škoda added a comment - looks ok, +1 for commit
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Side note: New field in DB means change in backup (tip: backup_final_files_structure_step)

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Side note: New field in DB means change in backup (tip: backup_final_files_structure_step) Ciao
          Hide
          Dongsheng Cai added a comment -

          Thanks for the note Eloy.
          The main file field has been removed from backup_final_files_structure_step

          Show
          Dongsheng Cai added a comment - Thanks for the note Eloy. The main file field has been removed from backup_final_files_structure_step
          Hide
          Eloy Lafuente (stronk7) added a comment -

          hehe, I guess it's not only about removing 'mainfile' from resources backup (that, indeed, needs dropping from DB too?)

          But it's about to add the new field 'sortorder' to backup_final_files_structure_step (seems not done).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - hehe, I guess it's not only about removing 'mainfile' from resources backup (that, indeed, needs dropping from DB too?) But it's about to add the new field 'sortorder' to backup_final_files_structure_step (seems not done). Ciao
          Hide
          Martin Dougiamas added a comment -

          Looks great now! Thanks everyone!

          Show
          Martin Dougiamas added a comment - Looks great now! Thanks everyone!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: