Responses to the comments are included inline below Craig's. On 7/17/07, Craig Andera <[EMAIL PROTECTED]> wrote: > > > Comments on this patch: > > 1) Edits to flexwiki.config should actually be made to > flexwiki.config.template. To allow developers to mess around with the > .config files on their machines but still maintain a "clean" copy that goes > in the distribution zip, during the build we throw away the .config files > and use the .config.template files instead.
Understood I will correct that in the next version of the patch that I send > > 2) In the great religious debate between whether braces are required > for single-line blocks…they are. J Minor point. I was looking through the existing code and found it both ways in different places... will fix. > > 3) ShowEditPageZ is a pretty ambiguous name I know I had no idea what > it did without reading the code body. A name like ShowEditPageFinal (or > similar) would be much better. I know it was just that I diliked putting the Final code section after the Middle code section, but you are right. Will revise the name in the next version. > > 4) Methods should be sorted alphabetically within a protection level. I missed the last 2 methods I guess. Will fix > > 5) You're slurping the whole file into memory before writing it out. > While I'm no fan of premature optimization, this does seem like a good > vector for a DoS attack. Then again, it's no different than the wikitext we > let people post… Actually the version without Attachment also has the same behaviour. The normal Edit form sends all of the original text and all of the current text. I needed to mimic this behaviour. If I have time I would replace this with an MD5 or SHA1 hash value, or use the HtmlControl variables in the pagebehind. > > 6) You're using string concatenation to put path names together > (ReturnDirectoryToWriteTo). Use Path.Combine instead. Will Fix. > > 7) What's the purpose of putting different file types in different > directories? Is it just to organize things on the server, i.e. convenience? That was the way the original upload code from the FlexWiki site did it so I kept it. This is available for discussion. There is no real reason in today's file systems. also if I was to make it possible to store the uploads in SQL Server it would all go in the same table. > > 8) You really limit the system to nine versions of a file? Is there a > reason for that? Again that was what the original code did. I can increase to 99 easily, it is more difficult to make it unlimited, but not impossible. > > 9) On my machine, when I click "Show Attachment Controls", the right > border slides off the screen, making me scroll. That's a bit inconvenient. That happens in Firefox, at least 2.0.0.4. It does not happen in IE 7.0.5730.11. It is behaviour like this that drives web programmers nuts. I need more time to work out why it behaves this way or find a fix. I agree it is annoying (Note IE does not give any scrollbars and some of the controls will end up below the window, making it necessary to blindly navigate to these hidden controls by tabbing.) > > 10) When I tried to upload an image, I got a server error "The connection > was reset". The error is super cryptic and I haven't had a chance to dig > into what's causing it. I also get that error now. I will investigate and fix for next patch > > 11) Along the same lines, I'd like to see a switch in FlexWiki.config that > lets me turn off this feature entirely. I can see some administrators being > unwilling to deploy it given the liability issues that could ensue by giving > people a place to post arbitrary files. It can default to on, but we should > be able to switch it off. If the flexwiki.config does not contain the setting for ContentUpload (i don't have the exact name available), the Attachment controls should not display or be accessible. I agree this was not my first design choice, but to make so that the control is not output to the page requires a full redesign of the EditWiki page. The use of the construct <% DoPage(); %> in the aspx file it means that all controls are placed on the page during the PageRender event. The HtmlInputFile control _must_ be on the page at the PageLoad event, otherwise security mechanisms kick in and prevent data being passed to the pagebehind code. I almost started such a rewrite, but felt that would have been presumptuous. Maybe there is an alternate solution that I am not seeing? > > 12) The "Send" and "Create Attachment" buttons are confusing to me – it > seems like "Create Attachment" inserts WikiTalk, but it's not clear – both > buttons imply "upload" to me. > The Send button causes the file to be uploaded to the server. The Create Attachment emits the WikiTalk based on the input from the controls exposed by the chosen radio button for format. > > Because I was unable to get it working, I couldn't do much more review than > that. Do you have the feature working on a webserver somewhere in its > current form? I'd love to hear what other people think of it, too. > A working version is available at (this is one without any authentication controls): http://ods.dyndns.org/FlexWiki/default.aspx/OdsWiki/AttachmentPage.html the above page includes some discussion of the features and links to other sample pages and also template pages for new topics to use attachment features > If anyone wants access to a wiki with the same features but with authentication and access controls then they should go to: http://ods.dyndns.org/FlexWiki20/RegisterUser.aspx and then send me an email so that I activate the account and inform them that it has been activated. Whew. John Davidson ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ _______________________________________________ Flexwiki-users mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/flexwiki-users
