Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#2641 closed enhancement (fixed)

[PATCH] ".DELETED" suffix only works on files, not directories

Reported by: fabio Owned by: leper
Priority: Nice to Have Milestone: Alpha 22
Component: Core engine Keywords: patch
Cc: Jan Wassenberg Patch:

Description

It would be useful for modding if a ".DELETED" suffixed file (and/or directory?) could be able to also disable a directory. Actually it only works for files.

Note that the directory removal should be done before adding mod directory (or file) with the same name.

Example: the mod ship a maps.DELETED file and a maps directory with some files. maps.DELETED should first applied removing public maps directory, then mod maps directory should be added.

Attachments (2)

vfs_DELETED_folders.patch (3.6 KB) - added by leper 5 years ago.
vfs_DELETED_folders+fix.patch (11.0 KB) - added by leper 4 years ago.
Support .DELETED, fix .DELETED when mounting a lower priority mod.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 years ago by leper

Milestone: Alpha 17Alpha 18

Changed 5 years ago by leper

Attachment: vfs_DELETED_folders.patch added

comment:2 Changed 5 years ago by leper

Keywords: patch review added
Summary: [modding] ".DELETED" suffix only works on files, not directories[PATCH] ".DELETED" suffix only works on files, not directories

For loose files the patch is trivial, but to properly handle .DELETED files in archives we need to explicitly order those first. Note that this will likely break if someone tries to build an archive by just zipping it up and not using the archive builder. (At least if they use .DELETED on folders and expect all their files to be there).

A possible enhancement to the patch would be checking if we find .DELETED files after having encountered a normal (not .DELETED) file in an archive. And even this might cause issues with having a .zip in side a .zip, where the first one removes a folder in the second, after the first one already started adding normal files. (Not unsolvable but quite some added complexity)

comment:3 Changed 4 years ago by fabio

A possible way to improve this would be to change the file extension of the mods from .zip to .pyr or something, maybe also requiring inside the .pyr a specific file (containing an hash of some file of the package) that needs to be available to properly load the mod. This way modders would be forced to generate the proper archive with the autobuilder which would then be sorted the right way.

In a future if/when we'll have a mod download server the modder will provide the zip and the server will automatically prepare the .pyr file.

Not the best solution, I agree, but if the sort order is important this way it could be forced.

The .DELETED dir would be useful for example to delete 0ad civs directory in pony mod, currently ponies can fight against human being, this is probably not wanted (since this mod already has its own 3 pony civs): http://youtu.be/TERel5Obie4

Last edited 4 years ago by fabio (previous) (diff)

comment:4 Changed 4 years ago by mimo

Milestone: Alpha 18Alpha 19

comment:5 Changed 4 years ago by leper

Keywords: review removed
Owner: set to leper
Status: newassigned

Removing review due to some possible issues that need some more investigation.

Mounting a mod with foo.DELETED (file or folder doesn't matter) with a lower priority than a mod with foo, could result in foo not being present.

How to nicely handle .DELETED files when encountering loose files or loose files and archives is also something I need to think about.

Requiring the specific order of .DELETED files in mods is something that will be kept in other iterations of this patch.

comment:6 Changed 4 years ago by leper

Milestone: Alpha 19Alpha 20

comment:7 Changed 4 years ago by leper

We should just handle .DELETED in the same way we handle replacing files. That is if the .DELETED file has a higher priority than the normal file we remove the normal file.

So in case someone tries to add a loose .DELETED file that should remove the normal file in an archive the timestamp of both files is a deciding factor (but this is consistent with file replacing).

For .DELETED folders we should just perform the replacement check for each file in that folder. This way we do not need the sorting of .DELETED files at all, since a normal file and a .DELETED file in the same archive should not result in no file, but the normal file. (Or it might depend on the timestamps, which means it is a bit broken)

Last edited 4 years ago by leper (previous) (diff)

Changed 4 years ago by leper

Support .DELETED, fix .DELETED when mounting a lower priority mod.

comment:8 Changed 4 years ago by leper

Cc: Jan Wassenberg added
Keywords: review added

This patch adds some tests (only work with the rest of the patch but with some small changes one can verify that the current .DELETED handling is broken as it ignores priority), and adds support for .DELETED for folders.

If a .DELETED file and a folder occur in the same mod the folder is created. .DELETED files only apply to lower priority mods. Since we have one mod in a user-writable folder for each enabled mod (mod selector) which is mounted with higher priority than the (probably archived) mod supporting .DELETED for the same priority sounds like a source of confusion and possibly bugs.

CCing Jan since he might use the VFS in some other project and some input would be nice too. (I'm not really sure about tne naming of the new functions.)

comment:9 Changed 3 years ago by Itms

Milestone: Alpha 20Alpha 21

comment:10 Changed 3 years ago by wraitii

Keywords: review removed

On windows 7 here. Seems to work correctly and passes the tests you wrote - which also look correct.

I think it's a little odd how you need a .deleted file and then you can add the folder instead of just naming the directory .deleted altogether but I guess it's far simpler that way.

Function names area little odd if you read the function itself but make sense in context of when they are used so it seems fine.

Think it can be committed.

comment:11 Changed 3 years ago by elexis

Keywords: review added

Thanks for the review, but I'll just keep it in the queue until it's committed, otherwise it might become forgotton.

comment:12 Changed 3 years ago by Itms

Keywords: rfc added; review removed

Move tickets from the review queue to the rfc one.

comment:13 Changed 3 years ago by elexis

Milestone: Alpha 21Alpha 22

Feature freeze in 2 days.

comment:14 Changed 3 years ago by wraitii

Resolution: fixed
Status: assignedclosed

In 18916:

Allow .Deleted to work on directories. Fixes #2641. Patch by leper.

comment:15 Changed 3 years ago by elexis

Keywords: rfc removed
Note: See TracTickets for help on using tickets.