Edgewall Software
Modify

Opened 19 years ago

Closed 18 years ago

#1269 closed defect (fixed)

Search an expression with CamelCase does not work

Reported by: oliver@… Owned by: Christian Boos
Priority: normal Milestone: 0.10
Component: search system Version: 0.8.1
Severity: normal Keywords: search CamelCase review
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

If I search an expression with CamelCase I get a wiki page. I dont get a search result page. Is that the right behaviour ?

Attachments (4)

quickjump-escape.diff (823 bytes ) - added by Alec Thomas 18 years ago.
Quickjump escape
quickjump.diff (2.3 KB ) - added by Alec Thomas 18 years ago.
Quickjump only when using top search bar
quickjump+missing.diff (4.0 KB ) - added by Alec Thomas 18 years ago.
As above plus add missing class to milestone and reports
quickjump+missing-r3431.diff (5.7 KB ) - added by Christian Boos 18 years ago.
Same as above, but the style is different: the quickjump results are more integrated with the other results, but still stand out on top of them.

Download all attachments as: .zip

Change History (27)

comment:1 by cboos@…, 19 years ago

At least, that's the intended behavior.

But I see your point. If your a looking for, e.g. a class name, you would like to see the commit messages or the tickets talking about that class, not the empty Wiki page for that name.

A simple fix would be to redirect to the wiki page only if such a page already exists.

In all cases, simply prefixing the CamelCase name by one or more spaces bypasses the redirection and does a regular search for that name.

comment:2 by Christopher Lenz, 19 years ago

The proper way to escape the quick search functionality is to prefix with an exclamation mark, e.g. CamelCase.

comment:3 by Matthew Good <trac matt-good net>, 19 years ago

What cmlenz meant to say was !CamelCase

(! in the wiki escaped CamelCase from being a wiki link)

comment:4 by david.tulloh@…, 19 years ago

This is particularily annoying when you use the advanced search and specifically uncheck wiki pages.

For example I was looking for PythonHandler and ModPythonHandler tickets to see if they relate to a problem I have. Getting empty wiki pages instead of tickets was very unexpected.

comment:5 by Christian Boos, 19 years ago

In the trunk, the search module has been reworked a bit, and the "quick jump" feature is now missing.

As this has to be reimplemented, we should take this opportunity to fix the problem you raised.

Possible solutions:

  • only quick jump if the corresponding object exists, otherwise revert to normal search.
  • same as above, but in addition, don't jump to the object, but list it at the top of the results. This would also make natural to simply enter a number, and see the links to the corresponding ticket, changeset and report having that number (if they exist)

The problem is that some people used the existing behavior on purpose, in order to create new Wiki pages quickly… But that feature was never advertised, and it's not a good Wiki practice anyway, so maybe it's better to change/fix that.

comment:6 by Christopher Lenz, 19 years ago

Component: generalsearch system

comment:7 by Christian Boos, 18 years ago

Milestone: 0.10
Owner: changed from Jonas Borgström to Christian Boos
Severity: normalminor
Status: newassigned

So what about doing the quick jump only if the page exist?

  • trac/Search.py

     
    2222from trac.util import TracError, escape, format_datetime, Markup
    2323from trac.web import IRequestHandler
    2424from trac.web.chrome import add_link, add_stylesheet, INavigationContributor
    25 from trac.wiki import IWikiSyntaxProvider
     25from trac.wiki import WikiSystem, IWikiSyntaxProvider
    2626
    2727
    2828class ISearchSource(Interface):
     
    253253        elif len(kwd) > 1 and kwd[0].isupper() and kwd[1].islower():
    254254            r = "((^|(?<=[^A-Za-z]))[!]?[A-Z][a-z/]+(?:[A-Z][a-z/]+)+)"
    255255            if re.match (r, kwd):
    256                 return self.env.href.wiki(kwd)
     256                if WikiSystem(self.env).has_page(kwd):
     257                    return self.env.href.wiki(kwd)
    257258
    258259    # IWikiSyntaxProvider methods

comment:8 by Christian Boos, 18 years ago

On top of r3350, doing a quickjump only if the page (or any other object) exists would be implemented like this:

  • trac/Search.py

     
    218218            return req.href.browser(kwd)
    219219        link = wiki_to_link(kwd, self.env, req)
    220220        if isinstance(link, Element):
    221             return link.attr['href']
     221            if 'missing' not in link.attr.get('class_', ''):
     222                return link.attr['href']
    222223
    223224    # IWikiSyntaxProvider methods

This will ignore redirections to objects that are known to be missing.

Also, it was suggested somewhere that we could disable the quickjump entirely in the search page, only have it when the search is triggered from the small form present on every page… opinions?

comment:9 by Matthew Good, 18 years ago

-1 on the last patch. Basing application logic on presentation information such as the CSS class is quite fragile.

comment:10 by Christian Boos, 18 years ago

I see this as an incremental approach: before r3350, you'd have to parse the resulting HTML string to retrieve that information.

In the future, the link variable above will not be an Element, but rather some kind of WOM element (as in Wiki Object Model), in which this missing information could be an attribute.

e.g.

    link = WikiParser(self.env, req).parse(kwd)
    if isinstance(link, Link) and not link.missing:
        return link.href

comment:11 by Alec Thomas, 18 years ago

Seems there are two issues here:

  1. Disabling quick jumps (solved with the ! prefix)
  2. An extensible/consistent way of adding quick jumps

This ticket only deals with the first of these and is easily solvable:

  • trac/Search.py

     
    164164        if query:
    165165            page = int(req.args.get('page', '1'))
    166166            redir = self.quickjump(req, query)
    167             if redir:
     167            if query.startswith('!'):
     168                query = query[1:]
     169            elif redir:
    168170                req.redirect(redir)
    169             elif query.startswith('!'):
    170                 query = query[1:]
    171171            terms = search_terms(query)
    172172            # Refuse queries that obviously would result in a huge result set
    173173            if len(terms) == 1 and len(terms[0]) < 3:

Perhaps we should close this for 0.10 and move discussion of 2 to another ticket?

Personally I'm -1 on adding a conditional check for existence because of the extra magic. For example, I'd prefer r9999 to tell me the changeset doesn't exist rather than perform a search.

by Alec Thomas, 18 years ago

Attachment: quickjump-escape.diff added

Quickjump escape

comment:12 by Alec Thomas, 18 years ago

Ignore that last comment, patch attached.

comment:13 by Christian Boos, 18 years ago

alect: is your patch supposed to fix a problem, or is it just an optimization? Anyway, that doesn't help with the issue of someone entering ClassName and ending up with a Create ClassName wiki page.

comment:14 by Christian Boos, 18 years ago

Forgot that your point 2. (An extensible/consistent way of adding quick jumps) was addressed, I believe. See r3350.

comment:15 by Alec Thomas, 18 years ago

It depends on your definition of fix I guess…the default behaviour of coming up with the create page can be circumvented by prefixing the page name with a !. eg. !ClassName, but the default is still the create page.

I personally like this behaviour, but maybe it could be extended with a configuration option trac.quick_jumps = false which would disable quick jumps entirely?

comment:16 by Christian Boos, 18 years ago

What I meant is that without your patch, Trac already behaves like this: the ! will negate the wiki syntax, and no link will be created, therefore redir will be None… The optimization you do is to simply by-pass the Wiki formatting when there's a leading "!". Why not, I'm OK with that.

But again, I think that this ticket is not about using the "!", rather with the newcomer's surprise when searching for something and ending up with a create wiki page, as david.tulloh illustrated it above (hm… no comment: links on p.e.c yet :( ).

Quick jumps are pretty useful, so I don't see the point of disabling that feature entirely just to workaround the above problem, which is only a minor inconvenience.

What has been proposed so far:

  1. Doing a search if the target of the quickjump is known as missing (option which doesn't seem to be popular… ok)
  2. Disabling the quickjump on the search page, but keeping it when the search is triggered from the top search box

In addition, I'd like to propose a variation on 2:

  1. On the search page, retrieve the link given by the quickjump, but don't redirect to it, rather present a link to it before the search results, which will be gathered regardless of the redir value. The quickjump toplevel search box behaves as usual.

What do you think?

comment:17 by Alec Thomas, 18 years ago

(Yes, quite true)

I like option 3, seems like the best of both worlds to me :)

by Alec Thomas, 18 years ago

Attachment: quickjump.diff added

Quickjump only when using top search bar

by Alec Thomas, 18 years ago

Attachment: quickjump+missing.diff added

As above plus add missing class to milestone and reports

comment:18 by Alec Thomas, 18 years ago

Added two attachments. The first simply implements option 3. I like it a lot.

The second is that, plus add the missing class to the links for milestone and report links. Should probably do the same for source links?

comment:19 by Alec Thomas, 18 years ago

Also (and I know this is not supported by IE6) perhaps we should use the following rather than programatically adding the "?" to missing links? Opinions?

a.missing:after { content: "?" }

comment:20 by Christian Boos, 18 years ago

Should probably do the same for source links?

Careful with that… see #2891

I'll test the patches asap

by Christian Boos, 18 years ago

Same as above, but the style is different: the quickjump results are more integrated with the other results, but still stand out on top of them.

comment:21 by Christian Boos, 18 years ago

Keywords: review added

I improved a bit the solution proposed by athomas. See attachment:quickjump+missing-r3431.diff.

Also, I'm +1 for the a.missing:after idea, since this would make it possible to print pages without that ? sign appended. It would also make it possible to avoid ? completely simply by making CSS customizations.

comment:22 by Christian Boos, 18 years ago

Note: need to change slightly the implementation, in order to support InterTrac's usage of quickjump. This can be done by having a hidden parameter noquickjump on the Search page, instead of having a quickjump hidden parameter in the header's quick search form.

comment:23 by Christian Boos, 18 years ago

Resolution: fixed
Severity: minornormal
Status: assignedclosed

Latest patch applied in r3469, with the above notice taken into account.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.