Moodle
  1. Moodle
  2. MDL-29413

filepicker form element does not have a delete file option

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: DEV backlog
    • Fix Version/s: FRONTEND
    • Component/s: Filepicker
    • Labels:
    • Rank:
      18928

      Description

      The filepicker works great to manage just one file that is needed in an activity, stored in an associated file area.

      It would be nice to have an option in the interface to delete the file instead of just being able to replace it and thus remove the only file in the files area associated.

      Then for example the filepicker could be used for a much nicer user profile image uploader, when used with the preview media file code for the file picker (see MDL-29412) that would show a thumbnail of the user profile pic then we could replace the current kind of clunky interface we have now.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          filepicker is supposed to upload one file which is later processed (such as when uploading CSV files), it is not supposed to contain any "current" file, filemanager has maximum number of files and the delete option.

          The user profile picture is a bit problematic because the file has to be stored in different resolutions, technically it should be possible to use filemanager instead of the filepicker, but it would require changes in internal storage or some other hacking...

          I am proposing to close this as not a bug.

          Show
          Petr Škoda added a comment - filepicker is supposed to upload one file which is later processed (such as when uploading CSV files), it is not supposed to contain any "current" file, filemanager has maximum number of files and the delete option. The user profile picture is a bit problematic because the file has to be stored in different resolutions, technically it should be possible to use filemanager instead of the filepicker, but it would require changes in internal storage or some other hacking... I am proposing to close this as not a bug.
          Hide
          Jamie Pratt added a comment -

          I think it is nice to have a separate form element to manage a single file stored in it's own file area. I am already using this in a drag and drop question type were we upload images. The filepicker js was nice and simple to tie into so I could respond to the user uploading an image and make a question preview area in the question editing form.

          I use prepare_draft_file_area to copy files in the file area to the draft file area for the file picker and then after form save I copy the files back into the file area.

          In the filepicker it is also relatively easy to write code to preview the media uploaded which would be more of a pain to do for the file manager.

          Could a delete file action be an option selectable in the form definition for the filepicker?

          Show
          Jamie Pratt added a comment - I think it is nice to have a separate form element to manage a single file stored in it's own file area. I am already using this in a drag and drop question type were we upload images. The filepicker js was nice and simple to tie into so I could respond to the user uploading an image and make a question preview area in the question editing form. I use prepare_draft_file_area to copy files in the file area to the draft file area for the file picker and then after form save I copy the files back into the file area. In the filepicker it is also relatively easy to write code to preview the media uploaded which would be more of a pain to do for the file manager. Could a delete file action be an option selectable in the form definition for the filepicker?
          Hide
          Petr Škoda added a comment -

          I personally do not like the idea of encouraging different use of filepicker forms element. File manager is supposed to do that. Hacking filepicker might be easier for you but I think it is a wrong direction.

          The difference is not "one file X many files", the difference is "pick one file for immediate use X manage files in area", sorry.

          Show
          Petr Škoda added a comment - I personally do not like the idea of encouraging different use of filepicker forms element. File manager is supposed to do that. Hacking filepicker might be easier for you but I think it is a wrong direction. The difference is not "one file X many files", the difference is "pick one file for immediate use X manage files in area", sorry.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Petr,
          I understand your objections but at the same time I find Jamie idea very interesting : I am working on a plugin for podcasting where teachers can add a small thumbnail image to medias as a preview. Currently my interface is similar (and as awfull and counter-intuitive! ) to the one used for the profile image : filepicker with a checkbox to delete the thumbnail image so that it is possible to revert to the default preview image.
          I don't really think this is a "manage files in area" task but more a "pick one file for immediate use" simply I need also to be able to delete this file.
          Any chance you can reconsider your position ?

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Petr, I understand your objections but at the same time I find Jamie idea very interesting : I am working on a plugin for podcasting where teachers can add a small thumbnail image to medias as a preview. Currently my interface is similar (and as awfull and counter-intuitive! ) to the one used for the profile image : filepicker with a checkbox to delete the thumbnail image so that it is possible to revert to the default preview image. I don't really think this is a "manage files in area" task but more a "pick one file for immediate use" simply I need also to be able to delete this file. Any chance you can reconsider your position ?
          Hide
          Petr Škoda added a comment -

          Yes, I like that preview idea too, both in filepicker and filemanager forms elements. BUT the filepicker is not supposed to manage existing files (in this example user avatar). I agree the profile page might better use the filemanger with 1 file, somebody just needs to improve the filemanager element to behave in a bit different way when maximum 1 file allowed, then of course somebody has to update the profile edit code to deal with this change...

          Show
          Petr Škoda added a comment - Yes, I like that preview idea too, both in filepicker and filemanager forms elements. BUT the filepicker is not supposed to manage existing files (in this example user avatar). I agree the profile page might better use the filemanger with 1 file, somebody just needs to improve the filemanager element to behave in a bit different way when maximum 1 file allowed, then of course somebody has to update the profile edit code to deal with this change...
          Hide
          Jamie Pratt added a comment -

          Hi Petr,

          Thanks for your great work with the Moodle architecture and managing the direction of the code.

          Allow me to express my point of view.

          When selecting a form element to use to manage one file in a file area the filepicker for me seemed the best choice. It does just that manages one file in a draft files area. I think since attaching just one file to an activity will be quite a common sub case then it makes sense to have special handling of this sub case. The issue is then whether to use the filepicker to handle this sub case or put special code into the file manager.

          To me it seems that extending the functionality of the filepicker to handle this case is more natural than extending the functionality of the file manager. The filepicker as you envisioned it is to upload one file to a file draft area, process that file and delete it. All that it takes to make it handle the manage single file in files area case is to prepare_draft_files_area before hand and save_draft_files_area afterwards. It would be nice also maybe allowing for an optional delete file action that could be selected through the file picker options.

          I had assumed that this was a valid use of filepicker to upload a single file although you had not documented this use of filepicker.

          Show
          Jamie Pratt added a comment - Hi Petr, Thanks for your great work with the Moodle architecture and managing the direction of the code. Allow me to express my point of view. When selecting a form element to use to manage one file in a file area the filepicker for me seemed the best choice. It does just that manages one file in a draft files area. I think since attaching just one file to an activity will be quite a common sub case then it makes sense to have special handling of this sub case. The issue is then whether to use the filepicker to handle this sub case or put special code into the file manager. To me it seems that extending the functionality of the filepicker to handle this case is more natural than extending the functionality of the file manager. The filepicker as you envisioned it is to upload one file to a file draft area, process that file and delete it. All that it takes to make it handle the manage single file in files area case is to prepare_draft_files_area before hand and save_draft_files_area afterwards. It would be nice also maybe allowing for an optional delete file action that could be selected through the file picker options. I had assumed that this was a valid use of filepicker to upload a single file although you had not documented this use of filepicker.
          Hide
          Jamie Pratt added a comment - - edited

          I just took another look at the filemanager element. In order to make the file manager form element optimally simple to handle just one file it would be nice to make a number of changes :

          • change the 'Add..' button label to 'Choose File...'
          • remove the download all button
          • remove the path link
          • remove the create folder button
          • remove the fileactions icon which has a popup menu using a dhtml layer and just have a delete icon in it's place. If we are uploading a single file :
            • we don't need a move action,
            • I can't imagine a case where we would need a rename action
            • The download action is not needed because clicking on the link in the file picker forces a download anyway
            • The delete action could optionally be implemented by a delete icon. It would be the default to have this delete option off as it is not needed in many cases.

          After all this work we would be back to the interface already almost implemented by the filepicker form element (with the exception of adding an optional icon for deleting a file).

          Show
          Jamie Pratt added a comment - - edited I just took another look at the filemanager element. In order to make the file manager form element optimally simple to handle just one file it would be nice to make a number of changes : change the 'Add..' button label to 'Choose File...' remove the download all button remove the path link remove the create folder button remove the fileactions icon which has a popup menu using a dhtml layer and just have a delete icon in it's place. If we are uploading a single file : we don't need a move action, I can't imagine a case where we would need a rename action The download action is not needed because clicking on the link in the file picker forces a download anyway The delete action could optionally be implemented by a delete icon. It would be the default to have this delete option off as it is not needed in many cases. After all this work we would be back to the interface already almost implemented by the filepicker form element (with the exception of adding an optional icon for deleting a file).
          Hide
          Jamie Pratt added a comment -

          Adding a screen shot of the filemanager element.

          Show
          Jamie Pratt added a comment - Adding a screen shot of the filemanager element.
          Hide
          Michael de Raadt added a comment -

          I tend to agree that deleting a file or performing other file activities within the filepicker is not going to be simple. The file picker is not a file management system.

          Your other file picker improvements are worth considering separately. Could you create a new issue with just these in it?

          Show
          Michael de Raadt added a comment - I tend to agree that deleting a file or performing other file activities within the filepicker is not going to be simple. The file picker is not a file management system. Your other file picker improvements are worth considering separately. Could you create a new issue with just these in it?
          Hide
          Jamie Pratt added a comment - - edited

          Hi Michael,

          The filepicker already seems to do a pretty good job of managing a files area with a limit of one file in it. Except it might be helpful to have a delete file action for use in some forms. It is simple and does everything that the filemanager file element would do for one file.

          This issue only deals with the delete file option for the filepicker. My other improvements to the filepicker are discussed in MDL-29413 and MDL-27919

          Show
          Jamie Pratt added a comment - - edited Hi Michael, The filepicker already seems to do a pretty good job of managing a files area with a limit of one file in it. Except it might be helpful to have a delete file action for use in some forms. It is simple and does everything that the filemanager file element would do for one file. This issue only deals with the delete file option for the filepicker. My other improvements to the filepicker are discussed in MDL-29413 and MDL-27919
          Hide
          Jamie Pratt added a comment -

          I was just looking at the ajax script that the filepicker interacts with. I think we could add an action there to delete the file single in the file area. I see that there is already a deleted named file action in the ajax script 'deletetmpfile'

          We would have an 'are you sure' dialog pop up before we communicate with the ajax.

          We would need to implement an alternative for browsers without js enabled.

          It would seem that it would be a good idea to put some effort into the single file in draft files area case as this case is probably as or more common than the multiple file/folder files area case.

          I would be willing to prepare a patch to implement this optional delete option when I have time if it seems there is some interest in going in that direction. But maybe I have missed some reason why this is not a good direction to go in?

          Show
          Jamie Pratt added a comment - I was just looking at the ajax script that the filepicker interacts with. I think we could add an action there to delete the file single in the file area. I see that there is already a deleted named file action in the ajax script 'deletetmpfile' We would have an 'are you sure' dialog pop up before we communicate with the ajax. We would need to implement an alternative for browsers without js enabled. It would seem that it would be a good idea to put some effort into the single file in draft files area case as this case is probably as or more common than the multiple file/folder files area case. I would be willing to prepare a patch to implement this optional delete option when I have time if it seems there is some interest in going in that direction. But maybe I have missed some reason why this is not a good direction to go in?
          Hide
          Tim Hunt added a comment -

          So, to summarise the above, I think we are agreed that it would be good to have a form element optimised for managing a single file, with preview, if the file is a media file.

          The area of disagreement is the best way to implement that, and there are three choices:
          1. modify the filepicker element for this. This is Jamie's preferred option, and he claims you just need to add a delete icon and the preview code. He already has a patch for the latter.
          2. modify the filemanager element. This is Petr's preferred option, because "File manager is supposed to do that." but see Jamie's long list of bullet points of changes that would be required for filemanager to support this.
          3. create a new form element for this. No one is advocating this, but the changes required in 2. are so big that 2. might turn into 3. anyway.

          I do not actually soo the problem with 1. I realise that the original intention was that filepicker was designed for picking one file for immediate use and then discarding, while file-manager was designed for managing files. That was a sensible plan when the plan was made.

          But what we have learned now is that filepicker already works for managing a single file, even though that was not originally intended. So, why not build on that knowledge, and adopt approach 1. for implementing single-file management?

          Show
          Tim Hunt added a comment - So, to summarise the above, I think we are agreed that it would be good to have a form element optimised for managing a single file, with preview, if the file is a media file. The area of disagreement is the best way to implement that, and there are three choices: 1. modify the filepicker element for this. This is Jamie's preferred option, and he claims you just need to add a delete icon and the preview code. He already has a patch for the latter. 2. modify the filemanager element. This is Petr's preferred option, because "File manager is supposed to do that." but see Jamie's long list of bullet points of changes that would be required for filemanager to support this. 3. create a new form element for this. No one is advocating this, but the changes required in 2. are so big that 2. might turn into 3. anyway. I do not actually soo the problem with 1. I realise that the original intention was that filepicker was designed for picking one file for immediate use and then discarding, while file-manager was designed for managing files. That was a sensible plan when the plan was made. But what we have learned now is that filepicker already works for managing a single file, even though that was not originally intended. So, why not build on that knowledge, and adopt approach 1. for implementing single-file management?
          Hide
          Jamie Pratt added a comment -

          I posted to the General Developer Forum trying to explore what I was thinking a little more clearly : http://moodle.org/mod/forum/discuss.php?d=186318

          Show
          Jamie Pratt added a comment - I posted to the General Developer Forum trying to explore what I was thinking a little more clearly : http://moodle.org/mod/forum/discuss.php?d=186318
          Hide
          Petr Škoda added a comment -

          I am wondering why you are spending so much time on persuading everybody that file picker should be made to do standard file management, I suppose it would take a lot less time to just fix the current file manager to deal with "1 file" limit and it could be easily integrated next week...

          The filepicker was implemented as a direct replacement of old upload form field that was not designed for file management either.

          Show
          Petr Škoda added a comment - I am wondering why you are spending so much time on persuading everybody that file picker should be made to do standard file management, I suppose it would take a lot less time to just fix the current file manager to deal with "1 file" limit and it could be easily integrated next week... The filepicker was implemented as a direct replacement of old upload form field that was not designed for file management either.
          Hide
          Jamie Pratt added a comment -

          Hi Petr,

          I am surprised you don't see the validity of my argument.

          Why add extra complexity to the file manager when what we really want in the common case of having to upload one file is more of a file picker. I like the simplicity of the file picker. The difference between a file uploader that saves or doesn't save it's files seems an artificial one.

          I think it is important that we can discuss as a community our ideas about how to improve Moodle and the best way to do this. I think this helps everybody to see things from different angles and leads to the best solution. Although I can see that coders are most productive when they are coding.

          Show
          Jamie Pratt added a comment - Hi Petr, I am surprised you don't see the validity of my argument. Why add extra complexity to the file manager when what we really want in the common case of having to upload one file is more of a file picker. I like the simplicity of the file picker. The difference between a file uploader that saves or doesn't save it's files seems an artificial one. I think it is important that we can discuss as a community our ideas about how to improve Moodle and the best way to do this. I think this helps everybody to see things from different angles and leads to the best solution. Although I can see that coders are most productive when they are coding.
          Hide
          Dongsheng Cai added a comment -

          Hi all

          I have to agree with Jamie, it's much easier to add a delete button to filepicker than customizing file manager, it's only a matter of adding a few lines of js code and a new ajax action.

          Let's not considering this as a file management issue, it's actually a user accessibility issue, when user picked a wrong file, the first thing they think of is deleting the file not picking another one to replace it.

          Show
          Dongsheng Cai added a comment - Hi all I have to agree with Jamie, it's much easier to add a delete button to filepicker than customizing file manager, it's only a matter of adding a few lines of js code and a new ajax action. Let's not considering this as a file management issue, it's actually a user accessibility issue, when user picked a wrong file, the first thing they think of is deleting the file not picking another one to replace it.
          Hide
          Jamie Pratt added a comment - - edited

          I think the ideal interface for the single file picker is as the filepicker element does now :

          • if there is an existing file you can just upload a new file to replace it as the selected file.
          • or you can press the delete button if you want and upload another file. I think despite what I have said in previous comments / posts maybe the delete button should always be present as people might want to delete a file before uploading a new one, even if it is not necessary.
          Show
          Jamie Pratt added a comment - - edited I think the ideal interface for the single file picker is as the filepicker element does now : if there is an existing file you can just upload a new file to replace it as the selected file. or you can press the delete button if you want and upload another file. I think despite what I have said in previous comments / posts maybe the delete button should always be present as people might want to delete a file before uploading a new one, even if it is not necessary.
          Hide
          Petr Škoda added a comment -

          Hmmm, filemanager element should have contained the support for 1 file limit since the very beginning. The fact that it is easier to hack filepicker is not an argument for me. This is not about adding delete or preview to file picker, I do not have anything against that, BUT I strongly disagree with abusing of filepicker for management of existing files. During saving of files you need to verify the contents of the draft area and merge it with previous files, which is done using the file manager options - suddenly all the benefits of using filepicker disappear because for security reasons you have to explicitly specify the same options like in file manager. Also once you officially allow new use of forms element you have to support it 100%, somebody should regression test it after any change, it will be a lot harder to improve/rewrite filepicker in the future because you would need to deal with two different use cases.

          To sum it up:
          1/ filepicker is a replacement for upload field, not file manager
          2/ normal developers would welcome better "1 file support" in file manager
          3/ delete option might be useful in filepicker - in case the file is optional
          4/ only the person working on filemanager internals has to spend few hours on development and testing of the "max 1 file"

          Show
          Petr Škoda added a comment - Hmmm, filemanager element should have contained the support for 1 file limit since the very beginning. The fact that it is easier to hack filepicker is not an argument for me. This is not about adding delete or preview to file picker, I do not have anything against that, BUT I strongly disagree with abusing of filepicker for management of existing files. During saving of files you need to verify the contents of the draft area and merge it with previous files, which is done using the file manager options - suddenly all the benefits of using filepicker disappear because for security reasons you have to explicitly specify the same options like in file manager. Also once you officially allow new use of forms element you have to support it 100%, somebody should regression test it after any change, it will be a lot harder to improve/rewrite filepicker in the future because you would need to deal with two different use cases. To sum it up: 1/ filepicker is a replacement for upload field, not file manager 2/ normal developers would welcome better "1 file support" in file manager 3/ delete option might be useful in filepicker - in case the file is optional 4/ only the person working on filemanager internals has to spend few hours on development and testing of the "max 1 file"
          Hide
          Jamie Pratt added a comment - - edited

          Can the filepicker and filemanager be thought of as form elements to interact with the draft files area?

          The filepicker whether it is being used to upload a file to be processed and disposed of or whether the file will be saved in a files area always should only pick one file for the draft files area.

          The filemanager is a manager that manages mulitiple files in the draft files area.

          The code for preparing the draft files area or saving the files from the draft files area is separate to the form elements themselves. And can deal with one or multiple files.

          It might be that someone needs a form element that will upload multiple files to the draft files area that will be then processed and not saved in another file area. You might want to process a bunch of files and make a zip package or whatever, unlikely I guess but I wanted to illustrate that the form elements can be thought of as separate to what is done with the files or where files come from to populate the draft files area.

          Show
          Jamie Pratt added a comment - - edited Can the filepicker and filemanager be thought of as form elements to interact with the draft files area? The filepicker whether it is being used to upload a file to be processed and disposed of or whether the file will be saved in a files area always should only pick one file for the draft files area. The filemanager is a manager that manages mulitiple files in the draft files area. The code for preparing the draft files area or saving the files from the draft files area is separate to the form elements themselves. And can deal with one or multiple files. It might be that someone needs a form element that will upload multiple files to the draft files area that will be then processed and not saved in another file area. You might want to process a bunch of files and make a zip package or whatever, unlikely I guess but I wanted to illustrate that the form elements can be thought of as separate to what is done with the files or where files come from to populate the draft files area.
          Hide
          Jamie Pratt added a comment -

          I'm sorry I feel I am being a bit of a bore on this issue. I promise this will be my last comment on it. I really don't want to waste people's valuable time but at the same time I feel frustrated because I am still trying to understand the objections to my idea and I feel we may be making a mistake in how we consider the difference between the filepicker and filemanager. I am still worried I may not have got my point across. So let me get this off my chest so I can get on with coding.

          I really don't see why duplicating what the filepicker already does in the filemanager is a good idea. The filepicker is a great interface to pick one file. If we are dealing with one file it doesn't need to be 'managed'. As I have said I think the one file case is very common so it bares some consideration.

          Does anyone disagree that the filepicker is a better interface for picking one file? It is optimally simple and was designed to be similar to the single file uploader element that users are familiar with.

          How does duplicating functionality help with security and maintainability of code?

          The actual difference between the functionality of the two form elements is just how many files they deal with. They both deal with files in the draft files area. When there is an existing file the filepicker can already handle the display of an existing file.

          The code for copying the files to the draft area and merging files from the draft area back to the plug in files area is separate to the form elements themselves. This code can already be used with the filepicker element as well as the filemanager element, I think it can probably be used as is.

          The I think very elegant system for file upload to a plug in files area I understand as follows :

          1. Populate form by preparing a draft files area with a full set of copies of file records for the files in the plug in files area.
          2. User is allowed to manage files in draft files area.
          3. Changes in files in draft files area is merged back into the plug in files area.

          Using the file picker we always should end up with exactly one file in the draft file area and if the code written for steps 1 and 3 that manages the populating of files in the file area and merging changes back into plug in files area is working properly it should not matter which form element we use for 2, either the filemanager to manage multiple files or the filepicker to pick one file.

          Show
          Jamie Pratt added a comment - I'm sorry I feel I am being a bit of a bore on this issue. I promise this will be my last comment on it. I really don't want to waste people's valuable time but at the same time I feel frustrated because I am still trying to understand the objections to my idea and I feel we may be making a mistake in how we consider the difference between the filepicker and filemanager. I am still worried I may not have got my point across. So let me get this off my chest so I can get on with coding. I really don't see why duplicating what the filepicker already does in the filemanager is a good idea. The filepicker is a great interface to pick one file. If we are dealing with one file it doesn't need to be 'managed'. As I have said I think the one file case is very common so it bares some consideration. Does anyone disagree that the filepicker is a better interface for picking one file? It is optimally simple and was designed to be similar to the single file uploader element that users are familiar with. How does duplicating functionality help with security and maintainability of code? The actual difference between the functionality of the two form elements is just how many files they deal with. They both deal with files in the draft files area. When there is an existing file the filepicker can already handle the display of an existing file. The code for copying the files to the draft area and merging files from the draft area back to the plug in files area is separate to the form elements themselves. This code can already be used with the filepicker element as well as the filemanager element, I think it can probably be used as is. The I think very elegant system for file upload to a plug in files area I understand as follows : 1. Populate form by preparing a draft files area with a full set of copies of file records for the files in the plug in files area. 2. User is allowed to manage files in draft files area. 3. Changes in files in draft files area is merged back into the plug in files area. Using the file picker we always should end up with exactly one file in the draft file area and if the code written for steps 1 and 3 that manages the populating of files in the file area and merging changes back into plug in files area is working properly it should not matter which form element we use for 2, either the filemanager to manage multiple files or the filepicker to pick one file.
          Hide
          Jamie Pratt added a comment -

          At the time I wrote all this I hadn't seen there was some rumblings of discontent amongst some serious Moodle users related to the ease of use of the new file system :

          http://moodle.org/mod/forum/discuss.php?d=171894#p794423

          and

          http://moodle.org/mod/forum/discuss.php?d=171894#p794271

          I think the new file system is a very cool piece of work. I love the way it uses content hashes and avoids duplicating files in the file storage and increases security. And all the thought that has gone into making it work properly with context and capabilities system.

          Show
          Jamie Pratt added a comment - At the time I wrote all this I hadn't seen there was some rumblings of discontent amongst some serious Moodle users related to the ease of use of the new file system : http://moodle.org/mod/forum/discuss.php?d=171894#p794423 and http://moodle.org/mod/forum/discuss.php?d=171894#p794271 I think the new file system is a very cool piece of work. I love the way it uses content hashes and avoids duplicating files in the file storage and increases security. And all the thought that has gone into making it work properly with context and capabilities system.
          Hide
          Marina Glancy added a comment -

          It is already possible to limit the number of files in filemanager to 1.
          At the same time I agree that "delete" button in filepicker would make sense

          Triaging this issue, thanks for the report.

          Show
          Marina Glancy added a comment - It is already possible to limit the number of files in filemanager to 1. At the same time I agree that "delete" button in filepicker would make sense Triaging this issue, thanks for the report.

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated: