getListing exclude deleted items

[michielbdejong]

I changed my mind about the fix for #652, for two reasons:

• I ran into problems (see below)
• Although it would be technically possible to change our data format or add extra routines to overcome these problems, I don't have time for that now to do that before the -beta3 release (and I don't think anybody else does, either)
• my original argument that it's not efficient to do all these extra lookups just for the sake of it.

I think we should do it this way. We already have a breaking change in the getListing interface anyway, so we may as well simplify it this way.

People can retrieve Content-Type information with getFile or getObject. I don't think any apps are currently using the other two pieces of metadata (ETag and Content-Length) now. In fact, I think most apps are not even using the Content-Type. For JSON-data it's irrelevant, and we can be sure no apps are looking at the Content-Type of binary documents, because -beta3 will be the first release in which retrieving binary documents actually works at all. :)

I wouldn't be opposed to adding a getMetaData function, though. But this will only start to make proper sense starting December 2014, when all servers are guaranteed to support HEAD requests.

For completeness, these are the problems I ran into:

• the metadata is now stored at the item node, no longer at the folder node. This means that you would have to retrieve all items from IndexedDB to collect all this info. In itself, this is doable. That's why I agreed to it at first.
• in 'ALL' caching strategy, when a folder has been retrieved but one or more items have not yet, we already store the item's metadata in its 'remote' version in IndexedDB. This is what sync.markChildren is for. So that's where this metadata can be found. So far, so good.
• in the 'SEEN' and 'FLUSH' caching strategies, we don't store this. The existence of a 'remote' version signals that it needs to be retrieved, and the way we stop SEEN and FLUSH from retrieving child nodes recursively, is by storing remote metadata only for existing nodes, unless the caching strategy is ALL https://github.com/remotestorage/remotestorage.js/blob/master/src/sync.js#L551-L553
• of course it's possible to change this. We could change this to always store the remote, and then move the check for the caching strategy to sync.needsFetch https://github.com/remotestorage/remotestorage.js/blob/master/src/sync.js#L271. We would then also need to change cachinglayer.getLatest to return the remote metadata if it exists. And if we do it properly, we should actually also move this metadata to the 'common' version in sync.autoMergeDocument, unless the caching strategy is 'ALL', because there's no reason to leave that in the 'remote' version if we're not going to also fetch the body. But currently when creating a remote version with only metadata, we're postponing the change event until the body is also available. But that would be impossible if we're storing metadata of nodes we're not planning to fetch, so then I can't really oversee if these changes may also affect how and when remote change events are triggered.
• in short, adding Content-Type information into the getListing results would touch the code in lots of places, with a lot of opportunity for introducing new bugs. I hope that explains it.

[raucao]

People can retrieve Content-Type information with getFile or getObject. I don't think any apps are currently using the other two pieces of metadata (ETag and Content-Length) now. In fact, I think most apps are not even using the Content-Type. For JSON-data it's irrelevant, and we can be sure no apps are looking at the Content-Type of binary documents, because -beta3 will be the first release in which retrieving binary documents actually works at all. :)

I'm waiting for this since we added it to the spec. "Nobody currently uses it" is a complete non-argument, when people can't currently use it with this library, since it was added to the spec, and while people are waiting for it.

I'm still waiting for it for the initial reasons we added it to the spec.

If you just don't want to do it now, be honest and keep the bug open, but don't say we intentionally leave it out of the library, although we added it to the spec in -02, because nobody uses it.

Thanks.

[michielbdejong]

ok, you're right. This is only a half-fix of the problem originally described in #652, so I split off the part of #652 that is not fixed by this PR into #721, and that issue should then stay open.

[raucao]

Cool, thanks!

[michielbdejong]

So that's a +1 then, right?

@galfert or @Ragnis, can you do the second review?

[raucao]

How is that a +1? I haven't reviewed yet.

[raucao]

This doesn't make sense. You just put a random object in a function in usable example code.

[michielbdejong]

fixed (if I understood your objection correctly)

[raucao]

Yes, now it's usable, but I found the old example better somehow. But that's just me.

[raucao]

+1

[michielbdejong]

Thanks!

@galfert or @Ragnis, please do the second review so we can close one more blocker bug.

[Ragnis]

+1

[michielbdejong]

Thanks!

Merged in 10187bb