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

      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

        Gliffy Diagrams

          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 Skoda 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 Skoda 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 Skoda added a comment -

            looks ok, +1 for commit

            Show
            Petr Skoda 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: