MoinMoin: Fix #104

[ThomasJWaldmann, 2011-12-24 00:23:31]

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

http://codereview.appspot.com/5504083/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:1397: if inspect.isclass(f):
can you do it without inspect?

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

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1337: is_ajax =
request.headers.get('X-Requested-With') == 'XMLHttpRequest'
iirc werkzeug has an attribute for that

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/static/js/common.js
File MoinMoin/static/js/common.js (right):

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/static/js/common.js#...
MoinMoin/static/js/common.js:1040: jQuery(document).ready(initMoinUsersettings);
somehow this all seems much too complicated.

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/templates/usersettin...
File MoinMoin/templates/usersettings_forms.html (right):

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/templates/usersettin...
MoinMoin/templates/usersettings_forms.html:80: {% endmacro %}
why do you need macros if it is just one call?

[julian.brost, 2011-12-24 13:47:14]

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

http://codereview.appspot.com/5504083/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:1397: if inspect.isclass(f):
On 2011/12/24 00:23:31, Thomas.J.Waldmann wrote:
> can you do it without inspect?

yes. is there something wrong with inspect?

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

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1337: is_ajax =
request.headers.get('X-Requested-With') == 'XMLHttpRequest'
On 2011/12/24 00:23:31, Thomas.J.Waldmann wrote:
> iirc werkzeug has an attribute for that

fixed that

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/static/js/common.js
File MoinMoin/static/js/common.js (right):

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/static/js/common.js#...
MoinMoin/static/js/common.js:1040: jQuery(document).ready(initMoinUsersettings);
On 2011/12/24 00:23:31, Thomas.J.Waldmann wrote:
> somehow this all seems much too complicated.

do you have any suggestions? ;)

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/templates/usersettin...
File MoinMoin/templates/usersettings_forms.html (right):

http://codereview.appspot.com/5504083/diff/2001/MoinMoin/templates/usersettin...
MoinMoin/templates/usersettings_forms.html:80: {% endmacro %}
On 2011/12/24 00:23:31, Thomas.J.Waldmann wrote:
> why do you need macros if it is just one call?

why just one call? it's called from usersettings.html and from
usersettings_ajax.html

[ThomasJWaldmann, 2011-12-24 15:27:54]

http://codereview.appspot.com/5504083/diff/1009/MoinMoin/static/js/common.js
File MoinMoin/static/js/common.js (right):

http://codereview.appspot.com/5504083/diff/1009/MoinMoin/static/js/common.js#...
MoinMoin/static/js/common.js:1045: var newform = $(data.form);
are you using tabs? don't.

[Reimar Bauer, 2011-12-26 20:35:57]

what are the differences in the html, css files?
should there some?

check if === is better than == for comparison

http://codereview.appspot.com/5504083/diff/20/MoinMoin/static/js/common.js
File MoinMoin/static/js/common.js (right):

http://codereview.appspot.com/5504083/diff/20/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:994: // move all tab titles to an <ul> at the
beginning of .moin-tabs
indenting looks strange

keep indenting method. not sure if tabs are used in that file. if so keep those
too. We prefer 4 blanks per level, but not if the file is completly done by
tabs.
later on it can be refactored.

check also other places

http://codereview.appspot.com/5504083/diff/20/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:1040: if ($(this).data('initialForm') ==
$(this).serialize()) {
is it == or === ?

see also

https://bitbucket.org/thomaswaldmann/moin-2.0/issue/38/jslint-our-js-stuff

or

http://www.w3schools.com/js/js_comparisons.asp

[julian.brost, 2011-12-26 20:45:18]

http://codereview.appspot.com/5504083/diff/20/MoinMoin/static/js/common.js
File MoinMoin/static/js/common.js (right):

http://codereview.appspot.com/5504083/diff/20/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:994: // move all tab titles to an <ul> at the
beginning of .moin-tabs
On 2011/12/26 20:35:57, Reimar Bauer wrote:
> indenting looks strange
> 
> keep indenting method. not sure if tabs are used in that file. if so keep
those
> too. We prefer 4 blanks per level, but not if the file is completly done by
> tabs.
> later on it can be refactored.
> 
> check also other places

Oops, missed some tabs.

http://codereview.appspot.com/5504083/diff/20/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:1040: if ($(this).data('initialForm') ==
$(this).serialize()) {
On 2011/12/26 20:35:57, Reimar Bauer wrote:
> is it == or === ?
> 
> see also
> 
> https://bitbucket.org/thomaswaldmann/moin-2.0/issue/38/jslint-our-js-stuff
> 
> or
> 
> http://www.w3schools.com/js/js_comparisons.asp

Using === wouldn't change anything as types are identical (because the value
that data() returns only get set to $(this).serialize() and nothing else).

[ThomasJWaldmann, 2011-12-26 23:07:11]

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

http://codereview.appspot.com/5504083/diff/12/MoinMoin/apps/frontend/views.py...
MoinMoin/apps/frontend/views.py:1401: if response['redirect'] is not None or not
request.is_xhr:
can you explain the condition?

http://codereview.appspot.com/5504083/diff/12/MoinMoin/apps/frontend/views.py...
MoinMoin/apps/frontend/views.py:1413: # TODO
always write TODO <what> (or just DO IT :)

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js
File MoinMoin/static/js/common.js (right):

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:987: function initMoinTabs($) {
misses "docstring", explain what it does / how it does it.

don't write "init tabs" :)

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:1007: function updateFromLocationHash() {
hash?

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:1021: setInterval(updateFromLocationHash, 40); //
there is no event for that
for what?

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:1027: function initMoinUsersettings($) {
above you indented by 4, here by 8.

[julian.brost, 2011-12-26 23:21:52]

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

http://codereview.appspot.com/5504083/diff/12/MoinMoin/apps/frontend/views.py...
MoinMoin/apps/frontend/views.py:1401: if response['redirect'] is not None or not
request.is_xhr:
On 2011/12/26 23:07:12, Thomas.J.Waldmann wrote:
> can you explain the condition?

We can only use flash() if the whole page reloads. This is only the case if it's
not an AJAX/XHR request (not request.is_xhr) or we redirect anyway
(response['redirect'] is not None).

Otherwise we just send the flash messages via the returned JSON and display them
from the javascript code.

http://codereview.appspot.com/5504083/diff/12/MoinMoin/apps/frontend/views.py...
MoinMoin/apps/frontend/views.py:1413: # TODO
On 2011/12/26 23:07:12, Thomas.J.Waldmann wrote:
> always write TODO <what> (or just DO IT :)

That was just an marker for me so that I don't forget. This is fixed in the
latest patch set.

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js
File MoinMoin/static/js/common.js (right):

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:987: function initMoinTabs($) {
On 2011/12/26 23:07:12, Thomas.J.Waldmann wrote:
> misses "docstring", explain what it does / how it does it.
> 
> don't write "init tabs" :)

What about this? That's how it is in the latest version.

// find all .moin-tabs elements and initialize them

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:1007: function updateFromLocationHash() {
On 2011/12/26 23:07:12, Thomas.J.Waldmann wrote:
> hash?

The part of the URL after the hash symbol (including the hash symbol). So for
"http://localhost:8080/+usersettings#personal" location.hash would be
"#personal".

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:1021: setInterval(updateFromLocationHash, 40); //
there is no event for that
On 2011/12/26 23:07:12, Thomas.J.Waldmann wrote:
> for what?

This is not absolutely necessary but otherwise you can't change the hash in the
URL manually.

http://codereview.appspot.com/5504083/diff/12/MoinMoin/static/js/common.js#ne...
MoinMoin/static/js/common.js:1027: function initMoinUsersettings($) {
On 2011/12/26 23:07:12, Thomas.J.Waldmann wrote:
> above you indented by 4, here by 8.

Fixed in the latest version.