On Friday 11 June 2010 23:58:31 Matthew Toseland wrote: > On Friday 11 June 2010 14:43:09 Matthew Toseland wrote: > > BEFORE RELEASING 1251, MUST FIX: > > - Internal error on persistent download for mp3 with the check filter flag > > enabled. This should be an unfilterable content error. > > My fault, fixed. > > > - WoT > > - Freetalk > > - Spider > > - XMLSpider > > - FlogHelper > > All done, thanks. Jars will be posted before 1251 is released. > > > - ContentFilter charset detection code MUST use readFully, or a loop > > achieving similar goals. read() doesn't read the full buffer. > > This is STILL not fixed. We use readFully() but we only read up to > available(). We should read the full buffer size specified by the > CharsetExtractor, up to the limit of the *SIZE OF THE BUCKET*. available() is > not very helpful here IMHO.
More issues that it would be good to fix before releasing a build with this code. This is all usability stuff which IMHO will make a difference to end-users' frustration levels (of course, filters for popular types, and straight-through decompression-to-filtering, will also help with usability; if any of these are big jobs we can postpone them but we must not forget them): 1) Error: Content sanitization failed: Unknown and potentially dangerous content type: application/octet-stream (The title of the warning page). This is redundant, too many words. IMHO we should just show Error: Unknown and ... 2) When downloading a file to disk, with an unrecognised MIME type, with filtering enabled, it now fails the download with a message explaining that it is not supported; this message is visible on the queue page. However, it is a *really* *long* message. All of the other failures are short messages like "New URI" or "Invalid URI: Invalid crypto algorithm". Okay, those aren't terribly helpful, but IMHO it is useful for it to be short, maybe with a link to a longer explanation. I'm not sure exactly how to handle this. IIRC GetFailed (the FCP mechanism which underlies the queue page) has an ExtraDescription field we could use for the long description. Then we just need a short description for the error itself? I think we already use this, assembling the error from the short description and then the long description after a colon ... this probably happens in FetchException ... hmmm. Worth thinking about anyway, if you can come up with a reasonably quick solution. I think we have short description and long description for specific error codes, this may be relevant? Of course the real solution is to write filters for the most popular content types e.g. this is an MP3. :) 3) There should be some way to retry the download with filtering turned off. Probably this should take the form of another button in the first column, or a check box ([ ] Turn off filter). In future when the queue page has been cleaned up, the first column will be just checkboxes, and the actions will be at the bottom, at which point we can have a checkbox at the bottom; but that is way out of scope for your Summer of Code project and I'm hoping Bombe will get around to it eventually. But the lack of any obvious ability to retry with filtering disabled, given that it was caused by an unsupported MIME type, is IMHO a serious usability issue which will cause a lot of end-user frustration. 4) The filter flag is enabled by default for download to disk from within fproxy. Even if we know ahead of time that the MIME type is one we don't support, and even if security levels are all set to LOW. This will cause people to have the above frustration. IMHO: - If the page the download button is on is itself a MIME type warning, the filtered checkbox should default to OFF. IIRC if we have a nontrivial MIME type it cannot change later on because ClientMetadata.mergeNoOverwrite() does not allow us to change a nontrivial MIME type; so as long as the MIME type is not empty and not application/octet-stream (which is the same thing iirc), if we show the user the warning and they accept it, that is the MIME type that we will have at the end of the fetch. However, USKs may throw a spanner in the works here... so it may still be useful to be able to record that we've shown the user the warning for type T and not to fail *IF* the output is of that type. - If the network security level and physical security level are set to LOW, the filtered checkbox should default to OFF. - Ideally, if we know both the size and the MIME type, we should combine the two warnings into a single page. That's long term, file a bug and forget about it. A shorter term solution might be to show the MIME type warning before we show the size warning, once we are fairly sure we know the MIME type (i.e. we detect a non-null non-application/octet-stream type in the ExpectedMIME event which fproxy picks up). > > > > Minor stuff: > > > > 15b6283e017a26dec96f35820028b84992b5260f > > - should pass the overridden mime type along in places where we generate > > expected mime type events as well. non-critical. > > - should maybe also pass it in other places where we set the expected mime > > type on FetchException's. again non-critical. > > > > 9c05c0b1d1840826d6e823dc7f9f46682a3afac8 > > (f0dddb70e787388f3a674e9530e44962ce43d1c3 improves a bit) > > - what about just calling getMessage() on FetchException? -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 197 bytes Desc: This is a digitally signed message part. URL: <https://emu.freenetproject.org/pipermail/devl/attachments/20100612/772a4dca/attachment.pgp>
