Re: [xwiki-devs] [xwiki-notifications] r18638 - in xoffice/trunk/xword: Wiki/src/main/resources/MSOffice XWikiLib XWikiLib/Clients XWikiLib/XWiki XWord
fciubotaru (SVN) wrote:
Author: fciubotaru Date: 2009-04-13 21:19:15 +0200 (Mon, 13 Apr 2009) New Revision: 18638
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs Modified: xoffice/trunk/xword/XWikiLib/Clients/IXWikiClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiHTTPClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiXMLRPCClient.cs xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs xoffice/trunk/xword/XWikiLib/XWikiLib.csproj xoffice/trunk/xword/XWord/AddinActions.cs Log: XOFFICE-39 Save and send data using the encoding of the wiki. Patch submitted by Cristina Scheau, reviewed by Florin Ciubotaru.
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml =================================================================== --- xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml (rev 0) +++ xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,61 @@ +<?xml version="1.0" encoding="ISO-8859-15"?> + +<xwikidoc> +<web>MSOffice</web> +<name>GetEncodingService</name> +<language></language> +<defaultLanguage>en</defaultLanguage> +<translation>0</translation> +<parent></parent> +<creator>XWiki.Admin</creator> +<author>XWiki.Admin</author> +<customClass></customClass> +<contentAuthor>XWiki.Admin</contentAuthor> +<creationDate>1239491696000</creationDate> +<date>1239493018000</date> +<contentUpdateDate>1239493018000</contentUpdateDate> +<version>2.1</version>
Normally we set the document version to 1.1...
+<title>GetEncodingsService</title> +<template></template> +<defaultTemplate></defaultTemplate> +<validationScript></validationScript> +<comment></comment> +<minorEdit>false</minorEdit> +<syntaxId>xwiki/1.0</syntaxId> +<hidden>false</hidden>
... and we remove the tag object when committing wiki documents.
+<object> +<class> +<name>XWiki.TagClass</name> +<customClass></customClass> +<customMapping></customMapping> +<defaultViewSheet></defaultViewSheet> +<defaultEditSheet></defaultEditSheet> +<defaultWeb></defaultWeb> +<nameField></nameField> +<validationScript></validationScript> +<tags> +<cache>0</cache> +<displayType>input</displayType> +<multiSelect>1</multiSelect> +<name>tags</name> +<number>1</number> +<prettyName>Tags</prettyName> +<relationalStorage>1</relationalStorage> +<separator> </separator> +<separators> ,|</separators> +<size>30</size> +<unmodifiable>0</unmodifiable> +<values></values> +<classType>com.xpn.xwiki.objects.classes.StaticListClass</classType> +</tags> +</class> +<name>MSOffice.GetEncodingService</name> +<number>0</number> +<className>XWiki.TagClass</className> +<guid>9fd947e4-3ebe-41b0-ba90-2cd80b3c7df3</guid> +<property> +<tags/> +</property> +</object> +<content>$xwiki.getEncoding()</content> +</xwikidoc> \ No newline at end of file
Added: xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs (rev 0) +++ xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,11 @@
Shouldn't there be a license header in here? I've looked, and it seems that most XWord source files don't have such a header, which is wrong.
+using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; + +namespace XWiki.Clients +{ + class InvalidEncodingNameException : Exception + { + } +}
Modified: xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 18:25:16 UTC (rev 18637) +++ xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -16,6 +16,7 @@ static String wikiStructureURL = "/xwiki/bin/view/MSOffice/StructureService?xpage=plain"; static String attachmentServiceURL = "/xwiki/bin/view/MSOffice/AttachmentService?xpage=plain"; static String protectedPagesURL = "/xwiki/bin/view/MSOffice/ProtectedPages?xpage=plain"; + static String getEncoding = "/xwiki/bin/view/MSOffice/GetEncodingService?xpage=plain";
I don't like this. URLs are pretty configurable, and this hardcoding makes it impossible for XWord to work with a non-default installation of XWiki. There should be a configurable prefix, which is used to construct the final URLs. I've also looked at this class as a whole, and it doesn't look like a Factory. Using the wrong name for a thing is bad, since somebody new to the code will waste important time trying to figure out why he doesn't see the factory in there. If this class will change in the future (once the communication mechanism is replaced) to behave like a factory, then you should write a class comment stating that although it doesn't look like a factory yet, it will soon be one.
/// <summary> /// Gets or sets the URL of the service that handles attachments. @@ -72,5 +73,15 @@ get { return protectedPagesURL; } set { protectedPagesURL = value; } } + + /// <summary> + /// Gets the encoding of the wiki. + /// </summary> + public static String GetEncoding + { + get { return getEncoding; }
Can you set a Get* member? Weird...
+ set { getEncoding = value; } + } + } }
-- Sergiu Dumitriu http://purl.org/net/sergiu/
Sergiu Dumitriu wrote:
fciubotaru (SVN) wrote:
Author: fciubotaru Date: 2009-04-13 21:19:15 +0200 (Mon, 13 Apr 2009) New Revision: 18638
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs Modified: xoffice/trunk/xword/XWikiLib/Clients/IXWikiClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiHTTPClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiXMLRPCClient.cs xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs xoffice/trunk/xword/XWikiLib/XWikiLib.csproj xoffice/trunk/xword/XWord/AddinActions.cs Log: XOFFICE-39 Save and send data using the encoding of the wiki. Patch submitted by Cristina Scheau, reviewed by Florin Ciubotaru.
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml =================================================================== --- xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml (rev 0) +++ xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,61 @@ +<?xml version="1.0" encoding="ISO-8859-15"?> + +<xwikidoc> +<web>MSOffice</web> +<name>GetEncodingService</name> +<language></language> +<defaultLanguage>en</defaultLanguage> +<translation>0</translation> +<parent></parent> +<creator>XWiki.Admin</creator> +<author>XWiki.Admin</author> +<customClass></customClass> +<contentAuthor>XWiki.Admin</contentAuthor> +<creationDate>1239491696000</creationDate> +<date>1239493018000</date> +<contentUpdateDate>1239493018000</contentUpdateDate> +<version>2.1</version>
Normally we set the document version to 1.1...
Didn't pay attention to this until now. I usually export without history, does that fix the problem? Or do we need to change this by hand?
+<title>GetEncodingsService</title> +<template></template> +<defaultTemplate></defaultTemplate> +<validationScript></validationScript> +<comment></comment> +<minorEdit>false</minorEdit> +<syntaxId>xwiki/1.0</syntaxId> +<hidden>false</hidden>
... and we remove the tag object when committing wiki documents.
Which tag?
+<object> +<class> +<name>XWiki.TagClass</name> +<customClass></customClass> +<customMapping></customMapping> +<defaultViewSheet></defaultViewSheet> +<defaultEditSheet></defaultEditSheet> +<defaultWeb></defaultWeb> +<nameField></nameField> +<validationScript></validationScript> +<tags> +<cache>0</cache> +<displayType>input</displayType> +<multiSelect>1</multiSelect> +<name>tags</name> +<number>1</number> +<prettyName>Tags</prettyName> +<relationalStorage>1</relationalStorage> +<separator> </separator> +<separators> ,|</separators> +<size>30</size> +<unmodifiable>0</unmodifiable> +<values></values> +<classType>com.xpn.xwiki.objects.classes.StaticListClass</classType> +</tags> +</class> +<name>MSOffice.GetEncodingService</name> +<number>0</number> +<className>XWiki.TagClass</className> +<guid>9fd947e4-3ebe-41b0-ba90-2cd80b3c7df3</guid> +<property> +<tags/> +</property> +</object> +<content>$xwiki.getEncoding()</content> +</xwikidoc> \ No newline at end of file
Added: xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs (rev 0) +++ xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,11 @@
Shouldn't there be a license header in here? I've looked, and it seems that most XWord source files don't have such a header, which is wrong.
I'm -1 on adding a license header to every file while developing .net. There are a lot of auto-generated source files that cannot contain it. Also, a class can be written across multiple files and a file can contain multiple classes. A class can have multiple vendors that package it in one or multiple assemblies dependent or independent from one another. It's hard to keep a consistency, this is why the convention I use is to have only one license header in the add-ins entry point file. If other add-ins will be created then each one will have it's license header in the entry point file, not matter if they will be a part of the same .net solution or not. But until then, there will be only one license file.
+using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; + +namespace XWiki.Clients +{ + class InvalidEncodingNameException : Exception + { + } +}
Modified: xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 18:25:16 UTC (rev 18637) +++ xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -16,6 +16,7 @@ static String wikiStructureURL = "/xwiki/bin/view/MSOffice/StructureService?xpage=plain"; static String attachmentServiceURL = "/xwiki/bin/view/MSOffice/AttachmentService?xpage=plain"; static String protectedPagesURL = "/xwiki/bin/view/MSOffice/ProtectedPages?xpage=plain"; + static String getEncoding = "/xwiki/bin/view/MSOffice/GetEncodingService?xpage=plain";
I don't like this. URLs are pretty configurable, and this hardcoding makes it impossible for XWord to work with a non-default installation of XWiki. There should be a configurable prefix, which is used to construct the final URLs.
I've also looked at this class as a whole, and it doesn't look like a Factory. Using the wrong name for a thing is bad, since somebody new to the code will waste important time trying to figure out why he doesn't see the factory in there. If this class will change in the future (once the communication mechanism is replaced) to behave like a factory, then you should write a class comment stating that although it doesn't look like a factory yet, it will soon be one.
Yes, that was supposed to be a factory, but this entire thing about using wiki pages as services is wrong, so it will be dropped. It's already around a lot more then planned.
/// <summary> /// Gets or sets the URL of the service that handles attachments. @@ -72,5 +73,15 @@ get { return protectedPagesURL; } set { protectedPagesURL = value; } } + + /// <summary> + /// Gets the encoding of the wiki. + /// </summary> + public static String GetEncoding + { + get { return getEncoding; }
Can you set a Get* member? Weird...
Bad naming of a property made by a java programmer. Sorry, I didn't notice it when I reviewed the patch. I'll fix it in one of the next commits. The correct name is just "Encoding".
+ set { getEncoding = value; } + } + } }
This is the first code review XWord had in months of develpement. The more the better.
Florin Ciubotaru wrote:
Sergiu Dumitriu wrote:
fciubotaru (SVN) wrote:
Author: fciubotaru Date: 2009-04-13 21:19:15 +0200 (Mon, 13 Apr 2009) New Revision: 18638
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs Modified: xoffice/trunk/xword/XWikiLib/Clients/IXWikiClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiHTTPClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiXMLRPCClient.cs xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs xoffice/trunk/xword/XWikiLib/XWikiLib.csproj xoffice/trunk/xword/XWord/AddinActions.cs Log: XOFFICE-39 Save and send data using the encoding of the wiki. Patch submitted by Cristina Scheau, reviewed by Florin Ciubotaru.
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml =================================================================== --- xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml (rev 0) +++ xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,61 @@ +<?xml version="1.0" encoding="ISO-8859-15"?> + +<xwikidoc> +<web>MSOffice</web> +<name>GetEncodingService</name> +<language></language> +<defaultLanguage>en</defaultLanguage> +<translation>0</translation> +<parent></parent> +<creator>XWiki.Admin</creator> +<author>XWiki.Admin</author> +<customClass></customClass> +<contentAuthor>XWiki.Admin</contentAuthor> +<creationDate>1239491696000</creationDate> +<date>1239493018000</date> +<contentUpdateDate>1239493018000</contentUpdateDate> +<version>2.1</version>
Normally we set the document version to 1.1...
Didn't pay attention to this until now. I usually export without history, does that fix the problem? Or do we need to change this by hand?
I don't know about other, but I do it by hand.
+<title>GetEncodingsService</title> +<template></template> +<defaultTemplate></defaultTemplate> +<validationScript></validationScript> +<comment></comment> +<minorEdit>false</minorEdit> +<syntaxId>xwiki/1.0</syntaxId> +<hidden>false</hidden>
... and we remove the tag object when committing wiki documents.
Which tag?
This one starting here:
+<object> +<class> +<name>XWiki.TagClass</name> +<customClass></customClass> +<customMapping></customMapping> +<defaultViewSheet></defaultViewSheet> +<defaultEditSheet></defaultEditSheet> +<defaultWeb></defaultWeb> +<nameField></nameField> +<validationScript></validationScript> +<tags> +<cache>0</cache> +<displayType>input</displayType> +<multiSelect>1</multiSelect> +<name>tags</name> +<number>1</number> +<prettyName>Tags</prettyName> +<relationalStorage>1</relationalStorage> +<separator> </separator> +<separators> ,|</separators> +<size>30</size> +<unmodifiable>0</unmodifiable> +<values></values> +<classType>com.xpn.xwiki.objects.classes.StaticListClass</classType> +</tags> +</class> +<name>MSOffice.GetEncodingService</name> +<number>0</number> +<className>XWiki.TagClass</className> +<guid>9fd947e4-3ebe-41b0-ba90-2cd80b3c7df3</guid> +<property> +<tags/> +</property> +</object>
and ending here.
+<content>$xwiki.getEncoding()</content> +</xwikidoc> \ No newline at end of file
Added: xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs (rev 0) +++ xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,11 @@
Shouldn't there be a license header in here? I've looked, and it seems that most XWord source files don't have such a header, which is wrong.
I'm -1 on adding a license header to every file while developing .net. There are a lot of auto-generated source files that cannot contain it. Also, a class can be written across multiple files and a file can contain multiple classes. A class can have multiple vendors that package it in one or multiple assemblies dependent or independent from one another. It's hard to keep a consistency, this is why the convention I use is to have only one license header in the add-ins entry point file. If other add-ins will be created then each one will have it's license header in the entry point file, not matter if they will be a part of the same .net solution or not. But until then, there will be only one license file.
OK. Fine by me.
+using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; + +namespace XWiki.Clients +{ + class InvalidEncodingNameException : Exception + { + } +}
Modified: xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 18:25:16 UTC (rev 18637) +++ xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -16,6 +16,7 @@ static String wikiStructureURL = "/xwiki/bin/view/MSOffice/StructureService?xpage=plain"; static String attachmentServiceURL = "/xwiki/bin/view/MSOffice/AttachmentService?xpage=plain"; static String protectedPagesURL = "/xwiki/bin/view/MSOffice/ProtectedPages?xpage=plain"; + static String getEncoding = "/xwiki/bin/view/MSOffice/GetEncodingService?xpage=plain";
I don't like this. URLs are pretty configurable, and this hardcoding makes it impossible for XWord to work with a non-default installation of XWiki. There should be a configurable prefix, which is used to construct the final URLs.
I've also looked at this class as a whole, and it doesn't look like a Factory. Using the wrong name for a thing is bad, since somebody new to the code will waste important time trying to figure out why he doesn't see the factory in there. If this class will change in the future (once the communication mechanism is replaced) to behave like a factory, then you should write a class comment stating that although it doesn't look like a factory yet, it will soon be one.
Yes, that was supposed to be a factory, but this entire thing about using wiki pages as services is wrong, so it will be dropped. It's already around a lot more then planned.
/// <summary> /// Gets or sets the URL of the service that handles attachments. @@ -72,5 +73,15 @@ get { return protectedPagesURL; } set { protectedPagesURL = value; } } + + /// <summary> + /// Gets the encoding of the wiki. + /// </summary> + public static String GetEncoding + { + get { return getEncoding; }
Can you set a Get* member? Weird...
Bad naming of a property made by a java programmer. Sorry, I didn't notice it when I reviewed the patch. I'll fix it in one of the next commits. The correct name is just "Encoding".
+ set { getEncoding = value; } + } + } }
This is the first code review XWord had in months of develpement. The more the better.
I usually stay out of early projects, since the code tends to move pretty fast, and in pretty big chunks, thus it's hard to track. As time allows, I'll do more code reviews from now on. -- Sergiu Dumitriu http://purl.org/net/sergiu/
Sergiu Dumitriu wrote:
Florin Ciubotaru wrote:
Sergiu Dumitriu wrote:
fciubotaru (SVN) wrote:
Author: fciubotaru Date: 2009-04-13 21:19:15 +0200 (Mon, 13 Apr 2009) New Revision: 18638
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs Modified: xoffice/trunk/xword/XWikiLib/Clients/IXWikiClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiHTTPClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiXMLRPCClient.cs xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs xoffice/trunk/xword/XWikiLib/XWikiLib.csproj xoffice/trunk/xword/XWord/AddinActions.cs Log: XOFFICE-39 Save and send data using the encoding of the wiki. Patch submitted by Cristina Scheau, reviewed by Florin Ciubotaru.
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml =================================================================== --- xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml (rev 0) +++ xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,61 @@ +<?xml version="1.0" encoding="ISO-8859-15"?> + +<xwikidoc> +<web>MSOffice</web> +<name>GetEncodingService</name> +<language></language> +<defaultLanguage>en</defaultLanguage> +<translation>0</translation> +<parent></parent> +<creator>XWiki.Admin</creator> +<author>XWiki.Admin</author> +<customClass></customClass> +<contentAuthor>XWiki.Admin</contentAuthor> +<creationDate>1239491696000</creationDate> +<date>1239493018000</date> +<contentUpdateDate>1239493018000</contentUpdateDate> +<version>2.1</version>
Normally we set the document version to 1.1...
Didn't pay attention to this until now. I usually export without history, does that fix the problem? Or do we need to change this by hand?
I don't know about other, but I do it by hand.
+<title>GetEncodingsService</title> +<template></template> +<defaultTemplate></defaultTemplate> +<validationScript></validationScript> +<comment></comment> +<minorEdit>false</minorEdit> +<syntaxId>xwiki/1.0</syntaxId> +<hidden>false</hidden>
... and we remove the tag object when committing wiki documents.
Which tag?
This one starting here:
Yes, I actually saw it. Just hoped I don't need to change it by hand(used with VS that usually does it all). Hopefully I'll drop this soon.
+<object> +<class> +<name>XWiki.TagClass</name> +<customClass></customClass> +<customMapping></customMapping> +<defaultViewSheet></defaultViewSheet> +<defaultEditSheet></defaultEditSheet> +<defaultWeb></defaultWeb> +<nameField></nameField> +<validationScript></validationScript> +<tags> +<cache>0</cache> +<displayType>input</displayType> +<multiSelect>1</multiSelect> +<name>tags</name> +<number>1</number> +<prettyName>Tags</prettyName> +<relationalStorage>1</relationalStorage> +<separator> </separator> +<separators> ,|</separators> +<size>30</size> +<unmodifiable>0</unmodifiable> +<values></values> +<classType>com.xpn.xwiki.objects.classes.StaticListClass</classType> +</tags> +</class> +<name>MSOffice.GetEncodingService</name> +<number>0</number> +<className>XWiki.TagClass</className> +<guid>9fd947e4-3ebe-41b0-ba90-2cd80b3c7df3</guid> +<property> +<tags/> +</property> +</object>
and ending here.
+<content>$xwiki.getEncoding()</content> +</xwikidoc> \ No newline at end of file
Added: xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs (rev 0) +++ xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,11 @@
Shouldn't there be a license header in here? I've looked, and it seems that most XWord source files don't have such a header, which is wrong.
I'm -1 on adding a license header to every file while developing .net. There are a lot of auto-generated source files that cannot contain it. Also, a class can be written across multiple files and a file can contain multiple classes. A class can have multiple vendors that package it in one or multiple assemblies dependent or independent from one another. It's hard to keep a consistency, this is why the convention I use is to have only one license header in the add-ins entry point file. If other add-ins will be created then each one will have it's license header in the entry point file, not matter if they will be a part of the same .net solution or not. But until then, there will be only one license file.
OK. Fine by me.
+using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; + +namespace XWiki.Clients +{ + class InvalidEncodingNameException : Exception + { + } +}
Modified: xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 18:25:16 UTC (rev 18637) +++ xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -16,6 +16,7 @@ static String wikiStructureURL = "/xwiki/bin/view/MSOffice/StructureService?xpage=plain"; static String attachmentServiceURL = "/xwiki/bin/view/MSOffice/AttachmentService?xpage=plain"; static String protectedPagesURL = "/xwiki/bin/view/MSOffice/ProtectedPages?xpage=plain"; + static String getEncoding = "/xwiki/bin/view/MSOffice/GetEncodingService?xpage=plain";
I don't like this. URLs are pretty configurable, and this hardcoding makes it impossible for XWord to work with a non-default installation of XWiki. There should be a configurable prefix, which is used to construct the final URLs.
I've also looked at this class as a whole, and it doesn't look like a Factory. Using the wrong name for a thing is bad, since somebody new to the code will waste important time trying to figure out why he doesn't see the factory in there. If this class will change in the future (once the communication mechanism is replaced) to behave like a factory, then you should write a class comment stating that although it doesn't look like a factory yet, it will soon be one.
Yes, that was supposed to be a factory, but this entire thing about using wiki pages as services is wrong, so it will be dropped. It's already around a lot more then planned.
/// <summary> /// Gets or sets the URL of the service that handles attachments. @@ -72,5 +73,15 @@ get { return protectedPagesURL; } set { protectedPagesURL = value; } } + + /// <summary> + /// Gets the encoding of the wiki. + /// </summary> + public static String GetEncoding + { + get { return getEncoding; }
Can you set a Get* member? Weird...
Bad naming of a property made by a java programmer. Sorry, I didn't notice it when I reviewed the patch. I'll fix it in one of the next commits. The correct name is just "Encoding".
+ set { getEncoding = value; } + } + } }
This is the first code review XWord had in months of develpement. The more the better.
I usually stay out of early projects, since the code tends to move pretty fast, and in pretty big chunks, thus it's hard to track. As time allows, I'll do more code reviews from now on.
That would be great. Thanks :)
Florin Ciubotaru wrote:
Sergiu Dumitriu wrote:
Florin Ciubotaru wrote:
Sergiu Dumitriu wrote:
fciubotaru (SVN) wrote:
Author: fciubotaru Date: 2009-04-13 21:19:15 +0200 (Mon, 13 Apr 2009) New Revision: 18638
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs Modified: xoffice/trunk/xword/XWikiLib/Clients/IXWikiClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiHTTPClient.cs xoffice/trunk/xword/XWikiLib/Clients/XWikiXMLRPCClient.cs xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs xoffice/trunk/xword/XWikiLib/XWikiLib.csproj xoffice/trunk/xword/XWord/AddinActions.cs Log: XOFFICE-39 Save and send data using the encoding of the wiki. Patch submitted by Cristina Scheau, reviewed by Florin Ciubotaru.
Added: xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml =================================================================== --- xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml (rev 0) +++ xoffice/trunk/xword/Wiki/src/main/resources/MSOffice/GetEncodingService.xml 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,61 @@ +<?xml version="1.0" encoding="ISO-8859-15"?> + +<xwikidoc> +<web>MSOffice</web> +<name>GetEncodingService</name> +<language></language> +<defaultLanguage>en</defaultLanguage> +<translation>0</translation> +<parent></parent> +<creator>XWiki.Admin</creator> +<author>XWiki.Admin</author> +<customClass></customClass> +<contentAuthor>XWiki.Admin</contentAuthor> +<creationDate>1239491696000</creationDate> +<date>1239493018000</date> +<contentUpdateDate>1239493018000</contentUpdateDate> +<version>2.1</version>
Normally we set the document version to 1.1...
Didn't pay attention to this until now. I usually export without history, does that fix the problem? Or do we need to change this by hand?
I don't know about other, but I do it by hand.
+<title>GetEncodingsService</title> +<template></template> +<defaultTemplate></defaultTemplate> +<validationScript></validationScript> +<comment></comment> +<minorEdit>false</minorEdit> +<syntaxId>xwiki/1.0</syntaxId> +<hidden>false</hidden>
... and we remove the tag object when committing wiki documents.
Which tag?
This one starting here:
Yes, I actually saw it. Just hoped I don't need to change it by hand(used with VS that usually does it all). Hopefully I'll drop this soon.
For the object, I don't think you must do it by hand, I think removing the tag object (attached to the page in the wiki by default upon creation) before exporting it should have the same result. But the version I think you have not much choice but change by hand. Happy coding, Anca
+<object> +<class> +<name>XWiki.TagClass</name> +<customClass></customClass> +<customMapping></customMapping> +<defaultViewSheet></defaultViewSheet> +<defaultEditSheet></defaultEditSheet> +<defaultWeb></defaultWeb> +<nameField></nameField> +<validationScript></validationScript> +<tags> +<cache>0</cache> +<displayType>input</displayType> +<multiSelect>1</multiSelect> +<name>tags</name> +<number>1</number> +<prettyName>Tags</prettyName> +<relationalStorage>1</relationalStorage> +<separator> </separator> +<separators> ,|</separators> +<size>30</size> +<unmodifiable>0</unmodifiable> +<values></values> +<classType>com.xpn.xwiki.objects.classes.StaticListClass</classType> +</tags> +</class> +<name>MSOffice.GetEncodingService</name> +<number>0</number> +<className>XWiki.TagClass</className> +<guid>9fd947e4-3ebe-41b0-ba90-2cd80b3c7df3</guid> +<property> +<tags/> +</property> +</object>
and ending here.
+<content>$xwiki.getEncoding()</content> +</xwikidoc> \ No newline at end of file
Added: xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs (rev 0) +++ xoffice/trunk/xword/XWikiLib/Clients/InvalidEncodingNameException.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -0,0 +1,11 @@
Shouldn't there be a license header in here? I've looked, and it seems that most XWord source files don't have such a header, which is wrong.
I'm -1 on adding a license header to every file while developing .net. There are a lot of auto-generated source files that cannot contain it. Also, a class can be written across multiple files and a file can contain multiple classes. A class can have multiple vendors that package it in one or multiple assemblies dependent or independent from one another. It's hard to keep a consistency, this is why the convention I use is to have only one license header in the add-ins entry point file. If other add-ins will be created then each one will have it's license header in the entry point file, not matter if they will be a part of the same .net solution or not. But until then, there will be only one license file.
OK. Fine by me.
+using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; + +namespace XWiki.Clients +{ + class InvalidEncodingNameException : Exception + { + } +}
Modified: xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs =================================================================== --- xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 18:25:16 UTC (rev 18637) +++ xoffice/trunk/xword/XWikiLib/XWiki/XWikiURLFactory.cs 2009-04-13 19:19:15 UTC (rev 18638) @@ -16,6 +16,7 @@ static String wikiStructureURL = "/xwiki/bin/view/MSOffice/StructureService?xpage=plain"; static String attachmentServiceURL = "/xwiki/bin/view/MSOffice/AttachmentService?xpage=plain"; static String protectedPagesURL = "/xwiki/bin/view/MSOffice/ProtectedPages?xpage=plain"; + static String getEncoding = "/xwiki/bin/view/MSOffice/GetEncodingService?xpage=plain";
I don't like this. URLs are pretty configurable, and this hardcoding makes it impossible for XWord to work with a non-default installation of XWiki. There should be a configurable prefix, which is used to construct the final URLs.
I've also looked at this class as a whole, and it doesn't look like a Factory. Using the wrong name for a thing is bad, since somebody new to the code will waste important time trying to figure out why he doesn't see the factory in there. If this class will change in the future (once the communication mechanism is replaced) to behave like a factory, then you should write a class comment stating that although it doesn't look like a factory yet, it will soon be one.
Yes, that was supposed to be a factory, but this entire thing about using wiki pages as services is wrong, so it will be dropped. It's already around a lot more then planned.
/// <summary> /// Gets or sets the URL of the service that handles attachments. @@ -72,5 +73,15 @@ get { return protectedPagesURL; } set { protectedPagesURL = value; } } + + /// <summary> + /// Gets the encoding of the wiki. + /// </summary> + public static String GetEncoding + { + get { return getEncoding; }
Can you set a Get* member? Weird...
Bad naming of a property made by a java programmer. Sorry, I didn't notice it when I reviewed the patch. I'll fix it in one of the next commits. The correct name is just "Encoding".
+ set { getEncoding = value; } + } + } }
This is the first code review XWord had in months of develpement. The more the better.
I usually stay out of early projects, since the code tends to move pretty fast, and in pretty big chunks, thus it's hard to track. As time allows, I'll do more code reviews from now on.
That would be great. Thanks :)
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
participants (3)
-
Anca Paula Luca -
Florin Ciubotaru -
Sergiu Dumitriu