Rebased metadata editor

Rebased metadata editor

[Reimar Bauer, 2012-07-30 11:24:26]

some comments and questions

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

http://codereview.appspot.com/6447057/diff/1/MoinMoin/forms.py#newcode13
MoinMoin/forms.py:13: import json
try:
    import json
except ImportError:
    import simplejson as json

http://codereview.appspot.com/6447057/diff/1/MoinMoin/forms.py#newcode47
MoinMoin/forms.py:47: JSON = OptionalMultilineText.with_properties(lang='en',
dir='ltr').validated_by(ValidJSON())
we have some other languages implemented. Is it needed to use the one from
usersettings and fall back to en?

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

http://codereview.appspot.com/6447057/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:143: class BaseMetaForm(Form):
is this covered by a test?

[Reimar Bauer, 2012-07-30 12:03:52]

redone, please look at the comments of the other version

http://codereview.appspot.com/6447057/diff/3002/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6447057/diff/3002/MoinMoin/forms.py#newcode13
MoinMoin/forms.py:13: import json
see my other comment about json import, do it the same like on other json
imports

[spy, 2012-07-30 12:09:45]

some answers

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

http://codereview.appspot.com/6447057/diff/1/MoinMoin/forms.py#newcode13
MoinMoin/forms.py:13: import json
On 2012/07/30 11:24:27, Reimar Bauer wrote:
> try:
>     import json
> except ImportError:
>     import simplejson as json

it is not required anymore
http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode16

http://codereview.appspot.com/6447057/diff/1/MoinMoin/forms.py#newcode47
MoinMoin/forms.py:47: JSON = OptionalMultilineText.with_properties(lang='en',
dir='ltr').validated_by(ValidJSON())
On 2012/07/30 11:24:27, Reimar Bauer wrote:
> we have some other languages implemented. Is it needed to use the one from
> usersettings and fall back to en?

This was copied from xiaq's changeset, he has already said something on this
occasion. Will ask him on his codereview.

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

http://codereview.appspot.com/6447057/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:143: class BaseMetaForm(Form):
On 2012/07/30 11:24:27, Reimar Bauer wrote:
> is this covered by a test?

not yet, first we should decide with xiaq which implementation of metaeditor we
will use.

[xiaq, 2012-07-31 03:04:08]

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

http://codereview.appspot.com/6447057/diff/1/MoinMoin/forms.py#newcode47
MoinMoin/forms.py:47: JSON = OptionalMultilineText.with_properties(lang='en',
dir='ltr').validated_by(ValidJSON())
On 2012/07/30 11:24:27, Reimar Bauer wrote:
> we have some other languages implemented. Is it needed to use the one from
> usersettings and fall back to en?

well, json is not a natural language. :-P

lang='en', dir='ltr' should be sane for editing json.

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

http://codereview.appspot.com/6447057/diff/10001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:153: extra_meta_text = (JSON.using(label=L_("Extra
MetaData (JSON)"))
things go wrong when metadata happens to have a key named "extra_meta_text". :)

it's unlikely, but for cleanliness's sake i put extra_meta_text in _ModifyForm
instead of BaseMetaForm in my patch set.

[spy, 2012-07-31 09:06:37]

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

http://codereview.appspot.com/6447057/diff/10001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:153: extra_meta_text = (JSON.using(label=L_("Extra
MetaData (JSON)"))
On 2012/07/31 03:04:08, xiaq wrote:
> things go wrong when metadata happens to have a key named "extra_meta_text".
:)
> 
> it's unlikely, but for cleanliness's sake i put extra_meta_text in _ModifyForm
> instead of BaseMetaForm in my patch set.

Why do they go wrong? Play with it on http://vps.psviderski.name:8080  You can
create "extra_meta_text" key in the JSON editor and everything will work fine.
It just will not be displayed in JSON field. So, for cleanliness's sake
"extra_meta_text" key could be filtered in meta_cleanup method.

See next changeset http://codereview.appspot.com/6447057/#ps3004

[xiaq, 2012-07-31 12:26:48]

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

http://codereview.appspot.com/6447057/diff/10001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:153: extra_meta_text = (JSON.using(label=L_("Extra
MetaData (JSON)"))
On 2012/07/31 09:06:37, spy wrote:
> On 2012/07/31 03:04:08, xiaq wrote:
> > things go wrong when metadata happens to have a key named "extra_meta_text".
> :)
> > 
> > it's unlikely, but for cleanliness's sake i put extra_meta_text in
_ModifyForm
> > instead of BaseMetaForm in my patch set.
> 
> Why do they go wrong? Play with it on http://vps.psviderski.name:8080  You can
> create "extra_meta_text" key in the JSON editor and everything will work fine.
> It just will not be displayed in JSON field. So, for cleanliness's sake
> "extra_meta_text" key could be filtered in meta_cleanup method.
> 

ah, they don't. :)

still, my idea is to keep in BaseMetaForm only meta keys, nothing more. perhaps
a more practical example is that you might want need metadata property keyed
"template" one day. just a suggestion.

> See next changeset http://codereview.appspot.com/6447057/#ps3004

[spy, 2012-07-31 17:33:06]

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

http://codereview.appspot.com/6447057/diff/10001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:153: extra_meta_text = (JSON.using(label=L_("Extra
MetaData (JSON)"))
> a more practical example is that you might want need metadata property keyed
> "template" one day. just a suggestion.

Agree with that. After all the extra_meta_text is a temporary field. It won't be
in the final release of moin2. So, why is it so much attention and debate about
it?

Another thing is the "template" constant. Since we have splitted the ModifyForm
in two separate Meta and Data forms, and introduced the macros-based data
editor, there should be the way to define the template for a particular meta
form where could be defined the macros-based meta editor.
Maybe use uppercase for that?

Later I want to define BlogMetaForm and BlogEntryMetaForm in high-level
Blog(Contentful) and BlogEntry(Contentful) classes (these classes use itemtype).
Also I can overwrite the Contentful's ModifyForm property. I am able to assign
meta_form=BlogMetaForm/BlogEntryMetaForm there. Am I right understand your
itemtype abstraction idea?

[ThomasJWaldmann, 2012-07-31 19:43:01]

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

http://codereview.appspot.com/6447057/diff/10001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:153: extra_meta_text = (JSON.using(label=L_("Extra
MetaData (JSON)"))
better don't assume what will be in a final release.
we are not that far yet and currently we will always keep the json. for user
defined keys, for keys we do not support yet.

[xiaq, 2012-08-01 01:38:54]

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

http://codereview.appspot.com/6447057/diff/10001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:153: extra_meta_text = (JSON.using(label=L_("Extra
MetaData (JSON)"))
On 2012/07/31 17:33:06, spy wrote:
> > a more practical example is that you might want need metadata property keyed
> > "template" one day. just a suggestion.
> 
> Agree with that. After all the extra_meta_text is a temporary field. It won't
be
> in the final release of moin2. So, why is it so much attention and debate
about
> it?
> 
> Another thing is the "template" constant. Since we have splitted the
ModifyForm
> in two separate Meta and Data forms, and introduced the macros-based data
> editor, there should be the way to define the template for a particular meta
> form where could be defined the macros-based meta editor.
> Maybe use uppercase for that?
> 

my idea is that the Content's Form is somehow isolated from the Item's, but the
metadata's isn't. (That is, if you need to change the metadata editor, you
probably want to change the overall modify view as well.) The BaseMetaForm is
there to create a namespace so that metadata keys don't get mixed with other
Form elements. Seems better named BaseMetaSchema to avoid confusion.

So I'd keep the extra_meta_text widget in ModifyForm and add a meta_template
class property to it.

> Later I want to define BlogMetaForm and BlogEntryMetaForm in high-level
> Blog(Contentful) and BlogEntry(Contentful) classes (these classes use
itemtype).
> Also I can overwrite the Contentful's ModifyForm property. I am able to assign
> meta_form=BlogMetaForm/BlogEntryMetaForm there. Am I right understand your
> itemtype abstraction idea?

Consider overriding do_modify as well. (Replace the "ok" button with "save
draft" and "publish", for instance.)

Still, the itemtype idea is pretty simple and current implementation is a bit
rough, so some details can still be polished.

http://codereview.appspot.com/6447057/diff/10001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:153: extra_meta_text = (JSON.using(label=L_("Extra
MetaData (JSON)"))
On 2012/07/31 19:43:01, ThomasJWaldmann wrote:
> better don't assume what will be in a final release.
> we are not that far yet and currently we will always keep the json. for user
> defined keys, for keys we do not support yet.

yes, i would prefer to keep extra_meta_text at least before the plain json
editor is reintroduced and there is a way to switch between that and the
widget-based metadata editor we're currently implementing. otherwise it results
in lost functionality (which might not matter for users, but does for
developers).