-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Major refactoring of filesystemadaptor
The code here had got a bit broken by some PRs that I should have checked more carefully. I’ve done a major refactoring which will hopefully make it easier to understand, and fixes a number of problems: * Problem with eg .md tiddlers not being deleted correctly * Problem with Windows path separators not being usable within $:/config/FileSystemPaths on Windows * Problem with filename clashes not being detected correctly when saving to a different directory via $:/config/FileSystemPaths * Enables slashes within tiddler titles to be mapped into folders * Enables plain text files like .md and .css to be saved with .meta files instead of as .tid files (see #2558) * No longer replaces spaces with underscores As this is such a major update, I’d be grateful if Node.js users could give it a careful run through — in particular, you’ll need to try creating new tiddlers of various types and ensure that the expected files are created.
- Loading branch information
Showing
3 changed files
with
149 additions
and
82 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a short check with.
and get the following behaviour:
There are files
$__StoryList.tid and Draft of 'New Tiddler'.tid
plus 2 new unneeded directories with the same names, without .tid extension.3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I rename "draft of new tiddler" to "asdf" also an
asdf
directory is created and theDraft of 'New Tiddler'
director is not deleted.3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
system is win10 with git shell, which is MINGW64.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same test with win10 powershell -> same results.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodejs version is 4.5.0 both systems
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did test all different type fields. ... They all seem to create the right
file / metafile
combinations.3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to work as intended. But may be a side effect of the above.
I'll stop testing for now.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
files created in the explorer by hand.
then start the server and open localhost
produces wrong type. .. type: .sh type should be:
text/plain
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal
Since the type field is essential now, it should be set to
text/vnd.tiddlywiki
by default.If a tiddler is saved that way it should be saved as "asdf.tid"
If a tiddler has no type field it should be saved as "asdf" and "asdf.meta"
This is consistent and it is possible to create files without an extension.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmario
That works for me, could you try again? Any useful debugging output
Confirmed, am investigating now. I also verified that it works in 5.1.13
It is not intended to be essential (once the bugs are ironed out).
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed in d5b04c2
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did test it again with latest master
d5b04c2
... and the problem with the additional directories is gone. .. That's strange.You are right. This line alone works.
I actually did test this and missed the 3rd line :/
line 4 doesn't work, since
[!has[draft.of]]
grabs the "token" ... So that's a problem. We need to document this behaviour, a little bit better.3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs states:
but if the workflow is like this:
$:/config/FileSystemPaths
[tag[task]addprefix[mytasks/]]
... no other lines**So imo that's a problem. ** ... That's the exact workflow as I did test it yesterday.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following workflow also causes a problem:
$:/config/FileSystemPaths
[tag[task]addprefix[mytasks/]]
... no other linesasdf
tagged:task
and saveasdf
is moved tomytasks
-> OKasdf
and remove the tagmytasks
directoryIMO that's a bug
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also see an other problem.
Since eg: test.txt and test.txt.meta are 2 different files now, the save operation is not atomic anymore. ...
So we will run into all kind of strange consistency problems with systems, that use a caching mechanism to write files (OSes) and systems that watch file IO to tirgger actions. eg: file syncing software.
Those problems will be rare, practically impossible to reproduce and that's why they will be extremely hard to find.
just some thoughts
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmario
Hmmm it's a cascade that stops at the first match; do you mean that that is a problem, or the docs?
These settings affect the filename chosen for a new tiddler. Tiddlers that were originally loaded from a file at startup are always saved back to that file.
Note that this behaviour hasn't changed.
We've always saved a separate .meta file eg for JPGs. All that this change does is use the technique for all recognised file types.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. ... Both versions may cause unexpected behaviour. So let's stick to the actual behaviour and improve the docs.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I just forgot about that :)
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmario any help on docs gratefully received 😄
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I missed this behaviour. I thought images created with TW have been saved as
.tid
files. ...3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried, that users have completely wrong expectations about this feature. So we may label it as: "Experts use only!" at the moment.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until this commit, only .JPG files were saved as separate files with a .meta file. That commit introduced a few other problems, but broadened the support to all binary files.
Well, if there's a specific misunderstanding that we can anticipate, the docs could provide a cautionary note. Your example above might be a good example to include in the docs.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jermolene
How can we switch off the 2 files feature. I personally don't want to deal with 2 different files. .. at least not all the time.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd have to make it configurable... 😅
There are many plausible options: some people might like to switch to JSON instead of .tid...
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantages are quite compelling, though. It means that one gets syntax highlighting when working on CSS/JS/HTML tiddler files in an external editor, for example.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. ... but it also opens an other attack vector: eg: test.cmd, test.bat, test.ps1 are executables in windows systems. .. and if test.ps1 was created by the user or node.js on behalf of the user, there are almost no security checks.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pmario I was going to say "there are currently no executable file types registered in boot.js, so files like test.cmd, test.bat, test.ps1 will be created as .tid files", but then I checked and found that they are erroneously getting created as test.cmd.txt etc. I'll investigate and fix.
3708f6c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it is the expected behaviour; the files import as 'text/plain' and so are exported as .tid files.