Uploaded image for project: '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

      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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            fred 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
            fred 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 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 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
            fred 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
            fred 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
            andyjdavis 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
            andyjdavis 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
            fred Frédéric Massart added a comment -

            Many thanks Andrew, pushing for integration !

            Show
            fred Frédéric Massart added a comment - Many thanks Andrew, pushing for integration !
            Hide
            damyon 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 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 Damyon Wiese added a comment -

            Failing on point 2.

            Show
            damyon Damyon Wiese added a comment - Failing on point 2.
            Hide
            fred 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
            fred 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 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 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
            fred 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
            fred 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 Damyon Wiese added a comment -

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

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

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            fred 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
            fred 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
            andyjdavis 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
            andyjdavis 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
            fred Frédéric Massart added a comment -

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

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

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

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

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

            Show
            fred Frédéric Massart added a comment - Thanks Eloy, I've added that in. Cheers!
            Hide
            stronk7 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
            stronk7 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 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 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 Damyon Wiese added a comment -

            Hi Fred,

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

            Thanks, Damyon

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

            Note: ran unit tests on oracle for this change.

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

            All works as expected

            Show
            dmonllao David Monllaó added a comment - All works as expected
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  14/May/13