Moodle
  1. Moodle
  2. MDL-37641

Prettier names when a file already exists

    Details

    • Testing Instructions:
      Hide
      1. Name a file 'MDL-37641.txt'
      2. Go to your privates files
      3. Upload the file
      4. Make sure it uploads ok
      5. Upload the file again
      6. Make sure you're asked to rename it to 'MDL-37641 (1).txt'
      7. Upload the same file again
      8. Make sure you're asked to rename it to 'MDL-37641 (2).txt'
      9. Drag and drop the same file
      10. Make sure you're asked to rename it to 'MDL-37641 (3).txt'
      11. Rename 'MDL-37641 (2).txt' to 'MDL-37641 (Moodle is great).txt'
      12. Upload the same file again
      13. Make sure you're asked to rename it to 'MDL-37641 (4).txt'
      Show
      Name a file ' MDL-37641 .txt' Go to your privates files Upload the file Make sure it uploads ok Upload the file again Make sure you're asked to rename it to ' MDL-37641 (1).txt' Upload the same file again Make sure you're asked to rename it to ' MDL-37641 (2).txt' Drag and drop the same file Make sure you're asked to rename it to ' MDL-37641 (3).txt' Rename ' MDL-37641 (2).txt' to ' MDL-37641 (Moodle is great).txt' Upload the same file again Make sure you're asked to rename it to ' MDL-37641 (4).txt'
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-37641-master
    • Rank:
      47334

      Description

      Improve the way the repository generates new file names when the file already exists. Currently a file will have _2 appended to it. So we can quickly get this:

      • test.txt
      • test_2.txt
      • test_2_2.txt

      The improvement could follow what most OS do:

      • test.txt
      • test (1).txt
      • test (2).txt

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          The proposed patch implements a logic to limit the number of loops as we are querying the database for each one. Also, I have deprecated the method append_suffix() as it has no utility.

          Show
          Frédéric Massart added a comment - The proposed patch implements a logic to limit the number of loops as we are querying the database for each one. Also, I have deprecated the method append_suffix() as it has no utility.
          Hide
          Marina Glancy added a comment -

          Fred, that will be awesome to implement! Btw I was always wondering why dndupload does not get the file name from server in a response but generates it itself?

          Show
          Marina Glancy added a comment - Fred, that will be awesome to implement! Btw I was always wondering why dndupload does not get the file name from server in a response but generates it itself?
          Hide
          Frédéric Massart added a comment -

          Because it has all the information it needs to, and querying the server for each file name taken will be very very consuming. I would have preferred to keep the logic in one place, but that won't be possible here.

          Show
          Frédéric Massart added a comment - Because it has all the information it needs to, and querying the server for each file name taken will be very very consuming. I would have preferred to keep the logic in one place, but that won't be possible here.
          Hide
          Andrew Davis added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [NA] Language
          [Y] Databases
          [Y] Testing
          [NA] Security
          [NA] Documentation
          [Y] Git
          [Y] Sanity check

          Fred, you need to write code with something wrong with it for me to find

          Submit for integration whenever you're ready.

          Show
          Andrew Davis added a comment - [Y] Syntax [Y] Output [Y] Whitespace [NA] Language [Y] Databases [Y] Testing [NA] Security [NA] Documentation [Y] Git [Y] Sanity check Fred, you need to write code with something wrong with it for me to find Submit for integration whenever you're ready.
          Hide
          Frédéric Massart added a comment -

          Many thanks Andrew, pushing for integration !

          Show
          Frédéric Massart added a comment - Many thanks Andrew, pushing for integration !
          Hide
          Damyon Wiese added a comment -

          Fred - 2 comments.

          1. Is it worth changing the naming convention from "blah_X.ext" to "blah (X).ext". It is more readable in a page, but will cause less readable URLs (encoding) and for existing sets of files we will now have:

          file.txt
          file_2.txt
          file_2_2.txt
          file (2).txt
          file (3).txt

          Which is a bit of a mess.
          (This is just a comment - welcome your opinion on this)

          2. DB performance - the patch will be no worse for performance than the code that was there before - but it is still ugly. I think we should add "get_unused_filename" to lib/filestorage/file_storage.php - where we can do a 'like' query to get all possible conflicting filenames at once and resolve a unique name.

          Show
          Damyon Wiese added a comment - Fred - 2 comments. 1. Is it worth changing the naming convention from "blah_X.ext" to "blah (X).ext". It is more readable in a page, but will cause less readable URLs (encoding) and for existing sets of files we will now have: file.txt file_2.txt file_2_2.txt file (2).txt file (3).txt Which is a bit of a mess. (This is just a comment - welcome your opinion on this) 2. DB performance - the patch will be no worse for performance than the code that was there before - but it is still ugly. I think we should add "get_unused_filename" to lib/filestorage/file_storage.php - where we can do a 'like' query to get all possible conflicting filenames at once and resolve a unique name.
          Hide
          Damyon Wiese added a comment -

          Failing on point 2.

          Show
          Damyon Wiese added a comment - Failing on point 2.
          Hide
          Frédéric Massart added a comment -

          Thanks Damyon,

          1. Well, I have noticed that most operating systems would append (1) to the file name and then increment it. As our users are real world users, reflecting the same behaviour than common Operating Systems made sense to me. Most users never use underscore in their names, though it's just me, but I think it makes sense.

          2. This issue was not meant to take care of any performance improvement, can you confirm that you want me to introduce a new method for this? If yes, I could also change repository::draftfile_exists() to something more efficient based on a new method in file_storage too. Isn't that out of the scope of this issue?

          I'm happy either way, just want to make sure I understand the protocol .

          Show
          Frédéric Massart added a comment - Thanks Damyon, 1. Well, I have noticed that most operating systems would append (1) to the file name and then increment it. As our users are real world users, reflecting the same behaviour than common Operating Systems made sense to me. Most users never use underscore in their names, though it's just me, but I think it makes sense. 2. This issue was not meant to take care of any performance improvement, can you confirm that you want me to introduce a new method for this? If yes, I could also change repository::draftfile_exists() to something more efficient based on a new method in file_storage too. Isn't that out of the scope of this issue? I'm happy either way, just want to make sure I understand the protocol .
          Hide
          Damyon Wiese added a comment -

          OK Fred,

          For 1 - the list of files will be ugly now when there are more than 2 - so it wont hurt to change the format.

          For 2 - We can create a new issue for that and fix it separately (but I think it needs fixing).

          Show
          Damyon Wiese added a comment - OK Fred, For 1 - the list of files will be ugly now when there are more than 2 - so it wont hurt to change the format. For 2 - We can create a new issue for that and fix it separately (but I think it needs fixing).
          Hide
          Frédéric Massart added a comment -

          Thanks Damyon, sending for integration again. I raised MDL-37793 to take care of the performance issues.

          Show
          Frédéric Massart added a comment - Thanks Damyon, sending for integration again. I raised MDL-37793 to take care of the performance issues.
          Hide
          Damyon Wiese added a comment -

          Np Fred - I'll pull this in on Monday (it's not urgent to rush the testing).

          Show
          Damyon Wiese added a comment - Np Fred - I'll pull this in on Monday (it's not urgent to rush the testing).
          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
          Damyon Wiese added a comment -

          Hi Fred,

          Looking at this again, I really don't like the naming when there are more than 100 files.

          I have 2 reasons for this:
          1. dndupload has it's own logic for resolving name clashes which will produce a different file name in this situation (Will always be "basename (X).jpg")
          2. the logic is no more or less likely to resolve name clashes given that the files would most likely be generated by this function anyway and are much harder to read/sort.

          Can you change this to always follow the "basename (X).jpg" pattern?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Fred, Looking at this again, I really don't like the naming when there are more than 100 files. I have 2 reasons for this: 1. dndupload has it's own logic for resolving name clashes which will produce a different file name in this situation (Will always be "basename (X).jpg") 2. the logic is no more or less likely to resolve name clashes given that the files would most likely be generated by this function anyway and are much harder to read/sort. Can you change this to always follow the "basename (X).jpg" pattern? Thanks, Damyon
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Frédéric Massart added a comment -

          Sending for peer review again.

          • I've create a new method in file_storage to guess the next available file name;
          • The repository method uses that new one;
          • I spotted an easy performance improvement in repository::draftfile_exists(), which I fixed;
          • I've added unit tests for the 3 methods.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Sending for peer review again. I've create a new method in file_storage to guess the next available file name; The repository method uses that new one; I spotted an easy performance improvement in repository::draftfile_exists(), which I fixed; I've added unit tests for the 3 methods. Cheers, Fred
          Hide
          Andrew Davis added a comment -

          Nice work. It looks great to me although I've said that before. The only minor niggle is in test_get_unused_filename(). At line 1409 you've got a single blank line and I'm not sure why. Unless I'm missing something either remove it or actually put in a blank line after each assert ie

          get_unused_filename()
          assert()

          get_unused_filename()
          assert()

          get_unused_filename()
          assert()

          Show
          Andrew Davis added a comment - Nice work. It looks great to me although I've said that before. The only minor niggle is in test_get_unused_filename(). At line 1409 you've got a single blank line and I'm not sure why. Unless I'm missing something either remove it or actually put in a blank line after each assert ie get_unused_filename() assert() get_unused_filename() assert() get_unused_filename() assert()
          Hide
          Frédéric Massart added a comment -

          Thanks Andrew, I've removed the extra line. Pushing for integration, cheers!

          Show
          Frédéric Massart added a comment - Thanks Andrew, I've removed the extra line. Pushing for integration, cheers!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tiny note: the deprecated function needs documentation (upgrade.txt).

          Show
          Eloy Lafuente (stronk7) added a comment - Tiny note: the deprecated function needs documentation (upgrade.txt).
          Hide
          Frédéric Massart added a comment -

          Thanks Eloy, I've added that in. Cheers!

          Show
          Frédéric Massart added a comment - Thanks Eloy, I've added that in. Cheers!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologies for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Damyon Wiese added a comment -

          Hi Fred,

          All looks good. This has been pushed to master now.

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Fred, All looks good. This has been pushed to master now. Thanks, Damyon
          Hide
          Damyon Wiese added a comment -

          Note: ran unit tests on oracle for this change.

          Show
          Damyon Wiese added a comment - Note: ran unit tests on oracle for this change.
          Hide
          David Monllaó added a comment -

          All works as expected

          Show
          David Monllaó added a comment - All works as expected
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Because

          A
          MARVELOUS
          A       U
          Z  YOU  P
          I  ARE  E
          N  PPL  R
          G       B
            TNKS! 
          

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: