Improved the item metadata section GUI

Improved the item metadata section GUI

[RogerHaase, 2014-06-26 20:06:31]

https://codereview.appspot.com/110210043/diff/40001/MoinMoin/themes/basic/tem...
File MoinMoin/themes/basic/templates/meta.html (right):

https://codereview.appspot.com/110210043/diff/40001/MoinMoin/themes/basic/tem...
MoinMoin/themes/basic/templates/meta.html:21: <ul class="list-group">
These list-group classes are from bootstrap?  Can you put this code in
templates/util.html (or other shared location) and import it?  Would need a
comment to explain the classes are from bootstrap, but as modernized rendering
is ul/li default the classes should do no harm.

https://codereview.appspot.com/110210043/diff/40001/MoinMoin/themes/basic/tem...
MoinMoin/themes/basic/templates/meta.html:75: </ul>
old version was in alphabetical order which is helpful if you want to see a
particular field.  Still missing some fields: comment, name, name_old...others

[RogerHaase, 2014-06-27 15:16:46]

looks good, commit

[Thomas.J.Waldmann, 2014-06-27 15:27:34]

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/apps/frontend/vi...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/apps/frontend/vi...
MoinMoin/apps/frontend/views.py:553: meta_rendered=item._render_meta(),
why did you remove Markup()?

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/items/__init__.p...
MoinMoin/items/__init__.py:380: return self.meta_to_dict(self.meta,
use_filter=False)
that doesn't look like "rendering" any more.

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/items/__init__.p...
MoinMoin/items/__init__.py:417: return json.dumps(meta, sort_keys=True,
indent=2, ensure_ascii=False)
well, if we keep that other method ("meta_to_dict"), the duplicate code here has
to go.

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/meta.html
File MoinMoin/templates/meta.html (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/meta.h...
MoinMoin/templates/meta.html:17: {{ utils.meta_info(meta_rendered) }}
guess why it is called "meta_rendered"?

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/utils....
File MoinMoin/templates/utils.html (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/utils....
MoinMoin/templates/utils.html:108: <li class="list-group-item">Action : {{
meta_rendered['action'] }}</li>
confusing naming.

meta_rendered suggests it is html, but it is a dict (and not rendered).

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/utils....
MoinMoin/templates/utils.html:109: <li class="list-group-item">Address : {{
meta_rendered['address'] }}</li>
you use bootstrap stuff here.

the code in utils is meant to be used by all themes, right?
will this be called also by non-bootstrap themes?

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/utils....
MoinMoin/templates/utils.html:158: <li class="list-group-item">Name Old :
Old Name:

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__....
MoinMoin/themes/__init__.py:483: :returns: whether link exists or not
you don't mean the link, you mean the item the link points to.

thus, method name and docs are confusing.

names are important, think of a better one.

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__....
MoinMoin/themes/__init__.py:489: if is_local_wiki(wikiname):
can you explain what you are doing here?

[ajgupta93, 2014-06-27 16:06:22]

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/apps/frontend/vi...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/apps/frontend/vi...
MoinMoin/apps/frontend/views.py:553: meta_rendered=item._render_meta(),
On 2014/06/27 15:27:34, Thomas.J.Waldmann wrote:
> why did you remove Markup()?

I wanted to use a dictionary in the template so that i could customize the view.
I'll change the name of the function _render_meta to something else.

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/items/__init__.p...
MoinMoin/items/__init__.py:380: return self.meta_to_dict(self.meta,
use_filter=False)
On 2014/06/27 15:27:34, Thomas.J.Waldmann wrote:
> that doesn't look like "rendering" any more.

I'll change the name.

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/meta.html
File MoinMoin/templates/meta.html (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/meta.h...
MoinMoin/templates/meta.html:17: {{ utils.meta_info(meta_rendered) }}
On 2014/06/27 15:27:34, Thomas.J.Waldmann wrote:
> guess why it is called "meta_rendered"?

I'll change the name

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/utils....
File MoinMoin/templates/utils.html (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/templates/utils....
MoinMoin/templates/utils.html:109: <li class="list-group-item">Address : {{
meta_rendered['address'] }}</li>
On 2014/06/27 15:27:34, Thomas.J.Waldmann wrote:
> you use bootstrap stuff here.
> 
> the code in utils is meant to be used by all themes, right?
> will this be called also by non-bootstrap themes?

Yes the non-bootstrap themes will work and treat this is as a normal list.

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__....
MoinMoin/themes/__init__.py:483: :returns: whether link exists or not
On 2014/06/27 15:27:34, Thomas.J.Waldmann wrote:
> you don't mean the link, you mean the item the link points to.
> 
> thus, method name and docs are confusing.
> 
> names are important, think of a better one.

Ok.. I'll change the name.

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__....
MoinMoin/themes/__init__.py:489: if is_local_wiki(wikiname):
On 2014/06/27 15:27:34, Thomas.J.Waldmann wrote:
> can you explain what you are doing here?

I am checking whether the item pointed to by the link exists in the current
wiki, so that i can display broken links differently in the template.

[Thomas.J.Waldmann, 2014-06-29 16:25:57]

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__....
MoinMoin/themes/__init__.py:489: if is_local_wiki(wikiname):
getInterwikiHome(name) returns the location of the user homepage of user <name>.
How is that related to a generic "item_exists" functionality?

[ajgupta93, 2014-06-30 16:31:20]

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__....
MoinMoin/themes/__init__.py:489: if is_local_wiki(wikiname):
On 2014/06/29 16:25:57, Thomas.J.Waldmann wrote:
> getInterwikiHome(name) returns the location of the user homepage of user
<name>.
> How is that related to a generic "item_exists" functionality?

I want to show an link to an item as a valid link in case it does not belong to
the local wiki. So thats why i check it and if it is not the local wiki, i set
exists as true in case of a non-local wiki.

[Thomas.J.Waldmann, 2014-07-06 15:35:37]

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/110210043/diff/20002/MoinMoin/themes/__init__....
MoinMoin/themes/__init__.py:489: if is_local_wiki(wikiname):
please answer my question.

[Thomas.J.Waldmann, 2014-07-12 15:42:47]

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/templates/utils...
File MoinMoin/templates/utils.html (right):

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/templates/utils...
MoinMoin/templates/utils.html:108: <li class="list-group-item">Action : {{
meta['action'] }}</li>
do a search on your code base and fix all those " :" cases where no blank should
be before the colon.

separate cs for that.

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/templates/utils...
MoinMoin/templates/utils.html:116: <a href="{{ item }}">
if this external link contains a query string with & character, I would expect
that jinja2 escapes this to &amp; which is not wanted in that case.

so you need to append | safe here to avoid that.

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:478: def item_exists(self, link_itemname):
why is this called "link_itemname" and not just itemname?

[ajgupta93, 2014-07-14 12:19:14]

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/templates/utils...
File MoinMoin/templates/utils.html (right):

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/templates/utils...
MoinMoin/templates/utils.html:108: <li class="list-group-item">Action : {{
meta['action'] }}</li>
On 2014/07/12 15:42:47, Thomas.J.Waldmann wrote:
> do a search on your code base and fix all those " :" cases where no blank
should
> be before the colon.
> 
> separate cs for that.

Done.

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/templates/utils...
MoinMoin/templates/utils.html:116: <a href="{{ item }}">
On 2014/07/12 15:42:47, Thomas.J.Waldmann wrote:
> if this external link contains a query string with & character, I would expect
> that jinja2 escapes this to &amp; which is not wanted in that case.
> 
> so you need to append | safe here to avoid that.

Done.

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:478: def item_exists(self, link_itemname):
On 2014/07/12 15:42:47, Thomas.J.Waldmann wrote:
> why is this called "link_itemname" and not just itemname?

Done.

[Thomas.J.Waldmann, 2014-07-17 16:10:33]

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/templates/utils...
File MoinMoin/templates/utils.html (right):

https://codereview.appspot.com/110210043/diff/140001/MoinMoin/templates/utils...
MoinMoin/templates/utils.html:116: <a href="{{ item }}">
no, not done. I doubt that works and even if it does: you fixed the wrong place
(and you didn't test it).