150333 - Next and Previous start value may need to be definable by search plugin.

Status: RESOLVED INCOMPLETE

(SeaMonkey :: Search, defect)

Description

[Matthew A. McGuigan]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020605
BuildID:    2002060511

The start value used by nsinternetsearchservice is 0 and needs to be 1 in order
for results to display and increment properly with excite.

In
 http://lxr.mozilla.org/seamonkey/source/xpfe/components/search/src/nsInternetSearchService.cpp

If I read the code correctly it is in this section of code on line 4622 that the
value in question is set to 0:


4621   // XXX get page
4622   PRInt32 errorCode, index = 0;
4623   PRInt32 factorInt = factor.ToInteger(&errorCode);
4624   
4625   if (NS_SUCCEEDED(errorCode))
4626   {
4627     // if factor is garbled assume 10
4628     if (factorInt <= 0)
4629       factorInt = 10;
4630 
4631     if (direction < 0)
4632     {
4633       // don't pass back a negative index!
4634       if (0 <= (page - 1))
4635         --page;
4636     }
4637     index = factorInt * page;
4638   }
4639 
4640   return index;
4641 }

If the value  for "index" could be defined in the plugin instead of the program
assuming 0 is the correct value, it should solve the problem.

Reproducible: Always
Steps to Reproduce:
1.Place this search plugin in your mozilla searchplugins directory. 

# Start File Excite.src
# This is a beta search plugin and excite has been making changes to 
# their search service every few days lately, so there is no gaurantee
# that this plugin will work at all in the near future.

<search
    name="Excite"
    action="http://msxml.excite.com/info.xcite/dog/webresults.htm"
    method="GET"
  >
<input name="qkw" user>
<input name="qcat" value="web">
<input name="qk" value="20">
<inputnext name="qi" factor="20">
<inputprev name="qi" factor="20">
<interpret
      browserResultType = "result"
	bannerStart="<table cellspacing=0 cellpadding=3 border=0 width=96%>"
	bannerEnd="</table>"

	resultListStart="</table><br>"
	resultListEnd="<br>	</font>"

	resultItemStart=".&nbsp;&nbsp;"
	resultItemEnd="&nbsp;<br>"
>
<interpret
      browserResultType = "result"
#
bannerStart="<!-- ----------Advertising.com Banner Code---------- -->"
#
bannerEnd="<!-- ----------Copyright 2000, Advertising.com---------- -->"

	resultListStart="<tr>	<td>"
	resultListEnd="</td>	</tr>	</table>"

	resultItemStart=".&nbsp;&nbsp;"
	resultItemEnd="&nbsp;</font><br>"
>
</search>
# end Excite.src

2.  Perform search from sidebar or address bar using plugin.

Actual Results:  The results start at and display as 0.-18.

Expected Results:  The results should start at and display as 1.-20.

Comment 1

[Matthew A. McGuigan]

Please note that the search results display correctly if the following lines are
removed from the plugin:

<inputnext name="qi" factor="20">
<inputprev name="qi" factor="20">

However, removing those lines disables the next and previous buttons in the sidebar.

Comment 2

[Samir Gehani]

helpwanted

Comment 3

[Sam Schinke]

ok, the following URL, however, displays perfectly, giving results 1-20:

http://msxml.excite.com/info.xcite/dog/results?otmpl=dog/webresults.htm&qkw=test&qcat=web&qi=1&qk=20

This appears to be a classic "off by one" error, common where zero and one-index systems coexist. Excite's search result pages are 1-indexed (in actual fact, they display "qk" results from result "qi", inclusive, and don't seem to handle "qi=0").

What I would suggest is adding an "offset" property that simply offsets the factor. It should probably be called something else, though.

The "getIndex" function would set the index to index = (factor * page) + offset, with offset defaulting to zero. This avoids changing the behavior of existing plugins.

It might also be worth checking what the mac folks do in these cases, as I'm sure the sherlock "standard" was designed to take things like this into account.

It solves this particular case quickly and easily, and also allows for other search-engines that aren't zero-indexed in their additional results display. More complex search-engines (perhaps using letters to index results pages? who knows) can be dealt with when they become visible.

I'd offer to code this addition (searchservice.cpp seems well enough designed and commented to make an addition painless), but I'm unfamiliar with mozilla programming conventions, as well as how to actually create a patch.

Matthew, the "index" you observe being set to zero is only being set to that in preparation for "index = factorInt * page;" further down the function. This essentially gives you "currentresultspage * factor" which is correct in most cases, though in this case, one needs to be added for every page.

Another slight "bother" that I've noticed is that if the values in inputnext and inputprev have the same name, the value is placed in the URL twice. Is this broken? If so, I'll post a new bug for it.

Sam

Comment 4

[Matthew A. McGuigan]

Thanks sam,

On the issue of the value being placed in the URL twice.  In testing, if the
"name" is the same for both one only needs to define inputnext.  Inputprev will
assume the the same name and factor.  From AltaVista.src for example:

<inputnext name="stq" factor="10">
<inputprev>

NOTE: The <inputprev> still needs to be there or the "Previous" button will be
disabled.

Using similar code in your Google plugin should remove the annoyance without any
adverse affects.

Unless Apple Sherlock handles this differently or we can find a test case that
fails we most likely will get a won't fix.  

Joolz,

Does Apple handle this the same way?  

If not and a bug doesn't already exist for this, we need to submit a bug.

Comment 5

[Erich 'Ricky' Iseli]

> if the values in inputnext and inputprev have the same name, the value
> is placed in the URL twice. Is this broken? If so, I'll post a new bug
> for it.

See the bug 151390 (Google-URL duplicates substring "start=0"), is this the same
issue?

On a sidenote, Sam, is there a way to break your long lines? it's very hard to
read your bug reports if we have to scroll horizontally. Thanks.

Comment 6

[Matthew A. McGuigan]

Added Sam to CC

Yes that is the issue.

http://bugzilla.mozilla.org/show_bug.cgi?id=151390

Adding post to that discussion.

Comment 7

[Sam Schinke]

Erich,

Sorry about the long lines.

I think I'll post it as a bug in bugzilla. It seems that when posting
via IE6.0 sp1 the lines do not wrap in the textarea. Rather, they do
wrap in the composition textarea, but the resulting bugzilla post
doesn't seem to match.

Posts seem to work fine from mozilla, though.

The other bug about the substrings is the same issue I mentioned, yes.

Sam

Comment 8

[julius ross]

As far as I can tell the previous/next functionality is a mozilla extension to
the sherlock specification.  For instance I can find nothing about it in
<http://developer.apple.com/technotes/tn/tn1141.html> and there is no similar
feature in the Sherlock application for MacOSX.

Julius

Comment 9

[Matthew A. McGuigan]

Note that buy.com plugin is also affected by this bug though less severe.

Details:  First request for next results will show the same results you where
looking at.  On second attempt it will show the next results without skipped
results.

FYI: please look at http://bugzilla.mozilla.org/show_bug.cgi?id=174661 before
attempting to use the buy.com plugin with a nightly build.

Comment 10

[Erich 'Ricky' Iseli]

*** Bug 207179 has been marked as a duplicate of this bug. ***

Comment 11

[Erich 'Ricky' Iseli]

*** Bug 202462 has been marked as a duplicate of this bug. ***

Comment 12

[Christopher Whitt]

Added top100 keyword - evangelism triage for Moz 1.7

Comment 13

[Robert Strong (they/them - no direct email)]

Attachment: patch

This patch adds allows for an optional adjust attribute for inputnext and
inputprev. I named the attribute adjust which I believe is consistent with the
attribute factor in that they both describe the operation to be performed. If
the attribute does not exist or doesn't successfully convert to an integer it
defaults to 0 so the behavior is consistent with the current behavior. I have
included the patch for bug 150337 in this patch and can remove it if this is
preferable. Thanks.

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: testcase for patch

The attached zip file has a search plugin I created for XUL Planet that uses
the adjust attribute. Just unzip and place the two files in your apps
searchplugins directory and with the patch applied it will adjust the URL param
for the next and previous results when using the Next and Previous buttons.

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 174575 [details] [diff] [review]
patch

I started to work on fixing advanced mode next / previous functionality so I am
clearing the reviews. I also noticed that with the removal of the page
decrement that the direction argument is no longer needed. Comments on this as
well as the attached patch in progress are appreciated as always.

Comment 16

[Robert Kaiser]

MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 18

[Philip Chee]

Now using openSearch backend. Closing as OBSOLETE