Improved search GUI

Improved search GUI

[RogerHaase, 2014-06-10 21:15:55]

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/constants/conte...
File MoinMoin/constants/contenttypes.py (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/constants/conte...
MoinMoin/constants/contenttypes.py:107: u'application/pdf':'PDF'
this seems like a duplication of things defined in items/content.py.  Is there
no way to use those values directly/indirectly?

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/templates/searc...
File MoinMoin/templates/search.html (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/templates/searc...
MoinMoin/templates/search.html:50: <td width="20%" valign="top">
this width and other styling would be better done in css

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

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:657: return rights
this was discussed here
http://moinmo.in/MoinMoinChat/Logs/moin-dev/2014-06-07#CA-7e52a95622d5f5d0b39...

my understanding was it was decided that this was not useful on the search
results page

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/basic/st...
File MoinMoin/themes/basic/static/custom-less/basic.less (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/basic/st...
MoinMoin/themes/basic/static/custom-less/basic.less:375: }
as we discussed, I dislike having to triplicate this code. Better to write it
once and import 3 times.  If needed, a theme could customize parts

[ajgupta93, 2014-06-11 09:22:17]

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/constants/conte...
File MoinMoin/constants/contenttypes.py (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/constants/conte...
MoinMoin/constants/contenttypes.py:107: u'application/pdf':'PDF'
On 2014/06/10 21:15:54, RogerHaase wrote:
> this seems like a duplication of things defined in items/content.py.  Is there
> no way to use those values directly/indirectly?

These things are not defined in the items/content.py. Moreover a similarly
defined function "shorten_content_type" was already present as a TODO task in
the themes/__init__.py -
https://codereview.appspot.com/109730047/patch/120001/130007

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/templates/searc...
File MoinMoin/templates/search.html (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/templates/searc...
MoinMoin/templates/search.html:50: <td width="20%" valign="top">
On 2014/06/10 21:15:54, RogerHaase wrote:
> this width and other styling would be better done in css

Ok.. will do that

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/basic/st...
File MoinMoin/themes/basic/static/custom-less/basic.less (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/basic/st...
MoinMoin/themes/basic/static/custom-less/basic.less:375: }
On 2014/06/10 21:15:54, RogerHaase wrote:
> as we discussed, I dislike having to triplicate this code. Better to write it
> once and import 3 times.  If needed, a theme could customize parts

Should i make a new css file in the static/css/ section like "common.css" and
also should i directly define css or code in stylus or lessc ?

[RogerHaase, 2014-06-12 04:27:32]

Please run ./ tests.

Don't add to this changeset, but your future plans should consider giving the
user some visual clue that there is a running ajax process -- change cursor to
wait or flash a message without making the page bounce.

fyi, now coding-std is complaining that stylus is adding an extra blank line at
the end of foobar common.css.

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:273: Add various terms to the filter for the
search query to for the selected file types
...query for...

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:282:
_filter.append(Term("contenttype",str(item)))
pep8 space after ,

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:336: filetypes = filetypes.split(',')[:-1] # To
remove the extra u'' at the end of the list
pep8 2 spaces before # of inline comment

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/constants/conte...
File MoinMoin/constants/contenttypes.py (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/constants/conte...
MoinMoin/constants/contenttypes.py:107: u'application/pdf':'PDF'
On 2014/06/11 09:22:17, ajgupta93 wrote:
> On 2014/06/10 21:15:54, RogerHaase wrote:
> > this seems like a duplication of things defined in items/content.py.  Is
there
> > no way to use those values directly/indirectly?
> 
> These things are not defined in the items/content.py. Moreover a similarly
> defined function "shorten_content_type" was already present as a TODO task in
> the themes/__init__.py -
> https://codereview.appspot.com/109730047/patch/120001/130007

ok, pep8 requires space after :

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/static/js/searc...
File MoinMoin/static/js/search.js (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/static/js/searc...
MoinMoin/static/js/search.js:31: var allrev = false, mtime = false,
filetypes='';
defining one variable per line would make this more readable. Need spaces in
"filetypes = '';"

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

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:50: 
pep8 2 blank lines

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:658: 
pep8 2 blank lines

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/basic/st...
File MoinMoin/themes/basic/static/custom-less/basic.less (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/basic/st...
MoinMoin/themes/basic/static/custom-less/basic.less:375: }
On 2014/06/11 09:22:17, ajgupta93 wrote:
> On 2014/06/10 21:15:54, RogerHaase wrote:
> > as we discussed, I dislike having to triplicate this code. Better to write
it
> > once and import 3 times.  If needed, a theme could customize parts
> 
> Should i make a new css file in the static/css/ section like "common.css" and
> also should i directly define css or code in stylus or lessc ?

Let's wait for tw to concur.

[RogerHaase, 2014-06-12 20:00:51]

Found one more nitpicky issue.

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/foobar/s...
File MoinMoin/themes/foobar/static/css/common.css (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/foobar/s...
MoinMoin/themes/foobar/static/css/common.css:454:
}.moin-suggestions{color:#f90;display:inline;padding-left:4px}
Just one more weird Stylus thing -- while this }.moin-sugg... works, it should
be fixed - see main.styl

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/foobar/s...
File MoinMoin/themes/foobar/static/css/stylus/main.styl (right):

https://codereview.appspot.com/109730047/diff/120001/MoinMoin/themes/foobar/s...
MoinMoin/themes/foobar/static/css/stylus/main.styl:1160: 
move the changes below to here so the media print stays at the bottom and
common.css is better formatted.

[Thomas.J.Waldmann, 2014-06-14 19:18:43]

note: i focussed on the python code.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:273: Add various terms to the filter for the
search query to for the selected file types
to for?

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:274: in the advanced search options.
remove the "advanced " from "advanced search options" at all places here also
when you remove it from the ui

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:276: :param name: _filter(the current filter),
filetypes(list of selected filetypes)
you misunderstood how these docstrings are made.

:param _filter: good description

you need to do a global review of ALL your docstrings, many of them are wrong.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:279: if len(filetypes):
if filetypes:

or even: leave it away. checking whether something is in an empty list should be
rather quick.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:280: alltrue = "all" in filetypes
all_types?

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:281: if ("markup" in filetypes) or alltrue:
are the parentheses required here? also check for all other parentheses usages.

also, if you do such an expression, list the easy-to-compute stuff first.
evaluating a boolean is very cheap.
evaluating "str in list-of-str" is more expensive.
python does short circuit boolean expressions.
so if "A or B" is evaluated and A is True, it will never evaluate B.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:283: _filter.append(Term("contenttype",
str(item)))
why are you using str() here?

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:301: _filter.append(Term("contenttype",
str(item)))
you have quite a lot of duplication here.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:302: _filter = Or(_filter)
so, you get some previous _filter and append a lot of filetype terms and then
you just OR them ALL?

I doubt this is the wanted/expected behaviour.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:314: 3. new - search new items first
you don't mean "search first".

you mean sort the results, so the newer results are first.

also, the attribute name and its values are badly chosen.

you don't mean mtime (that is the modification time), you mean the sorting (or
time_sorting in case there are others also).

and the values could be "none", "ascending", "descending" (or "none",
"newest-first", "oldest-first" if you prefer).

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:320: facets.append(sorting.FieldFacet("mtime"))
well, even if reverse=False is the default, it would look better if you gave it
nevertheless.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:350: idx_name = LATEST_REVS
not sure why you made 4 lines out of one.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:355: _filter = add_file_filters(_filter,
filetypes)
hah, you're lucky. your code works for _filter = [].

but just imagine, the _filter has a non-empty content before this call...

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/constants/conte...
File MoinMoin/constants/contenttypes.py (right):

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/constants/conte...
MoinMoin/constants/contenttypes.py:31: u'application/docbook+xml;charset=utf-8'
add a , here also

do a global refactoring of all your lists/dicts/etc. and always have the last
element formatted in the same way as any other one [== have the comma]).

But remember: do it in python, but not e.g. in json (not allowed there).

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

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:636: return ctype
return CONTENTTYPES_MAP.get(contenttype, u"Unknown")

maybe check how many callers this has. if it is just 1, then you maybe want to
inline this.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:647: rights_list = []
one usually doesn't include the type in the name (no "..._list").

a good convention is:
name singular == one thing.
name plural == multiple things (a list, a set).

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:648: rights = u''
not needed.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:657: rights = " | ".join(str(right) for right in
rights_list)
why str()? you'll only get UnicodeEncodeError exceptions with no good reason.

and after you got rid of str(), think again about what's left / what's really
needed.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/themes/__init__...
MoinMoin/themes/__init__.py:667: app.jinja_env.filters['shorten_ctype'] =
content_map
why not keep the old name? or "shorten_ctype"?

[ajgupta93, 2014-06-18 17:50:00]

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:302: _filter = Or(_filter)
On 2014/06/14 19:18:42, Thomas.J.Waldmann wrote:
> so, you get some previous _filter and append a lot of filetype terms and then
> you just OR them ALL?
> 
> I doubt this is the wanted/expected behaviour.

I am quite sure that that's the way it should be. The user has a group of items
from which they want the search results, so it can be any on the groups and
thats why the "OR"

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:350: idx_name = LATEST_REVS
On 2014/06/14 19:18:42, Thomas.J.Waldmann wrote:
> not sure why you made 4 lines out of one.

The earlier version always evaluated to true as "request.get.args('history')"
returns a string be it "true" or "false" and as such "if history" will always
evaluate to True.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:355: _filter = add_file_filters(_filter,
filetypes)
On 2014/06/14 19:18:42, Thomas.J.Waldmann wrote:
> hah, you're lucky. your code works for _filter = [].
> 
> but just imagine, the _filter has a non-empty content before this call...

The variable _filter even originally was defined just before this call, and isnt
even used before this. So it'll be empty when I call this function. Also even if
it is non empty I am just appending more filters to that, so it shouldnt matter.

[Thomas.J.Waldmann, 2014-06-19 16:38:42]

again, i concentrated on the .py stuff

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:302: _filter = Or(_filter)
i didn't say that ORing the stuff you compute here is wrong.

but you also OR it with what you had in the _filter before.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:350: idx_name = LATEST_REVS
well, you can simple change the boolean expression that was wrong, no need to
expand it to 4 lines.

https://codereview.appspot.com/109730047/diff/180001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:355: _filter = add_file_filters(_filter,
filetypes)
it does matter as you do an OR above there (and you likely want an AND). But as
it is empty, you didn't see the difference.

https://codereview.appspot.com/109730047/diff/240001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/109730047/diff/240001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:299: _filter.append(Term("contenttype",
itemtype))
better! :)

[RogerHaase, 2014-06-20 14:15:22]

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/static/css/comm...
File MoinMoin/static/css/common.css (right):

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/static/css/comm...
MoinMoin/static/css/common.css:1: .moin-suggestions {
add a comment at top to say how this is used

also:

TODO: When bootstrap/lessc vs stylus issue is resolved, this file should be
converted and colors changed to variables.

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/static/css/comm...
MoinMoin/static/css/common.css:16: color: grey;
gray?

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/static/js/searc...
File MoinMoin/static/js/search.js (right):

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/static/js/searc...
MoinMoin/static/js/search.js:16: $(document).on("click",
".moin-searchoption-bar", (function(){
a nit, but since you separate most words with -, also do ..search-option..

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/templates/searc...
File MoinMoin/templates/search.html (right):

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/templates/searc...
MoinMoin/templates/search.html:19: <div class="moin-searchoption-bar"><h4
style="display:inline-block">Search Options</h4><span
class="icon-chevron-down"></span></div>
search-option

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/themes/basic/st...
File MoinMoin/themes/basic/static/custom-less/basic.less (right):

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/themes/basic/st...
MoinMoin/themes/basic/static/custom-less/basic.less:331: color: #c8ddf1;
these colors should be variables pulled from ?

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/themes/foobar/s...
File MoinMoin/themes/foobar/static/css/stylus/main.styl (right):

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/themes/foobar/s...
MoinMoin/themes/foobar/static/css/stylus/main.styl:1165: color grey
should be in color_palete.styl; also s/b gray:
http://www.w3schools.com/cssref/css_colornames.asp

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/themes/moderniz...
File MoinMoin/themes/modernized/static/css/stylus/main.styl (right):

https://codereview.appspot.com/109730047/diff/260001/MoinMoin/themes/moderniz...
MoinMoin/themes/modernized/static/css/stylus/main.styl:1661: color grey
this should pull a color from color_pallete.styl

[Thomas.J.Waldmann, 2014-06-23 12:56:18]

maybe remember that this is a typical antipattern you should avoid:

if expression:
  something = True
else:
  something = False

better: something = expression

https://codereview.appspot.com/109730047/diff/300001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/109730047/diff/300001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:339: history = False
history = request.args.get('history') == "true"

https://codereview.appspot.com/109730047/diff/300001/MoinMoin/static/js/searc...
File MoinMoin/static/js/search.js (right):

https://codereview.appspot.com/109730047/diff/300001/MoinMoin/static/js/searc...
MoinMoin/static/js/search.js:37: }
can you just directly assign the boolean value of the expression without using
initialization for false and a "if" for true, like I showed in the python code?

https://codereview.appspot.com/109730047/diff/300001/MoinMoin/themes/basic/st...
File MoinMoin/themes/basic/static/custom-less/moin-variables.less (right):

https://codereview.appspot.com/109730047/diff/300001/MoinMoin/themes/basic/st...
MoinMoin/themes/basic/static/custom-less/moin-variables.less:31:
@search_option_bar_text_color:       #c8ddf1;
did you use tabs here of why does the indentation look strange here?

[Thomas.J.Waldmann, 2014-06-23 14:03:44]

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/apps/frontend/v...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:274: in the advanced search options.
still "advanced"?

i told you multiple times to fix this globally...

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:278: :returns: the required _filter for the
advanced search query
same here

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:303: _filter = And(_filter)
good, And and Or finally right. \o/

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:312: :param time_sorting: defines the sorting
order and can have 3 types to values :
"can have 3 types to values"???

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/apps/frontend/v...
MoinMoin/apps/frontend/views.py:316: :returns: required facets for the advanced
search query
^^

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/constants/conte...
File MoinMoin/constants/contenttypes.py (right):

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/constants/conte...
MoinMoin/constants/contenttypes.py:108: }
btw, where did you get these lists / mappings from? is it copy & pasted and then
rearranged from somewhere else in the code?

i vaguely remember we have similar datastructures somewhere...

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/static/js/searc...
File MoinMoin/static/js/search.js (right):

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/static/js/searc...
MoinMoin/static/js/search.js:32: var allrev = false;
the "= false" is not needed.

[ajgupta93, 2014-06-25 18:17:19]

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/constants/conte...
File MoinMoin/constants/contenttypes.py (right):

https://codereview.appspot.com/109730047/diff/320001/MoinMoin/constants/conte...
MoinMoin/constants/contenttypes.py:108: }
On 2014/06/23 14:03:44, Thomas.J.Waldmann wrote:
> btw, where did you get these lists / mappings from? is it copy & pasted and
then
> rearranged from somewhere else in the code?
> 
> i vaguely remember we have similar datastructures somewhere...

No all the definitions are not there. Actually this was a TODO -
https://codereview.appspot.com/109730047/patch/320001/330009

[RogerHaase, 2014-06-25 20:37:24]

There are still some minor problems, but as this is better than what we have and
your patch is already too big, commit and work on the small issues later...

1. it is not obvious to click on search options bar to expand/contract
(glyphicons not working)

2. when I click Markup Text checkbox the All checkbox does not get unchecked.

3. if I uncheck All, the list of hits does not zero