Adding Bookmark functionality

[ThomasWaldmann, 2011-06-11 14:18:22]

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

http://codereview.appspot.com/4592048/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:553: valid_user = 1
you are abusing integers for booleans

for what do you need valid_user anyway?

http://codereview.appspot.com/4592048/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:660: user=valid_user,
naming consistency is nice to have.

so, in case you need to give that to the template (I don't think you need, see
comment there), why not naming it valid_user there also?

http://codereview.appspot.com/4592048/diff/1/MoinMoin/templates/global_histor...
File MoinMoin/templates/global_history.html (right):

http://codereview.appspot.com/4592048/diff/1/MoinMoin/templates/global_histor...
MoinMoin/templates/global_history.html:11: {% if user  %}
you could just use flaskg.user.valid

http://codereview.appspot.com/4592048/diff/1/MoinMoin/templates/global_histor...
MoinMoin/templates/global_history.html:14: {{ _("(currently set to %(bookmark)d
)", bookmark=bookmark|datetimeformat) }}
the output of datetimeformat is not an integer, thus %d is wrong

also, you are relying on english word sequence when appending "Bookmark" and
"currently set to ...". That might not work for other languages, don't do that.

http://codereview.appspot.com/4592048/diff/1/MoinMoin/templates/global_histor...
MoinMoin/templates/global_history.html:15: <a href="{{
url_for('frontend.bookmark', time=del) }}">delete bookmark</a>
del -> NameError

i18n missing

http://codereview.appspot.com/4592048/diff/1/MoinMoin/templates/global_histor...
MoinMoin/templates/global_history.html:17: {{ _("(not set)  ") }} <a href="{{
url_for('frontend.bookmark', time=current_timestamp) }}">set bookmark</a>
same issue with word sequence as above, i18n missing.

http://codereview.appspot.com/4592048/diff/1/MoinMoin/templates/global_histor...
MoinMoin/templates/global_history.html:29: {% if user %}
see above

http://codereview.appspot.com/4592048/diff/1/MoinMoin/templates/global_histor...
MoinMoin/templates/global_history.html:30: <a class="bookmark-link" href="{{
url_for('frontend.bookmark', time=latest_timestamp) }}">Set Bookmark</a>
i18n missing

http://codereview.appspot.com/4592048/diff/1/MoinMoin/templates/global_histor...
MoinMoin/templates/global_history.html:72: <div id="moin-bookmark-reached">{{
_("Bookmark Reached") }}</div>
small "r"?

http://codereview.appspot.com/4592048/diff/1/MoinMoin/themes/modernized/stati...
File MoinMoin/themes/modernized/static/css/common.css (right):

http://codereview.appspot.com/4592048/diff/1/MoinMoin/themes/modernized/stati...
MoinMoin/themes/modernized/static/css/common.css:224:
.moin-history-container-header h2 { display:inline; }
mind your whitespace style

[ThomasWaldmann, 2011-06-11 21:12:11]

http://codereview.appspot.com/4592048/diff/2003/MoinMoin/templates/global_his...
File MoinMoin/templates/global_history.html (right):

http://codereview.appspot.com/4592048/diff/2003/MoinMoin/templates/global_his...
MoinMoin/templates/global_history.html:14: {% set del = "del" %}
please give this string directly in url_for

del is a python statement, you are risking strange effects by using it as name.

http://codereview.appspot.com/4592048/diff/2003/MoinMoin/templates/global_his...
MoinMoin/templates/global_history.html:31: <a class="bookmark-link" href="{{
url_for('frontend.bookmark', time=latest_timestamp) }}">{{ _("Set Bookmark")
}}</a>
above you use "set bookmark", now you use "Set Bookmark" - intended?

[Reimar Bauer, 2011-06-12 11:38:33]

small PEP8 empty lines

and a question.

Bookmark versus Remind

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

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:551: 
no empty line needed

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:556: 
no empty line needed

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/templates/global_his...
File MoinMoin/templates/global_history.html (right):

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/templates/global_his...
MoinMoin/templates/global_history.html:14: {{ _("Bookmark (currently set to
%(bookmark)s). ", bookmark=bookmark_time|datetimeformat) }}
Remind instead of Bookmark?

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/templates/global_his...
MoinMoin/templates/global_history.html:17: {{ _("Bookmark (not set). ") }}<a
href="{{ url_for('frontend.bookmark', time=current_timestamp) }}">{{ _("Set
bookmark") }}</a>
We have used that "Set bookmark" a long time, and some user ever where confused
by that. It did not mean to add something to the browsers bookmarks. But a lot
of apps excatly doing that.

We likly do just 
"Set reminder to continue from here"

[sinha, 2011-06-12 13:37:08]

Bookmark vs Reminder Issue.

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/templates/global_his...
File MoinMoin/templates/global_history.html (right):

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/templates/global_his...
MoinMoin/templates/global_history.html:14: {{ _("Bookmark (currently set to
%(bookmark)s). ", bookmark=bookmark_time|datetimeformat) }}
On 2011/06/12 11:38:34, Reimar Bauer wrote:
> Remind instead of Bookmark?

Remind or Reminder ?

But all other functions in moin is implemented as the name `bookmark`, even the
url is as /+bookmark?time=..., So wouldn't it create any confusion ?

[ThomasWaldmann, 2011-06-12 13:50:16]

comments about usage of term "bookmark"

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/templates/global_his...
File MoinMoin/templates/global_history.html (right):

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/templates/global_his...
MoinMoin/templates/global_history.html:14: {{ _("Bookmark (currently set to
%(bookmark)s). ", bookmark=bookmark_time|datetimeformat) }}
On 2011/06/12 13:37:08, sinha wrote:
> On 2011/06/12 11:38:34, Reimar Bauer wrote:
> > Remind instead of Bookmark?
> 
> Remind or Reminder ?
> 
> But all other functions in moin is implemented as the name `bookmark`, even
the
> url is as /+bookmark?time=..., So wouldn't it create any confusion ?

Please stay with bookmark as term for this. We use this since ages and I don't
see a need to change it.

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/templates/global_his...
MoinMoin/templates/global_history.html:17: {{ _("Bookmark (not set). ") }}<a
href="{{ url_for('frontend.bookmark', time=current_timestamp) }}">{{ _("Set
bookmark") }}</a>
On 2011/06/12 11:38:34, Reimar Bauer wrote:
> We have used that "Set bookmark" a long time, and some user ever where
confused
> by that. It did not mean to add something to the browsers bookmarks. But a lot
> of apps excatly doing that.
> 
> We likly do just 
> "Set reminder to continue from here"
> 

A reminder is something else. Usually related to something actively alerting you
(by ringing, by paging, by emailing) at and about some specific date/time.

A bookmark in a paper book is closely related to what we use the term for in
moin. The paper bookmark tells "i have read until there" - and this is exactly
what we use it for on recent changes.

[Reimar Bauer, 2011-06-14 06:22:56]

ok

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

http://codereview.appspot.com/4592048/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1: # Copyright: 2003-2010
MoinMoin:ThomasWaldmann
ok from me