Hi Leon.  Can you please reattach your patch, and make sure to select the right mimetype?  If it's attached as a proper patch file, we can review it right in bugzilla.  Thanks!

Also, since a search may return hundreds of notes, I think it might make sense to put the "Clear Search" button at the top of the note list, just under the search field.

Other refinements to consider are scrollbars (yuck?) or a "More Notes..." button that brings in more results with AJAX magic.  This makes sense whether or not the list of notes represents a search result.

Review of attachment 148922 [details] [review]:

Looks great, Leon!  Just a few comments:

::: notes/templates/notes/note_detail.html
@@ -22,2 +23,3 @@
-        <li class="note-item{% if n.pinned %} pinned{% endif %}"><a href="{{ n.get_absolute_url }}">{{ n.title|safe }}</a></li>
-{% for n in all_notes %}
+{% for n in list_notes %}
+        <li class="note-item{% if n.pinned %} pinned{% endif %}"><a href="{{ n.get_absolute_url }}{% if query%}?query={{query}}{% endif %}">{{ n.title|safe }}</a></li>
+    

It would be cleaner if you added a URLConf entry (in urls.py) and a new view to cover this query parameter case.  This way, you can use {% url snowy.notes.views.note_detail_with_query query %}.

Created attachment 149156 [details] [review]
New patch, seperating the search out into a seperate view and template

I changed the patch a bit, now the search has its own view and template. However, I did not manage to pass the query as a parameter via URLconf, mainly because the form only submits via GET. So at the moment, the note_detail_with_query view still extracts the query from the GET dictionary.

The specific part you mentioned didn't really become much nicer:

+{% for n in list_notes %}
+         <li class="note-item{% if n.pinned %} pinned{% endif %}">
+         <a href="{% url note_detail_with_query username=n.author.username, note_id=n.id, slug=n.slug %}?query={{query}}">{{ n.title|safe }}</a>
+         </li>
+{% endfor %}

The GET parameter name is still hardcoded and the url lookup got a lot longer. I could of course add a query parameter to note.get_absolute_url, but that would violate MVC. If this can be improved, I'd be happy to know how.

Brad, any feedback on the updated patch?

The structure of the first patch looks better to me. In the separate note_detail_with_query.html of the second patch, the search form and the note list had to be duplicated, making it more work to change the sidebar. Also when more pages get the sidebar, it seems these all would have to get special _with_query.html files too.

In the first patch,

  {% if query %}?query={{query}}{% endif %}

might be made cleaner by letting user_notes_list_with_query() set an extra context variable called url_params, which could set to either "?query=[query]" or "".

Also, you could remove the 'query' parameter from

  {% user_notes_list request author query as list_notes %}

since the tag handler can get that value from the request variable. Brad mentioned on IRC that passing in request.GET instead would be clearer, but currently also request.user is used within the tag.

This way you can avoid the query lookup in views.note_detail.

(There are some small whitespace inconsistencies in the code, between the {{ and {% tags.)

For the rest, both patches work well here and look good to me! I think refinements like putting the 'clear' button elsewhere and adding ajax magic should be posted in separate bugs.

Yeah, I actually agree with Sander.  Sorry for steering you wrong, Leon.

Created attachment 162587 [details] [review]
Updated for latest git master

My patch has been updated to reflect sander's suggestions and should apply cleanly to the latest git master.

This patch also removes the note_search view because the new functionality effectively replaces it. I also fixed the "# XXX: Replace this with a NotesManager" in the notes list templatetag along the way!

Looks pretty good, but it seems to limit the amount of results returned compared to the old search view.  I used to get 24 results for "mario", now I only get 18.

This is because settings.SNOWY_LIST_MAX_NOTES limits the number of notes displayed in the sidebar. By default, it is set to 18.

One solution I see is to put a "...and X more results" line in the sidebar that, when clicked, extends the list and adds a scrollbar. The same could also be done for the "More Notes..." link that redirects to a rather ugly list of all notes at the moment.

Another way to improve it would be to improve the search. Interpreting spaces as AND (so searching for "mario racing" also finds "mario kart racing") and adding an OR and a "notebook:" keyword would help to find notes and narrow the search down. Not really a fix, more of a workaround. However, I think it would be better to discuss these changes in a separate bug.

(In reply to comment #9)
> This is because settings.SNOWY_LIST_MAX_NOTES limits the number of notes
> displayed in the sidebar. By default, it is set to 18.

Then, in my opinion, we should ignore that limit when showing search results.

> One solution I see is to put a "...and X more results" line in the sidebar
> that, when clicked, extends the list and adds a scrollbar. The same could also
> be done for the "More Notes..." link that redirects to a rather ugly list of
> all notes at the moment.

I'd rather not do "More Results...".  It feels like too many steps.  I'd rather have all results shown in the sidebar, even if there are 400 of them.

We could clean it up in the future by having a "More Results..." button that added 18 to the sidebar each time you clicked it (kind of like on Twitter's web interface).  That would be AJAXy and cute but not required right now imho.

> Another way to improve it would be to improve the search. Interpreting spaces
> as AND (so searching for "mario racing" also finds "mario kart racing") and
> adding an OR and a "notebook:" keyword would help to find notes and narrow the
> search down. Not really a fix, more of a workaround. However, I think it would
> be better to discuss these changes in a separate bug.

Yup, this sounds like a separate bug (though definitely a good idea).

Created attachment 163208 [details] [review]
List all search results in the sidebar

Updated according to sandy's suggestion not to limit the amount of search results. I found that adding a scrollbar looks a bit out of place so I decided it would be better to just extend the whole page. This is the main change in the patch:

+ if not search_query:
+     # do not limit the amount of search results returned
+     list_notes = list_notes[:settings.SNOWY_LIST_MAX_NOTES

This looks good.  Leon, do you have commit access?

Nope, I don't have acces to the git repo (this is only my third patch to the gnome project :) )

Cool, thanks, pushed.  I noticed an error in my terminal when doing a search with an empty search box.  Might want to look into it, even though it technically "works":

[09/Jun/2010 19:20:57] "GET /sandy/notes/1/new-note-525/?query= HTTP/1.1" 200 6194
Traceback (most recent call last):

+ Trace 222328

I don't think this is related to this patch. I also get this error from time to time when working with the django development server. Usually the page will get stuck while loading and when I reload I get the error you posted in the terminal. It seems to occur at random for me (though I have the feeling that it happened more on Ubuntu than it does on my current Debian unstable system). Searching with an empty search box seems to work fine for me here.