10 Nov
2003
10 Nov
'03
8:25 p.m.
Hi David,
as there was no SmallWiki meeting today I am giving the code-review I
prepared using e-mail else it gets out-dated. If you have any
questions, please ask.
- Get rid of #isKindOf:
- Don't emit html-tags manually using "html text:
'<b>Role:</b>'" as
this does not allow to render to another output format. Furthermore the
b-tag is depreciated and won't validate for HTML4 or XHTML. Either use
"html strong: 'Role:'" or much better "html spanClass:
'label' with:
'Role:'" instead.
- Don't emit html-characters manually using "html text:
'&nsbsp;'" as
this does not allow to render to another output format. Use "html
space" instead.
- It is much simpler to use "html cssClass: 'label'" instead of
"html
attributeAt: 'class' put: 'label'".
- Please don't use JavaScript, as it does not work on my machine
even-tough I have enabled it. It is easily possible to replace it with
server-side logic.
- Don't return associations of strings in AdminAction
class>>defaultMenu with tile and the url-postfix, but the actions
classes. All actions in SmallWiki are able to return their title and
know much better how to build their action-url than your code does.
- The class name of AdminAttachments doesn't seem to match what the
action is actually doing.
- AdminAttachments class>>generate_id: should be renamed to
AdminAttachments class>>generateId: as the underscore is not portable
to other Smalltalk dialects.
- Furthermore AdminAttachments class>>generate_id: should be moved to
Structure as it has nothing to do with the action-class and basically
works on a structure.
- Last but not least, you shouldn't need to use AdminAttachments
class>>generate_id: at all. As far as I understand you are building and
parsing this id to identify structures within a wiki. If you use
hidden-fields it is possible to pass objects from one action to the
other and you do not have to deal with strings, this is much simpler
and cleaner.
- I haven't implemented the read-accessor of User>>password
intentionally, because it is planned to store the password as a
hash-value instead of plain-text later on. This is a security issue to
have the password accessible like that and to render it when editing
users. It is the responsibility of the user-class to validate the
user-password using the message #validatePassword: and it should not be
accessible form outside.
- The following sections are some things that are basically reported by
SmallLint or things that I think could have been written simpler. I've
not tested the code I am suggesting, but it should work more or less:
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Structure>>rolesIncludes: aRole
"check if a role is contained in a roles collection"
roles notNil ifTrue: [^roles includes: aRole].
^false
==>
Structure>>rolesIncludes: aRole
^roles notNil and: [ roles includes: aRole ]
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Structure>>roleWithName: aRoleName
"returns a role with certain name"
self rolesDo: [:role | role name = aRoleName ifTrue: [^role]].
^nil
==>
Structure>>roleWithName: aString
self roles
detect: [ :each | each name = aString ]
ifNone: [ nil ]
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Structure>>selfAndAllChildren
"return all children and their children.... as collection
- at this point: just return self in a collection "
| collection |
(collection := OrderedCollection new) add: self.
^collection
==>
Structure>>withChildren
^OrderedCollection with: self
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
X>>foo
X bar
==>
X>>foo
self class bar
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
AdminAction class>>defaultSeparator
"return the default string separator
- this seperator is used to generate the input fields of the
form"
^String new writeStream tab contents
==>
AdminAction class>>defaultSeparator
^String with: Character tab
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
AdminAction>>cutStructure: aStructureToDelete ofSelfAndChildrenOf:
aStructure
"cut the indicated structure out of the tree, if the paste destination
was not in its subtree"
| errorStream myParents |
errorStream := String new writeStream.
myParents := aStructure parents copy.
(myParents includes: aStructureToDelete)
ifFalse:
[self deleteStructure: aStructureToDelete OfSelfAndChildrenOf: self
server root]
ifTrue:
[errorStream
nextPutAll: '--> Paste of ''';
nextPutAll: aStructureToDelete title;
nextPutAll: ''' was ok, but can not cut: destination is
sub-structure of the element to be pasted!'.
self error: errorStream contents]
==>
AdminAction>>cutStructure: aStructureToDelete ofSelfAndChildrenOf:
aStructure
...
self error: (String streamContents: [ :stream |
stream nextPutAll: ' ... ])
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
CharacterArray>>asArrayOfSubstrings
"split the string at its seperators and return array with substrings"
| first last collection |
collection := OrderedCollection new.
last := 0.
[first := self findFirst: [:char | char isSeparator not]
startingAt: last + 1.
first ~= 0]
whileTrue:
[last := (self
findFirst: [:char | char isSeparator]
startingAt: first) - 1.
last < 0 ifTrue: [last := self size].
collection add: (self copyFrom: first to: last)].
^collection asArray
==>
CharacterArray>>asArrayOfSubstrings
^self runsSatisfying: [ :each | each isSeparator ]
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Cheers,
Lukas
--
Lukas Renggli
http://renggli.freezope.org
10 Nov
10 Nov
8:46 p.m.
thanks for your time and review :)
- By the way we got a new client for using SmallWiki: a group from the
natural science institute
The ease to change the banner is really good.
- By the way Michele told me that we have to pay
attention with the icons of the validator because the site of w3c is
submerged by request for these logos so this is better to avoid having
them.
- Lukas I looked inside the edit pane and I could not find the help.
May be I'm blind. I have the impression that having help at the top
level even without editing could be good.
- I saw that people are confused by folders today again.
- why when we have a folder we have *myFolder* and then it is written
*/myFolder*
because when you add a folder to a page containing folders you get:
*/ESE*
*/SDWiki*
and if you enter */MyFolder* the system does not parse it so the
end-user that was making a guess on the way he should write folder is
confused.
Stef
On Lundi, nov 10, 2003, at 20:25 Europe/Zurich, Lukas Renggli wrote:
Hi David,
as there was no SmallWiki meeting today I am giving the code-review I
prepared using e-mail else it gets out-dated. If you have any
questions, please ask.
- Get rid of #isKindOf:
- Don't emit html-tags manually using "html text:
'<b>Role:</b>'" as
this does not allow to render to another output format. Furthermore
the b-tag is depreciated and won't validate for HTML4 or XHTML. Either
use "html strong: 'Role:'" or much better "html spanClass:
'label'
with: 'Role:'" instead.
- Don't emit html-characters manually using "html text:
'&nsbsp;'" as
this does not allow to render to another output format. Use "html
space" instead.
- It is much simpler to use "html cssClass: 'label'" instead of
"html
attributeAt: 'class' put: 'label'".
- Please don't use JavaScript, as it does not work on my machine
even-tough I have enabled it. It is easily possible to replace it with
server-side logic.
- Don't return associations of strings in AdminAction
class>>defaultMenu with tile and the url-postfix, but the actions
classes. All actions in SmallWiki are able to return their title and
know much better how to build their action-url than your code does.
- The class name of AdminAttachments doesn't seem to match what the
action is actually doing.
- AdminAttachments class>>generate_id: should be renamed to
AdminAttachments class>>generateId: as the underscore is not portable
to other Smalltalk dialects.
- Furthermore AdminAttachments class>>generate_id: should be moved to
Structure as it has nothing to do with the action-class and basically
works on a structure.
- Last but not least, you shouldn't need to use AdminAttachments
class>>generate_id: at all. As far as I understand you are building
and parsing this id to identify structures within a wiki. If you use
hidden-fields it is possible to pass objects from one action to the
other and you do not have to deal with strings, this is much simpler
and cleaner.
- I haven't implemented the read-accessor of User>>password
intentionally, because it is planned to store the password as a
hash-value instead of plain-text later on. This is a security issue to
have the password accessible like that and to render it when editing
users. It is the responsibility of the user-class to validate the
user-password using the message #validatePassword: and it should not
be accessible form outside.
- The following sections are some things that are basically reported
by SmallLint or things that I think could have been written simpler.
I've not tested the code I am suggesting, but it should work more or
less:
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Structure>>rolesIncludes: aRole
"check if a role is contained in a roles collection"
roles notNil ifTrue: [^roles includes: aRole].
^false
==>
Structure>>rolesIncludes: aRole
^roles notNil and: [ roles includes: aRole ]
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Structure>>roleWithName: aRoleName
"returns a role with certain name"
self rolesDo: [:role | role name = aRoleName ifTrue: [^role]].
^nil
==>
Structure>>roleWithName: aString
self roles
detect: [ :each | each name = aString ]
ifNone: [ nil ]
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Structure>>selfAndAllChildren
"return all children and their children.... as collection
- at this point: just return self in a collection "
| collection |
(collection := OrderedCollection new) add: self.
^collection
==>
Structure>>withChildren
^OrderedCollection with: self
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
X>>foo
X bar
==>
X>>foo
self class bar
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
AdminAction class>>defaultSeparator
"return the default string separator
- this seperator is used to generate the input fields of the
form"
^String new writeStream tab contents
==>
AdminAction class>>defaultSeparator
^String with: Character tab
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
AdminAction>>cutStructure: aStructureToDelete ofSelfAndChildrenOf:
aStructure
"cut the indicated structure out of the tree, if the paste
destination was not in its subtree"
| errorStream myParents |
errorStream := String new writeStream.
myParents := aStructure parents copy.
(myParents includes: aStructureToDelete)
ifFalse:
[self deleteStructure: aStructureToDelete OfSelfAndChildrenOf: self
server root]
ifTrue:
[errorStream
nextPutAll: '--> Paste of ''';
nextPutAll: aStructureToDelete title;
nextPutAll: ''' was ok, but can not cut: destination is
sub-structure of the element to be pasted!'.
self error: errorStream contents]
==>
AdminAction>>cutStructure: aStructureToDelete ofSelfAndChildrenOf:
aStructure
...
self error: (String streamContents: [ :stream |
stream nextPutAll: ' ... ])
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
CharacterArray>>asArrayOfSubstrings
"split the string at its seperators and return array with substrings"
| first last collection |
collection := OrderedCollection new.
last := 0.
[first := self findFirst: [:char | char isSeparator not]
startingAt: last + 1.
first ~= 0]
whileTrue:
[last := (self
findFirst: [:char | char isSeparator]
startingAt: first) - 1.
last < 0 ifTrue: [last := self size].
collection add: (self copyFrom: first to: last)].
^collection asArray
==>
CharacterArray>>asArrayOfSubstrings
^self runsSatisfying: [ :each | each isSeparator ]
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Cheers,
Lukas
--
Lukas Renggli
http://renggli.freezope.org
10:12 p.m.
- By the way we got a new client for using SmallWiki:
a group from
the natural science institute The ease to change the banner is really
good.
That is cool, I hope that all the design is preserved as the internal
description of the layout has been changed the last few weeks and I
updated the server (see below).
- By the way Michele told me that we have to pay
attention with the
icons of the validator because the site of w3c is submerged by request
for these logos so this is better to avoid having them.
I didn't know and I don't exactly understand what the problem is. They
are saying "To show your readers that you have taken the care to create
an interoperable Web page, you may display this icon on any page that
validates." when validating a page
(http://validator.w3.org/
check?uri=http%3A%2F%2Fkilana.unibe.ch%3A9090). Of course one can
remove it easily by going to the template-editor and ...
- Lukas I looked inside the edit pane and I could not
find the help.
May be I'm blind. I have the impression that having help at the top
level even without editing could be good.
I checked and there is no help displayed, because someone deleted it
using the admin-password. I've restored it now, so it is displayed
again. I've had to update the kilana-server to use the latest version
of SmallWiki, but I failed to update Swazoo (that's why the server was
offline for about half an hour).
Everything should be running again and there all the bug-fixes and
enhancements (like the nested lists) done during the last weeks are
useable. There seems to be a problem paring code-blocks, but that seems
to be a loading problem of StORE again ...
- I saw that people are confused by folders today
again.
I know, but we discussed about adding additional complexity before I
changed the implementation of the folder-class.
- why when we have a folder we have *myFolder* and
then it is written
*/myFolder*
because when you add a folder to a page containing folders you get:
This is because the look-up of links is slightly different in Folders
compared to pages. The look-up in a page is started within the parent
to access the brothers/sisters of the current page, whereas the look-up
of a folder starts in the folder itself, to make accessing the children
more easily. From anywhere it should be possible to access the any item
using an absolute path like */a/b/c*.
*/ESE*
*/SDWiki*
and if you enter */MyFolder* the system does not parse it so the
end-user that was making a guess on the way he should write folder is
confused.
In some cases it could be that the rendering is different to the string
you actually gave at the beginning, but I think as long as it stays
parse-able it should be ok. Try to reproduce with the new version, else
it is really a bug.
Cheers,
Lukas
--
Lukas Renggli
http://renggli.freezope.org
10:30 p.m.
- By the way
we got a new client for using SmallWiki: a group from
the natural science institute The ease to change the banner is really
good.
That is cool, I hope that all the design is preserved as the internal
description of the layout has been changed the last few weeks and I
updated the server (see below). - By the way Michele told me that we have to pay
attention with the
icons of the validator because the site of w3c is submerged by
request for these logos so this is better to avoid having them.
I didn't know and I don't exactly understand what the problem is. They
are saying "To show your readers that you have taken the care to
create an interoperable Web page, you may display this icon on any
page that validates." when validating a page
(http://validator.w3.org/
check?uri=http%3A%2F%2Fkilana.unibe.ch%3A9090). Of course one can
remove it easily by going to the template-editor and ... - Lukas I
looked inside the edit pane and I could not find the help.
May be I'm blind. I have the impression that having help at the top
level even without editing could be good.
I checked and there is no help displayed, because someone deleted it
using the admin-password. I've restored it now, so it is displayed again.
I've had to update the
kilana-server to use the latest version of SmallWiki, but I failed to
update Swazoo (that's why the server was offline for about half an
hour).
My point is that we should understand what confused them and see how we
can provide an adapted answer. We should keep in mind that Wiki users
are far from advanced users. I also think about the reaction that my
wife could have in front of a feature. I thought that the text they get
when editing a folder is also confusing.
- I saw that people are confused by folders today
again.
I know, but we discussed about adding additional complexity before I
changed the implementation of the folder-class. - why when we
have a folder we have *myFolder* and then it is written
*/myFolder*
because when you add a folder to a page containing folders you get:
This is because the look-up of links is slightly different in Folders
compared to pages. The look-up in a page is started within the parent
to access the brothers/sisters of the current page, whereas the
look-up of a folder starts in the folder itself, to make accessing the
children more easily. From anywhere it should be possible to access
the any item using an absolute path like */a/b/c*.
*/ESE*
*/SDWiki*
and if you enter */MyFolder* the system does not parse it so the
end-user that was making a guess on the way he should write folder is
confused.
In some cases it could be that the rendering is different to the
string you actually gave at the beginning, but I think as long as it
stays parse-able it should be ok. Try to reproduce with the new
version, else it is really a bug.
Cheers,
Lukas
--
Lukas Renggli
http://renggli.freezope.org
11:50 p.m.
- By the way we got a new client for using SmallWiki:
a group from
the natural science institute The ease to change the banner is
really good.
That is cool, I hope that all the design is preserved as the internal
description of the layout has been changed the last few weeks and I
updated the server (see below). - By the way Michele told me that we have to pay
attention with the
icons of the validator because the site of w3c is submerged by
request for these logos so this is better to avoid having them.
I didn't know and I don't exactly understand what the problem is.
They are saying "To show your readers that you have taken the care to
create an interoperable Web page, you may display this icon on any
page that validates." when validating a page
(http://validator.w3.org/
check?uri=http%3A%2F%2Fkilana.unibe.ch%3A9090). Of course one can
remove it easily by going to the template-editor and ... - Lukas I looked inside the edit pane and I could not
find the help.
May be I'm blind. I have the impression that having help at the top
level even without editing could be good.
I checked and there is no help displayed, because someone deleted it
using the admin-password. > Cheers,
> Lukas
Dito and good night ...
--
Lukas Renggli
http://renggli.freezope.org
10:33 p.m.
New subject: Another one
- why when we
have a folder we have *myFolder* and then it is written
*/myFolder*
because when you add a folder to a page containing folders you get:
This is because the look-up of links is slightly different in Folders
compared to pages. The look-up in a page is started within the parent
to access the brothers/sisters of the current page, whereas the
look-up of a folder starts in the folder itself, to make accessing the
children more easily. From anywhere it should be possible to access
the any item using an absolute path like */a/b/c*.
*/ESE*
*/SDWiki*
and if you enter */MyFolder* the system does not parse it so the
end-user that was making a guess on the way he should write folder is
confused.
In some cases it could be that the rendering is different to the
string you actually gave at the beginning, but I think as long as it
stays parse-able it should be ok. Try to reproduce with the new
version, else it is really a bug.
Cheers,
Lukas
--
Lukas Renggli
http://renggli.freezope.org
11:36 p.m.
New subject: Another one
Apparently it is:
On the wiki root page I tried to add the */FolderTest* but I got
unable to get non-local structure :)
It is only possible to create new structures on-line (from within the
page) when using simple links without any slashes (e.g. *FolderTest*).
--
Lukas Renggli
http://renggli.freezope.org
11 Nov
11 Nov
9:12 a.m.
New subject: Another one
On Lundi, nov 10, 2003, at 23:36 Europe/Zurich, Lukas Renggli wrote:
Yes this was my point then.
The user is then confused because all the folders already created look
like */xxx* while the one
he wants to create should be entered as *XXX*
may be we should extend the syntax to directly create folder using
*/xxx* this way this will be consistent.
Apparently it
is:
On the wiki root page I tried to add the */FolderTest* but I got
unable to get non-local structure :)
It is only possible to create new structures on-line (from within the
page) when using simple links without any slashes (e.g. *FolderTest*).
--
Lukas Renggli
http://renggli.freezope.org
7478
days inactive
7479
days old
7 comments
2 participants
participants (2)
-
ducasse -
Lukas Renggli