Metadata editor.

[spy, 2012-07-18 15:48:27]

http://codereview.appspot.com/6429043/diff/1/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6429043/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:676: extra_meta_text =
OptionalMultilineText.using(label=L_("Extra MetaData
(JSON)")).with_properties(rows=ROWS_META, cols=COLS).validated_by(ValidJSON())
Do we still need an extra JSON editor in case all meta keys are implemented as
widgets?

[ThomasJWaldmann, 2012-07-18 16:09:25]

some minor comments

http://codereview.appspot.com/6429043/diff/1/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6429043/diff/1/MoinMoin/forms.py#newcode77
MoinMoin/forms.py:77: class Reference(Select):
docstring missing

http://codereview.appspot.com/6429043/diff/1/MoinMoin/forms.py#newcode98
MoinMoin/forms.py:98: self.valid_values = zip(*info)[0]
that last line made me thinking.... :)

but don't you think it is less puzzling (and likely even cheaper to compute) to
just use dict(info).keys()? you could even reuse the dict so you don't need to
build it twice.

http://codereview.appspot.com/6429043/diff/1/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6429043/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:684: del meta[k]
meta.pop(k, None)  replaces both lines.

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

http://codereview.appspot.com/6429043/diff/1/MoinMoin/templates/modify_applet...
MoinMoin/templates/modify_applet.html:32: #}
well, i think we likely still need the json editor for some stuff. but that can
get readded in another changeset.

[Reimar Bauer, 2012-07-18 16:27:38]

some comments

http://codereview.appspot.com/6429043/diff/1/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6429043/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:676: extra_meta_text =
OptionalMultilineText.using(label=L_("Extra MetaData
(JSON)")).with_properties(rows=ROWS_META, cols=COLS).validated_by(ValidJSON())
On 2012/07/18 15:48:27, spy wrote:
> Do we still need an extra JSON editor in case all meta keys are implemented as
> widgets?

it depends a little bit, yes for now.

one thing you easily can do with the json editor is to
copy/paste and to delete.

Also you can introduce new types or more complexity.
Not sure if last is good but at the current state we can't decide that.

[xiaq, 2012-07-19 16:37:26]

http://codereview.appspot.com/6429043/diff/1/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6429043/diff/1/MoinMoin/forms.py#newcode77
MoinMoin/forms.py:77: class Reference(Select):
On 2012/07/18 16:09:25, ThomasJWaldmann wrote:
> docstring missing

fixing

http://codereview.appspot.com/6429043/diff/1/MoinMoin/forms.py#newcode98
MoinMoin/forms.py:98: self.valid_values = zip(*info)[0]
On 2012/07/18 16:09:25, ThomasJWaldmann wrote:
> that last line made me thinking.... :)
> 
> but don't you think it is less puzzling (and likely even cheaper to compute)
to
> just use dict(info).keys()? you could even reuse the dict so you don't need to
> build it twice.

ah that's good :) but i'd use OrderedDict to preserve the ordering of `info`.
Fixing in next patch set.

http://codereview.appspot.com/6429043/diff/1/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6429043/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:676: extra_meta_text =
OptionalMultilineText.using(label=L_("Extra MetaData
(JSON)")).with_properties(rows=ROWS_META, cols=COLS).validated_by(ValidJSON())
On 2012/07/18 15:48:27, spy wrote:
> Do we still need an extra JSON editor in case all meta keys are implemented as
> widgets?

see comment in modify_applet.html.

http://codereview.appspot.com/6429043/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:684: del meta[k]
On 2012/07/18 16:09:25, ThomasJWaldmann wrote:
> meta.pop(k, None)  replaces both lines.

ah, thanks :)

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

http://codereview.appspot.com/6429043/diff/1/MoinMoin/templates/modify_applet...
MoinMoin/templates/modify_applet.html:32: #}
On 2012/07/18 16:09:25, ThomasJWaldmann wrote:
> well, i think we likely still need the json editor for some stuff. but that
can
> get readded in another changeset.

moin1 had both WYSIWYG and text editors for content editing. we can do that for
metadata editing too.

[ThomasJWaldmann, 2012-07-22 12:42:34]

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

http://codereview.appspot.com/6429043/diff/1/MoinMoin/templates/modify_applet...
MoinMoin/templates/modify_applet.html:32: #}
hmm, but if implemented as ALTERNATIVES we either need a way to switch
dynamically between them WHILE editing or one would sometimes need 2 edits (e.g.
if the pretty metadata editor misses some field one needs to set, which is not
quite unlikely at the beginning).

http://codereview.appspot.com/6429043/diff/8001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6429043/diff/8001/MoinMoin/forms.py#newcode80
MoinMoin/forms.py:80: '''
please always use triple double-quotes.
of course triple single-quotes is also valid, but there is at least one vim
plugin out there that does not like '''.

http://codereview.appspot.com/6429043/diff/8001/MoinMoin/forms.py#newcode82
MoinMoin/forms.py:82: Results of a search query.
it it really another Item (as in item == set of revisions) or a revision or can
it be both and is basically just any UUID?

http://codereview.appspot.com/6429043/diff/8001/MoinMoin/forms.py#newcode101
MoinMoin/forms.py:101: # cheap so this is not needed?
well, maybe do not do premature optimizations, but rather if you really get
performance issues. doing some cache for the lifetime of a request seems
harmless, though, but in moin 1.9 we have memory issues due to longer term
caching.

btw, there is also flask-cache used in moin, see app.py.