Convert usage of Flatland forms

Convert usage of Flatland forms: http://etherpad.osuosl.org/moin-refactor-forms

[ThomasJWaldmann, 2012-06-16 11:54:30]

http://codereview.appspot.com/6303091/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6303091/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:469: comment =
OptionalText.using(label=L_('Comment')).with_properties(placeholder=L_("Comment
about your change"))
if it is just for the optional, I don't see much sense just creating a special
class for that.

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/forms.html
File MoinMoin/templates/forms.html (right):

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/forms.html#ne...
MoinMoin/templates/forms.html:146: 
if this stuff is replacing the "old" stuff, the old stuff should be removed (and
globally replaced by new stuff).

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/openid_regist...
File MoinMoin/templates/openid_register.html (right):

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/openid_regist...
MoinMoin/templates/openid_register.html:10: {{ forms.render(form['submit']) }}
hmm, wasn't the submit button moved out of the dl at most other places due to
html validation failing? please check.

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/rename.html
File MoinMoin/templates/rename.html (right):

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/rename.html#n...
MoinMoin/templates/rename.html:13: {{ forms.render(form['comment'])}}
hmm, so what's the difference between above 2 lines?

[xiaq, 2012-06-16 12:11:40]

http://codereview.appspot.com/6303091/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6303091/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:53: from MoinMoin.apps.frontend.forms import
Text, OptionalText, Submit
I left out this module. Fixing in next patch set.

http://codereview.appspot.com/6303091/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:469: comment =
OptionalText.using(label=L_('Comment')).with_properties(placeholder=L_("Comment
about your change"))
On 2012/06/16 11:54:30, ThomasJWaldmann wrote:
> if it is just for the optional, I don't see much sense just creating a special
> class for that.

(I forgot to do `hg add` to my newly created apps.frontend.views module.) Not
only optional, but also a property indicating the widget to be used. Wait for my
next patch set...

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/forms.html
File MoinMoin/templates/forms.html (right):

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/forms.html#ne...
MoinMoin/templates/forms.html:146: 
On 2012/06/16 11:54:30, ThomasJWaldmann wrote:
> if this stuff is replacing the "old" stuff, the old stuff should be removed
(and
> globally replaced by new stuff).

Yes, but doing that takes some time, so I'm illustrating how I'd like to have
things done and requesting for comments first. It it looks good to you guys,
I'll do that.

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/openid_regist...
File MoinMoin/templates/openid_register.html (right):

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/openid_regist...
MoinMoin/templates/openid_register.html:10: {{ forms.render(form['submit']) }}
On 2012/06/16 11:54:30, ThomasJWaldmann wrote:
> hmm, wasn't the submit button moved out of the dl at most other places due to
> html validation failing? please check.

Oh, this was a mistake. Fixing it in next patch set.

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/rename.html
File MoinMoin/templates/rename.html (right):

http://codereview.appspot.com/6303091/diff/1/MoinMoin/templates/rename.html#n...
MoinMoin/templates/rename.html:13: {{ forms.render(form['comment'])}}
On 2012/06/16 11:54:30, ThomasJWaldmann wrote:
> hmm, so what's the difference between above 2 lines?

Ultimately the above will be changed to

{{ forms.render(form['target']) }}

See my comment on forms.html.

[ThomasJWaldmann, 2012-06-16 12:32:58]

http://codereview.appspot.com/6303091/diff/5001/MoinMoin/templates/forms.html
File MoinMoin/templates/forms.html (right):

http://codereview.appspot.com/6303091/diff/5001/MoinMoin/templates/forms.html...
MoinMoin/templates/forms.html:127: 'text': annotated_input, 'submit': raw_input,
1 k/v per line please

http://codereview.appspot.com/6303091/diff/5001/MoinMoin/templates/forms.html...
MoinMoin/templates/forms.html:129: {%- set macro =
widgets_dict[field.properties.widget] or stub -%}
hmm, can this be merged into the above code block?

[Reimar Bauer, 2012-06-17 08:02:35]

some comments

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms.py
File MoinMoin/apps/frontend/forms.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms....
MoinMoin/apps/frontend/forms.py:17: 
"Enter a short comment about your change!"

whenever possible use punctuation

! if required or . if not

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/forms.py#newcode56
MoinMoin/forms.py:56: 
Natural sounds a bit strange to me.

But may be ok, Natural_Number is too long.

If it is only 0-255 it can be Byte

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/search/__init__.py
File MoinMoin/search/__init__.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/search/__init__.py#n...
MoinMoin/search/__init__.py:29: history = InlineCheckbox.using(label=L_('search
also in non-current revisions'))
is optional=True given?

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/destroy.html
File MoinMoin/templates/destroy.html (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/destroy.ht...
MoinMoin/templates/destroy.html:13: {{ forms.render(form['comment'])}}
keep blank }}

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/destroy.ht...
MoinMoin/templates/destroy.html:25: {{ forms.render(form['comment'])}}
please keep blank }}

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/forms.html
File MoinMoin/templates/forms.html (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/forms.html...
MoinMoin/templates/forms.html:84: Code above are will be either adapted or
removed to follow the new style. #}
-are

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/forms.html...
MoinMoin/templates/forms.html:95: 'filter': filter_,
filter_any ?

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/index.html
File MoinMoin/templates/index.html (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/index.html...
MoinMoin/templates/index.html:65: {{ forms.render(form['unknown_items']) }}
could be replaced by looping through a tuple or list

if there are plugins which extend types, we have to make this extensible too

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/lookup.html
File MoinMoin/templates/lookup.html (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/lookup.htm...
MoinMoin/templates/lookup.html:20: {{ forms.render(lookup_form['history']) }}
can be simplified 

and we have to think on how extensions can get into this lookup_form too

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/rename.html
File MoinMoin/templates/rename.html (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/rename.htm...
MoinMoin/templates/rename.html:12: {{ forms.render(form['target'])}}
keep blank before }}

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/search.html
File MoinMoin/templates/search.html (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/search.htm...
MoinMoin/templates/search.html:23: {{
forms.render(medium_search_form['history']) }}
how is it rendered, with markup or without?

[ThomasJWaldmann, 2012-06-17 12:47:45]

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms.py
File MoinMoin/apps/frontend/forms.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms....
MoinMoin/apps/frontend/forms.py:7: General Flatland forms containing hints for
the templates.
is it about widgets or about forms? you seem to disagree with yourself about
this. :)

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms....
MoinMoin/apps/frontend/forms.py:17: 
On 2012/06/17 08:02:35, Reimar Bauer wrote:
> "Enter a short comment about your change!"
> 
> whenever possible use punctuation
> 
> ! if required or . if not

as this is a form label, neither . nor ! are appropriate.

either nothing or : (IMHO).

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/forms.py#newcode30
MoinMoin/forms.py:30: Email =
String.using(label=L_('E-Mail')).with_properties(widget=WIDGET_EMAIL,
placeholder=L_("Your E-Mail address")).validated_by(IsEmail())
i guess we have to think about the defaults we want to set in this module. in
case of Email, you set "Your ..." as default label, but this can easily go wrong
if someone is using Email and not asking for "Your" email address, but e.g. for
a target email address. So I suggest to either not set default labels in this
module at all or at least make them more generic. If they are just made more
generic, there is some risk that they are just used "as is" and that the meaning
is unclear to the user then, so maybe having no default at all is the best
option.

[xiaq, 2012-06-18 02:49:51]

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms.py
File MoinMoin/apps/frontend/forms.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms....
MoinMoin/apps/frontend/forms.py:7: General Flatland forms containing hints for
the templates.
On 2012/06/17 12:47:45, ThomasJWaldmann wrote:
> is it about widgets or about forms? you seem to disagree with yourself about
> this. :)

Changing it in next patch set. Also this docstring belongs to MoinMoin/forms.py,
it's not appropriate here...

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms....
MoinMoin/apps/frontend/forms.py:17: 
On 2012/06/17 12:47:45, ThomasJWaldmann wrote:
> On 2012/06/17 08:02:35, Reimar Bauer wrote:
> > "Enter a short comment about your change!"
> > 
> > whenever possible use punctuation
> > 
> > ! if required or . if not
> 
> as this is a form label, neither . nor ! are appropriate.
> 
> either nothing or : (IMHO).

Hm, i suppose we keep the placeholder? Or change it to

placeholder=L_("Enter a short comment about your change")

?

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/forms.py#newcode30
MoinMoin/forms.py:30: Email =
String.using(label=L_('E-Mail')).with_properties(widget=WIDGET_EMAIL,
placeholder=L_("Your E-Mail address")).validated_by(IsEmail())
On 2012/06/17 12:47:45, ThomasJWaldmann wrote:
> i guess we have to think about the defaults we want to set in this module. in
> case of Email, you set "Your ..." as default label, but this can easily go
wrong
> if someone is using Email and not asking for "Your" email address, but e.g.
for
> a target email address. So I suggest to either not set default labels in this
> module at all or at least make them more generic. If they are just made more
> generic, there is some risk that they are just used "as is" and that the
meaning
> is unclear to the user then, so maybe having no default at all is the best
> option.

Okay, going to fix it.

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/forms.py#newcode56
MoinMoin/forms.py:56: 
On 2012/06/17 08:02:35, Reimar Bauer wrote:
> Natural sounds a bit strange to me.
> 
> But may be ok, Natural_Number is too long.
> 
> If it is only 0-255 it can be Byte

Byte is not that useful in the View sense, while the motivation for SmallInteger
is that it can be rendered with some widgets like a series of radio buttons, in
contrary to AnyInteger, which is rendered with plain text boxes (or something
similar that allows arbitrary integer to be specified).

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/search/__init__.py
File MoinMoin/search/__init__.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/search/__init__.py#n...
MoinMoin/search/__init__.py:29: history = InlineCheckbox.using(label=L_('search
also in non-current revisions'))
On 2012/06/17 08:02:35, Reimar Bauer wrote:
> is optional=True given?

Yes, see definition of InlineCheckbox in forms.py:38

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/lookup.html
File MoinMoin/templates/lookup.html (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/lookup.htm...
MoinMoin/templates/lookup.html:20: {{ forms.render(lookup_form['history']) }}
On 2012/06/17 08:02:35, Reimar Bauer wrote:
> can be simplified 
> 
> and we have to think on how extensions can get into this lookup_form too

I put my ideas to the last section of
http://etherpad.osuosl.org/moin-refactor-forms.

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/rename.html
File MoinMoin/templates/rename.html (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/rename.htm...
MoinMoin/templates/rename.html:12: {{ forms.render(form['target'])}}
On 2012/06/17 08:02:35, Reimar Bauer wrote:
> keep blank before }}

ok, fixing all whitespace issues in templates in next patch set.

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/search.html
File MoinMoin/templates/search.html (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/templates/search.htm...
MoinMoin/templates/search.html:23: {{
forms.render(medium_search_form['history']) }}
On 2012/06/17 08:02:35, Reimar Bauer wrote:
> how is it rendered, with markup or without?

Without. This is determined by type of history element (InlineCheckbox), see
apps/frontend/views.py:143.

[ThomasJWaldmann, 2012-06-18 08:07:02]

please also try all the stuff you changed.
run py.test.

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms.py
File MoinMoin/apps/frontend/forms.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms....
MoinMoin/apps/frontend/forms.py:17: 
As this is a default text, just keeping 'Comment' is fine IMHO. If you want to
be more verbose / more specific, you can override it.

http://codereview.appspot.com/6303091/diff/34/MoinMoin/templates/forms.html
File MoinMoin/templates/forms.html (right):

http://codereview.appspot.com/6303091/diff/34/MoinMoin/templates/forms.html#n...
MoinMoin/templates/forms.html:84: Code above are will be either adapted or
removed to follow the new style. #}
wording not fixed yet.

[xiaq, 2012-06-18 13:52:24]

On 2012/06/18 08:07:02, ThomasJWaldmann wrote:
> please also try all the stuff you changed.
> run py.test.
> 

I'll be testing once I finish...

>
http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms.py
> File MoinMoin/apps/frontend/forms.py (right):
> 
>
http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms....
> MoinMoin/apps/frontend/forms.py:17: 
> As this is a default text, just keeping 'Comment' is fine IMHO. If you want to
> be more verbose / more specific, you can override it.
> 
> http://codereview.appspot.com/6303091/diff/34/MoinMoin/templates/forms.html
> File MoinMoin/templates/forms.html (right):
> 
>
http://codereview.appspot.com/6303091/diff/34/MoinMoin/templates/forms.html#n...
> MoinMoin/templates/forms.html:84: Code above are will be either adapted or
> removed to follow the new style. #}
> wording not fixed yet.

[xiaq, 2012-06-18 13:52:42]

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms.py
File MoinMoin/apps/frontend/forms.py (right):

http://codereview.appspot.com/6303091/diff/4007/MoinMoin/apps/frontend/forms....
MoinMoin/apps/frontend/forms.py:17: 
On 2012/06/18 08:07:02, ThomasJWaldmann wrote:
> As this is a default text, just keeping 'Comment' is fine IMHO. If you want to
> be more verbose / more specific, you can override it.

Instead of changing the placeholder, I'll rename CommentForm to a more specific
BaseChangeForm to reflect that all its subclasses are Forms that change an item
in some way.

http://codereview.appspot.com/6303091/diff/34/MoinMoin/templates/forms.html
File MoinMoin/templates/forms.html (right):

http://codereview.appspot.com/6303091/diff/34/MoinMoin/templates/forms.html#n...
MoinMoin/templates/forms.html:84: Code above are will be either adapted or
removed to follow the new style. #}
On 2012/06/18 08:07:02, ThomasJWaldmann wrote:
> wording not fixed yet.

Hm, is the comment not good enough? Anyway I'll be converting forms.py
completely and sending in the result in next patch set.

[xiaq, 2012-06-19 11:40:54]

Patch set 6 just uploaded. Please review! :)

This is what I've made so far - Apart from some minor cleanups, I introduced
MultiColumnForm and TabbedForm widgets (Flatland form classes and corresponding
Jinja2 macros), which will be also useful for the metadata editor. The Jinja2
macros primarily come from a refactor of usersettings.html and
usersettings_form.html, which are now almost factored out totally by the more
general macros.

The code still needs some polishing - it's not complete, may contain silly bugs,
and I haven't run an automated test yet, since I'm still actively changing the
implementation. I will work out a more tidy and tested version and uploading in
a day or two.

Thanks!

[ThomasJWaldmann, 2012-06-19 13:22:14]

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1389: email =
email.with_properties(helper=L_("Changing your email address requires you to
verify it. A link will be sent to you."))
if / else

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1452: if form['name_'].value != flaskg.user.name
and user.search_users(name_exact=form['name_'].value):
maybe you could check whether Form.name is used at all.
it is defined everywhere, but I do not remember where (or whether) it is USED.

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py#newcode54
MoinMoin/forms.py:54: # XXX needs some thinking about this widget
such comments are pretty useless if you do not write WHAT PRECISELY to think
about

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py#newcode76
MoinMoin/forms.py:76: if type(members.get('layout', None)) == str:
isinstance(..., str)

really str? unicode? basestring?

None is the default default for .get().

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py#newcode143
MoinMoin/forms.py:143: layout = []
it looks a bit like you are beginning to mix up template stuff (layout) and
python stuff (data model).

not sure if that is a good idea.

[xiaq, 2012-06-19 14:25:50]

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1452: if form['name_'].value != flaskg.user.name
and user.search_users(name_exact=form['name_'].value):
On 2012/06/19 13:22:15, ThomasJWaldmann wrote:
> maybe you could check whether Form.name is used at all.
> it is defined everywhere, but I do not remember where (or whether) it is USED.

Flatland Form definitions call .named which in turn sets the name attribute, so
it should always be avoided.

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py#newcode54
MoinMoin/forms.py:54: # XXX needs some thinking about this widget
On 2012/06/19 13:22:15, ThomasJWaldmann wrote:
> such comments are pretty useless if you do not write WHAT PRECISELY to think
> about

Going to fix it. BTW it was just a note to remind myself :)

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py#newcode76
MoinMoin/forms.py:76: if type(members.get('layout', None)) == str:
On 2012/06/19 13:22:15, ThomasJWaldmann wrote:
> isinstance(..., str)
> 
> really str? unicode? basestring?
> 
> None is the default default for .get().

ah, going to fix it.

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py#newcode143
MoinMoin/forms.py:143: layout = []
On 2012/06/19 13:22:15, ThomasJWaldmann wrote:
> it looks a bit like you are beginning to mix up template stuff (layout) and
> python stuff (data model).
> 

IMHO python stuff is not necessarily data model. Flatland forms are closer to
view than data model. Forms like DeleteItemForm, PasswordLostForm don't really
reflect the data model; the UserSettings*Form's do correspond to the underlying
User object, but they add categorization (separation of the 6 tabs) and hints
information (labels) that's useful in the view but not in the model.

> not sure if that is a good idea.

[ThomasJWaldmann, 2012-06-21 12:00:19]

2 comments, one on cs6 and one on cs7

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/6007/MoinMoin/forms.py#newcode143
MoinMoin/forms.py:143: layout = []
it was esp. that "layout" stuff i was wondering about.

http://codereview.appspot.com/6303091/diff/1021/MoinMoin/templates/forms.html
File MoinMoin/templates/forms.html (right):

http://codereview.appspot.com/6303091/diff/1021/MoinMoin/templates/forms.html...
MoinMoin/templates/forms.html:131: {% macro filter_(field) %}
the name "filter" is somehow strange, it seems to tell about the specific usage
of it rather than what the widget itself is.

[jek, 2012-06-23 16:49:18]

This looks great!

http://codereview.appspot.com/6303091/diff/1021/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/1021/MoinMoin/forms.py#newcode68
MoinMoin/forms.py:68: Hidden = String.with_properties(widget=WIDGET_HIDDEN,
optional=True)
Should this be .using(optional=True) rather than set in properties?

[xiaq, 2012-06-25 05:30:27]

On 2012/06/23 16:49:18, jek wrote:
> This looks great!
> 

There seems to be not much doubt about the ordinary widgets, but as for form
widgets, Thomas expressed the concern that mixing structure and presentation may
not be good. What's your idea about this?

Thanks!

> http://codereview.appspot.com/6303091/diff/1021/MoinMoin/forms.py
> File MoinMoin/forms.py (right):
> 
> http://codereview.appspot.com/6303091/diff/1021/MoinMoin/forms.py#newcode68
> MoinMoin/forms.py:68: Hidden = String.with_properties(widget=WIDGET_HIDDEN,
> optional=True)
> Should this be .using(optional=True) rather than set in properties?

[xiaq, 2012-06-25 05:30:39]

http://codereview.appspot.com/6303091/diff/1021/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/1021/MoinMoin/forms.py#newcode68
MoinMoin/forms.py:68: Hidden = String.with_properties(widget=WIDGET_HIDDEN,
optional=True)
On 2012/06/23 16:49:18, jek wrote:
> Should this be .using(optional=True) rather than set in properties?

Ah yes, will fix it.

[ThomasJWaldmann, 2012-07-01 06:30:06]

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/apps/frontend/forms.py
File MoinMoin/apps/frontend/forms.py (right):

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/apps/frontend/forms...
MoinMoin/apps/frontend/forms.py:15: 
if this stuff is just used at one place, we maybe could put it there instead of
into a separate file?

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:496: filter_ =
ContenttypeFilterForm.field_schema_mapping.get(gname.replace(' ', '_'))
i don't see how this works, there is no .field_schema_mapping there?

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/forms.py#newcode72
MoinMoin/forms.py:72: Hidden = String.with_properties(widget=WIDGET_HIDDEN,
optional=True)
you did not fix it, see comments in previous changeset.

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/forms.py#newcode87
MoinMoin/forms.py:87: class LayoutForm(Form):
is this still used?

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/templates/index.html
File MoinMoin/templates/index.html (right):

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/templates/index.htm...
MoinMoin/templates/index.html:66: 'unknown_items'] %}
is a comma and a blank after last element allowed in jinja2?

that is our usual style in python (and also the ]: on a new line).

[xiaq, 2012-07-01 13:45:39]

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/apps/frontend/forms.py
File MoinMoin/apps/frontend/forms.py (right):

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/apps/frontend/forms...
MoinMoin/apps/frontend/forms.py:15: 
On 2012/07/01 06:30:06, ThomasJWaldmann wrote:
> if this stuff is just used at one place, we maybe could put it there instead
of
> into a separate file?

it used to be in apps/frontend/views.py, but items/__init__.py uses it too.
items module is imported by several fairly low-level modules, and this creates a
lot of circular dependencies.

if it is to be moved into another file, i'd prefer it moved into
items/__init__.py instead of apps/frontenv/views.py.

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:496: filter_ =
ContenttypeFilterForm.field_schema_mapping.get(gname.replace(' ', '_'))
On 2012/07/01 06:30:06, ThomasJWaldmann wrote:
> i don't see how this works, there is no .field_schema_mapping there?

It is a property defined in flatland.Dict.

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/forms.py#newcode87
MoinMoin/forms.py:87: class LayoutForm(Form):
On 2012/07/01 06:30:06, ThomasJWaldmann wrote:
> is this still used?

ah, somehow i didn't mange to remove this from the patch. fixing in next patch
set.

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/templates/index.html
File MoinMoin/templates/index.html (right):

http://codereview.appspot.com/6303091/diff/31001/MoinMoin/templates/index.htm...
MoinMoin/templates/index.html:66: 'unknown_items'] %}
On 2012/07/01 06:30:06, ThomasJWaldmann wrote:
> is a comma and a blank after last element allowed in jinja2?
> 
> that is our usual style in python (and also the ]: on a new line).

ah, fixing it.

[Reimar Bauer, 2012-07-02 08:09:10]

http://codereview.appspot.com/6303091/diff/40005/MoinMoin/templates/lookup.html
File MoinMoin/templates/lookup.html (right):

http://codereview.appspot.com/6303091/diff/40005/MoinMoin/templates/lookup.ht...
MoinMoin/templates/lookup.html:10: {{ forms.render(lookup_form['name']) }}
Why no loop over a list of all forms?

[ThomasJWaldmann, 2012-07-03 16:11:52]

looks ok to me now.

please do a "make test" and if ok, do some clean changeset(s).

[xiaq, 2012-07-04 03:46:44]

On 2012/07/03 16:11:52, ThomasJWaldmann wrote:
> looks ok to me now.
> 
> please do a "make test" and if ok, do some clean changeset(s).

have run "make test". no regression except for a couple of PEP8 issues which i'm
fixing and will send in.

personally i found the current patch set already clean enough, it does just one
thing (convert form to use widgets, not 100% complete though). where is cleanup
still needed? - perhaps move BaseChangeForm from apps/frontend/forms.py into
items/__init__.py?

[ThomasJWaldmann, 2012-07-05 12:22:15]

"do some clean changesets" was not intended to say that the current stuff is
unclean, was just intended to make you think about it.

so commit when you are ready with it.