Issue 6304060: Metadata editor, plus some refactor of apps.frontend.views and items modules (Closed)
|
Created:
12 years, 11 months ago by xiaq Modified:
12 years, 10 months ago Reviewers:
thomas.j.waldmann, Reimar Bauer Visibility:
Public. |
Descriptionclosing as it is now split into two issues.
Patch Set 1 #Patch Set 2 : fix for get_item and download_item #Patch Set 3 : rename meta_widgets.html to meta_form.html; fix for children order (use form.field_schema to get pr… #
Total comments: 1
Patch Set 4 : small mod on meta_form.html #Patch Set 5 : remove duplicate .named in Flatland form declaration #
Total comments: 35
Patch Set 6 : Address problems pointed out; refactor items module; (template is currently broken, ignore it) #
Total comments: 7
Patch Set 7 : Uploaded constants.forms missing in previous patch set #
Total comments: 22
MessagesTotal messages: 11
http://codereview.appspot.com/6304060/diff/4001/MoinMoin/templates/meta_form.... File MoinMoin/templates/meta_form.html (right): http://codereview.appspot.com/6304060/diff/4001/MoinMoin/templates/meta_form.... MoinMoin/templates/meta_form.html:21: {% macro input(field) %} seems i have duplicated some work in forms.html. i'll have a look and see how it can be improved.
Sign in to reply to this message.
http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... File MoinMoin/apps/frontend/meta_form.py (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... MoinMoin/apps/frontend/meta_form.py:17: Input = String.with_properties(widget='input', type='text') input is also used for quite some other sort of html ui elements (checkbox, radiobox), so using Input here is maybe a bit too generic. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... MoinMoin/apps/frontend/meta_form.py:24: # its field_schema class property. sounds legit. if fields_schema.extend() is all you need to do to make flatland happy, it looks ok. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... MoinMoin/apps/frontend/meta_form.py:27: pass instead of "pass", you can also just put a docstring here. for example the stuff you put as comment above this class. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:285: return deco good idea to reduce duplication. not sure if the removed code duplication is already worth the increased complexity, but if we have more views of same style, it might pay out in future. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:437: def modifier(view, request_args_map={}, must_may_write=True, abort404=True): maybe move this below presenter definition? must_may_write sounds a bit strange, maybe require_write or check_write? http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:471: def modify_meta(item): so we now have metadata editing separate from data editing? do we need 2 edits if we want to change both? http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/meta_form... File MoinMoin/templates/meta_form.html (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/meta_form... MoinMoin/templates/meta_form.html:26: {% endmacro %} did you test this stuff?
Sign in to reply to this message.
some comments for now http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... File MoinMoin/apps/frontend/meta_form.py (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... MoinMoin/apps/frontend/meta_form.py:17: Input = String.with_properties(widget='input', type='text') if that is a constant it PEP8 tells to have all letters upperacased INPUT http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:23: from functools import wraps \n before try http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:90: Disallow: /+modifymeta/ the first then +modifydata ? or a parameter for +modify to distinguish between meta and data http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:269: def presenter(view, add_trail=False, abort404=True): docstring needed that's from the refacroring part http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:270: def deco(wrapped): naming sounds hackish add a docstring too, too much wrapped names for me, lets see what thomas says. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:458: @modifier('modify', request_args_map={'contenttype':''}, abort404=False) this should be default application/octed-stream for an unknown mimetype, or? http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:494: # it's more elegant than stripping the prefixes manually. I'm not sure if this is the only method, do you discussed that with jek? 14:18 < xiaq> yeah i asked on #discorporate 14:18 < xiaq> jek wasn't there but alexmorega answered my question 14:18 < xiaq> one approach is to flatten and strip the prefix manually 14:18 < xiaq> another is to copy-init the sub-element so as to detach it from its parent 14:19 < xiaq> which is the approach i took - there is some comments in the code too 14:21 < xiaq> i went through flatland doc and source - and there seems no direct way to generate "relative names" 14:21 < xiaq> (when flatenning) http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/meta_form... File MoinMoin/templates/meta_form.html (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/meta_form... MoinMoin/templates/meta_form.html:6: {{- widget(form[field.name]) }} this is currently only for a text input form I think we should have different widget_formtype names and select the one needed based on some info from field / meta entry. Should a meta entry always be a dictionary or named list?: e.g. 'acl': {'widget': 'widget_acl', value:'', ... }, ... 'contenttype': {'widget': 'widget_text', 'value': "text/x.moin.wiki;charset=utf-8", ... }, ... http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/modify_me... File MoinMoin/templates/modify_meta.html (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/modify_me... MoinMoin/templates/modify_meta.html:5: {% set title = _("Modifying metadata of '%(item_name)s'", item_name=item_name) %} translation of item_name makes a translation complicated http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/modify_me... MoinMoin/templates/modify_meta.html:13: <dl> this part needs to be highliy variable What kind of elements are rendered must be requested from the available meta data. (May be we need a filter what is enabled to change) or a minimum set for a non existing item. That also needs some kind of toolbox to have some new entries http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/modify_me... MoinMoin/templates/modify_meta.html:14: {{ forms.render_textcha(gen, form) }} likly the textcha should not be rendered with the meta data editor It needs to be in a different location, not separated in one for edit data and another one for meta. You are right for meta changes / addings a textcha must also be answered.
Sign in to reply to this message.
http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... File MoinMoin/apps/frontend/meta_form.py (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... MoinMoin/apps/frontend/meta_form.py:17: Input = String.with_properties(widget='input', type='text') On 2012/06/09 13:02:00, Reimar Bauer wrote: > if that is a constant it PEP8 tells to have all letters upperacased > > INPUT Okay, i'll put INPUT = 'input' and TEXT = 'text' in a new constants.forms module. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... MoinMoin/apps/frontend/meta_form.py:17: Input = String.with_properties(widget='input', type='text') On 2012/06/09 12:54:04, ThomasJWaldmann wrote: > input is also used for quite some other sort of html ui elements (checkbox, > radiobox), so using Input here is maybe a bit too generic. Alright, I'll rename it to Text. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... MoinMoin/apps/frontend/meta_form.py:24: # its field_schema class property. On 2012/06/09 12:54:04, ThomasJWaldmann wrote: > sounds legit. if fields_schema.extend() is all you need to do to make flatland > happy, it looks ok. Yeah... but I'm also looking at alternative methods, this looks ugly. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/meta_... MoinMoin/apps/frontend/meta_form.py:27: pass On 2012/06/09 12:54:04, ThomasJWaldmann wrote: > instead of "pass", you can also just put a docstring here. > for example the stuff you put as comment above this class. Got it. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:23: from functools import wraps On 2012/06/09 13:02:00, Reimar Bauer wrote: > \n before try ok. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:90: Disallow: /+modifymeta/ On 2012/06/09 13:02:00, Reimar Bauer wrote: > the first then +modifydata ? > > or a parameter for +modify to distinguish between meta and data Hmm, currently +modify also presents a (primary) metadata editor. +modifymeta may be just merged into +modify later. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:269: def presenter(view, add_trail=False, abort404=True): On 2012/06/09 13:02:00, Reimar Bauer wrote: > docstring needed > > that's from the refacroring part okay. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:270: def deco(wrapped): On 2012/06/09 13:02:00, Reimar Bauer wrote: > naming sounds hackish > okay, > add a docstring too, > okay. > too much wrapped names for me, lets see what thomas says. seems decorators have to be written this way... http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:437: def modifier(view, request_args_map={}, must_may_write=True, abort404=True): On 2012/06/09 12:54:04, ThomasJWaldmann wrote: > maybe move this below presenter definition? > > must_may_write sounds a bit strange, maybe require_write or check_write? ok. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:458: @modifier('modify', request_args_map={'contenttype':''}, abort404=False) On 2012/06/09 13:02:00, Reimar Bauer wrote: > this should be default application/octed-stream for an unknown mimetype, or? There is a catch here, i use empty value to mean "same as key". see presenter code above. I'll document the behavior. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:471: def modify_meta(item): On 2012/06/09 12:54:04, ThomasJWaldmann wrote: > so we now have metadata editing separate from data editing? > do we need 2 edits if we want to change both? I'll try to merge +modifymeta into +modify. The main problem is that +modify currents presents all metadata key-value pairs, while +modifymeta only let you edit the ones with *known* keys. This can be fixed by presenting the unknown ones in plain JSON, and merge the meta_form part with the plain JSON part. Still open to discussion though. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:494: # it's more elegant than stripping the prefixes manually. On 2012/06/09 13:02:00, Reimar Bauer wrote: > I'm not sure if this is the only method, do you discussed that with jek? > 14:18 < xiaq> yeah i asked on #discorporate > 14:18 < xiaq> jek wasn't there but alexmorega answered my question > 14:18 < xiaq> one approach is to flatten and strip the prefix manually > 14:18 < xiaq> another is to copy-init the sub-element so as to detach it from > its parent > 14:19 < xiaq> which is the approach i took - there is some comments in the code > too > 14:21 < xiaq> i went through flatland doc and source - and there seems no direct > way to generate "relative names" > 14:21 < xiaq> (when flatenning) Yeah if there is too much overhead in copy-initing another Flatland Form, we optimize that later. But premature optimization is the root of all evil isn't it ;-) http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/meta_form... File MoinMoin/templates/meta_form.html (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/meta_form... MoinMoin/templates/meta_form.html:6: {{- widget(form[field.name]) }} On 2012/06/09 13:02:00, Reimar Bauer wrote: > this is currently only for a text input form > I think we should have different widget_formtype names > and select the one needed based on some info from field / meta entry. > > Should a meta entry always be a dictionary or named list?: > Uh, you mean a meta entry *schema* always be a dictionary or named list? Yes for that. > e.g. 'acl': {'widget': 'widget_acl', value:'', ... }, ... > 'contenttype': {'widget': 'widget_text', 'value': > "text/x.moin.wiki;charset=utf-8", ... }, ... > Yes - and this can be placed in Flatland element properties, like I did in meta_form.py for Input (going to be renamed to Text on next patch). http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/meta_form... MoinMoin/templates/meta_form.html:26: {% endmacro %} On 2012/06/09 12:54:04, ThomasJWaldmann wrote: > did you test this stuff? Yeah, and this is how it looks like: http://omni.tuna.tsinghua.edu.cn/~xiaq/moin2-metadata-editor.png Doesn't look very good honestly, but it works :-) http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/modify_me... File MoinMoin/templates/modify_meta.html (right): http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/modify_me... MoinMoin/templates/modify_meta.html:5: {% set title = _("Modifying metadata of '%(item_name)s'", item_name=item_name) %} On 2012/06/09 13:02:00, Reimar Bauer wrote: > translation of item_name makes a translation complicated Well IRCC the translator sees the format string, not the formatted one. BTW I was just following what's there in meta.html etc. http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/modify_me... MoinMoin/templates/modify_meta.html:13: <dl> On 2012/06/09 13:02:00, Reimar Bauer wrote: > this part needs to be highliy variable > Yes, and I already made it highly variable. In meta_form.html render_meta_form() consists of a loop, calling widget() for each children, and widget() is a dispatcher, taking hints from elements and decide what to render. So depending on how form['meta_form'] looks like the resulting metadata editor will look different. > What kind of elements are rendered must be requested from the available meta > data. (May be we need a filter what is enabled to change) > > or a minimum set for a non existing item. That also needs some kind of toolbox > to have some new entries http://codereview.appspot.com/6304060/diff/11002/MoinMoin/templates/modify_me... MoinMoin/templates/modify_meta.html:14: {{ forms.render_textcha(gen, form) }} On 2012/06/09 13:02:00, Reimar Bauer wrote: > likly the textcha should not be rendered with the meta data editor > > It needs to be in a different location, not separated in one for edit data and > another one for meta. You are right for meta changes / addings a textcha must > also be answered. ok, i'll just separate them.
Sign in to reply to this message.
Please review! :) http://codereview.appspot.com/6304060/diff/5008/MoinMoin/apps/frontend/meta_f... File MoinMoin/apps/frontend/meta_form.py (right): http://codereview.appspot.com/6304060/diff/5008/MoinMoin/apps/frontend/meta_f... MoinMoin/apps/frontend/meta_form.py:21: IntegerBar = Integer.with_properties(widget=INTEGER_BAR, lower=1, upper=5) Hard-coding lower=1, upper=5 is not clean and I'll see how to do this better. http://codereview.appspot.com/6304060/diff/5008/MoinMoin/apps/frontend/meta_f... MoinMoin/apps/frontend/meta_form.py:24: class _Dict(Dict): After reading source code of flatland.Form it seems that Form is just Dict with the sugar of defining Dicts with class syntax. Since we that's not what we want (class members are unordered), I decided to just subclass Dict directly. http://codereview.appspot.com/6304060/diff/5008/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/6304060/diff/5008/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:299: def modifier(view, request_args_map={}, require_write=True, abort404=True): the @modifier decorator is effectively dropped as +modifymeta is to be merged into +modify. http://codereview.appspot.com/6304060/diff/5008/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:470: def modify_item(item_name): See note on line 299. http://codereview.appspot.com/6304060/diff/5008/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:488: #@modifier('modifymeta') modify_meta is not used currently, it will be merged into +modify code. http://codereview.appspot.com/6304060/diff/5008/MoinMoin/templates/meta_form.... File MoinMoin/templates/meta_form.html (right): http://codereview.appspot.com/6304060/diff/5008/MoinMoin/templates/meta_form.... MoinMoin/templates/meta_form.html:1: {% import "forms.html" as forms %} Metadata editor templates is unused and broken ATM. Ignore them for now... http://codereview.appspot.com/6304060/diff/5008/MoinMoin/templates/modify_met... File MoinMoin/templates/modify_meta.html (right): http://codereview.appspot.com/6304060/diff/5008/MoinMoin/templates/modify_met... MoinMoin/templates/modify_meta.html:1: {% import "forms.html" as forms %} Metadata editor templates is unused and broken ATM. Ignore them for now...
Sign in to reply to this message.
Please see my comments on patch set 6.
Sign in to reply to this message.
for now, some comments, I need more time to do a second review later http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:299: def modifier(view, request_args_map={}, require_write=True, abort404=True): empty dict as default has some risks it should be None and internal set to {} for some more examples see http://www.deadlybloodyserious.com/2008/05/default-argument-blunders/ http://codereview.appspot.com/6304060/diff/9003/MoinMoin/constants/forms.py File MoinMoin/constants/forms.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/constants/forms.py#n... MoinMoin/constants/forms.py:1: # Copyright: 2011 MoinMoin:CheerXiao 2012 http://codereview.appspot.com/6304060/diff/9003/MoinMoin/constants/forms.py#n... MoinMoin/constants/forms.py:9: TEXT = "text" # single-line text No real opinion yet, but because of the various similiar constants I would prefix it with WIDGET_ to WIDGET_TEXT http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:711: if method == 'GET': Why do we need GET? http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:717: form = cls.from_flat(request.form.items() + request.files.items()) I prefer .append() for lists and "%s%s" % (x, y) or named for strings http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:728: if method == 'GET' and template_name is None and isinstance(self.rev, DummyRev): Why GET?
Sign in to reply to this message.
http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:299: def modifier(view, request_args_map={}, require_write=True, abort404=True): On 2012/06/14 11:20:56, Reimar Bauer wrote: > empty dict as default has some risks > > it should be None and internal set to {} > > for some more examples see > http://www.deadlybloodyserious.com/2008/05/default-argument-blunders/ This decorator is effectively neglected, see my comment on patch set 6. http://codereview.appspot.com/6304060/diff/9003/MoinMoin/constants/forms.py File MoinMoin/constants/forms.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/constants/forms.py#n... MoinMoin/constants/forms.py:1: # Copyright: 2011 MoinMoin:CheerXiao On 2012/06/14 11:20:56, Reimar Bauer wrote: > 2012 oh, thanks. http://codereview.appspot.com/6304060/diff/9003/MoinMoin/constants/forms.py#n... MoinMoin/constants/forms.py:9: TEXT = "text" # single-line text On 2012/06/14 11:20:56, Reimar Bauer wrote: > No real opinion yet, but because of the various similiar constants I would > prefix it with WIDGET_ to WIDGET_TEXT ok. http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:711: if method == 'GET': On 2012/06/14 11:20:56, Reimar Bauer wrote: > Why do we need GET? both GET and POST of +modify is handled in the items module, this is it worked before my refactor. see line 705 on the left. http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:717: form = cls.from_flat(request.form.items() + request.files.items()) On 2012/06/14 11:20:56, Reimar Bauer wrote: > I prefer .append() for lists and "%s%s" % (x, y) or named for strings again, this is just how it was written before my refactor... http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:728: if method == 'GET' and template_name is None and isinstance(self.rev, DummyRev): On 2012/06/14 11:20:56, Reimar Bauer wrote: > Why GET? see comment above.
Sign in to reply to this message.
http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:488: #@modifier('modifymeta') commented? http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:496: extra_meta = dict([(k, meta_dict.pop(k)) for k in extra_meta_keys]) where is this used? http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:1298: class Draw(TarMixin, Image): if you touch the drawing stuff, make sure that it still works (as for all other refactorings you do). also, if possible, keep refactorings and new stuff separate. http://codereview.appspot.com/6304060/diff/9003/MoinMoin/templates/modify_met... File MoinMoin/templates/modify_meta.html (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/templates/modify_met... MoinMoin/templates/modify_meta.html:16: {{ render_meta_form(form['meta_form'], extra_meta) }} indentation error
Sign in to reply to this message.
http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:488: #@modifier('modifymeta') On 2012/06/14 13:17:53, ThomasJWaldmann wrote: > commented? yeah, i'm merging it into +modify. http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:496: extra_meta = dict([(k, meta_dict.pop(k)) for k in extra_meta_keys]) On 2012/06/14 13:17:53, ThomasJWaldmann wrote: > where is this used? The idea is to have metadata with unknown (=not present in the MetaForm) meta keys presented in JSON so that no functionality is lost with the new metadata editor. http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:1298: class Draw(TarMixin, Image): On 2012/06/14 13:17:53, ThomasJWaldmann wrote: > if you touch the drawing stuff, make sure that it still works (as for all other > refactorings you do). > Yeah, I tested svgdraw, which is not broken by the refactor. But not the other two (they're Java applets, and I haven't got my Firefox work with Java applets...) > also, if possible, keep refactorings and new stuff separate. ok, i'll try to do that in future. http://codereview.appspot.com/6304060/diff/9003/MoinMoin/templates/modify_met... File MoinMoin/templates/modify_meta.html (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/templates/modify_met... MoinMoin/templates/modify_meta.html:16: {{ render_meta_form(form['meta_form'], extra_meta) }} On 2012/06/14 13:17:53, ThomasJWaldmann wrote: > indentation error going to fix...
Sign in to reply to this message.
http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:488: #@modifier('modifymeta') On 2012/06/14 13:28:28, xiaq wrote: > On 2012/06/14 13:17:53, ThomasJWaldmann wrote: > > commented? > > yeah, i'm merging it into +modify. OK, just clean up in the end. :) http://codereview.appspot.com/6304060/diff/9003/MoinMoin/apps/frontend/views.... MoinMoin/apps/frontend/views.py:496: extra_meta = dict([(k, meta_dict.pop(k)) for k in extra_meta_keys]) sure, but you do not use extra_meta variable yet.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
