Closed Bug 149631 Opened 22 years ago Closed 14 years ago

previous/next buttons in sidebar search don't work as expected

Categories

(SeaMonkey :: Search, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED EXPIRED
Future

People

(Reporter: mozilla, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 obsolete files)

Using Mozilla RC3 on Windows 2000 and Windows 98 I have found this issue.

I attempted to find items related to this in Bugzilla and couldn't
find any open issues and wasn't too sure how to go about adding this -
was it a bug? an enhancement? what?  So I guessed and submitted it as normal.

This enhancement, as I will refer to it, to the search-panel.js file
(contained in path \content\communicator\search of comm.jar file)
addresses this issue as originally commented in the existing version
of the released file (in RC3), found in the showMoreResults function

  // XXX check if we are in basic search mode

Obviously someone was going to work on this at some point but it
wasn't a major problem and was probably very LOW on the priority list.
It was something that I wanted to use so I coded it myself. 
Modifications are pretty straight forward I think and I tried to make
it very clear in the comments where the modifications are.

The main issue was that the Next/Previous results buttons on the
search tab would only select the URL for the search engine selected in
the BASIC search engine list regardless of whether or not you were in
advanced mode.  Precautions also needed to be taken if multiple search
engines were selected in the current category (doesn't work).

For example is you have Bugzilla selected as your search engine in basic
and then switch to advanced search mode and perform a search on "mozilla"
you will get a list of google items in the search results list.  The list will
overrun the currently visible items so the "Next" button will be enabled. 
However when you click on the Next button it delivers you to Bugzilla and
performs the next search - because Bugzilla was the selected item in basic mode.

Included below is the edited version of the showMoreResults function
that works with both BASIC and ADVANCED search modes.  Please feel free to 
contact me with any questions.

-Robert
mozilla@bovilexics.com

------------------------------
 code patch included below
------------------------------

/**
 * showMoreResults
 *
 * Run a query to show the next/previous page of search results for the
 * current search term.
 * 
 * @param direction : -1 => previous
 *                     1 => next
 */
function showMoreResults(direction)
{
  // XXX check if we are in basic search mode
  // 
  // Uncomment the following 3 lines to return original functionality.
  //
  // get search engine
  // var engine = document.getElementById("basicEngineMenu").selectedItem;
  // var engineURI = engine.id;

  //
  // BEGIN RF MODIFICATIONS
  //
  // Basically modified code from doSearch function.
  // Some code from both functions could be refactored as a
  // separate function to remove some of the duplicate code.

  var searchMode = nsPreferences.getIntPref("browser.search.mode", 0);

  var engineURI;
  var engineURIs = [];
  var foundEngine = false;

  if (searchMode > 0) {
    var itemNode;
    var engineBox = document.getElementById("searchengines");

    // in advanced search mode, get selected search engines
    // (for the current search category)
    for (var x = 0; x < engineBox.childNodes.length; ++x) {
      itemNode = engineBox.childNodes[x];

      if (itemNode.localName == "listitem" && itemNode.checked) {
        engineURI = itemNode.id;

        if (engineURI) {
          engineURIs[engineURIs.length] = engineURI;
          foundEngine = true;
        }
      }
    }

    // if more than one search engine selected in current
    // search category then just return an alert message
    if (engineURIs.length > 1) {
      alert("Previous/Next buttons cannot operate when multiple search engines
are selected in the current search category.");
      return;
    }
  }
  else
  {
    var basicEngines = document.getElementById("basicEngineMenu");
    engineURI = basicEngines.selectedItem.id;

    if (engineURI) {
      engineURIs[0] = engineURI;
      foundEngine = true;
    }
  }

  if (!foundEngine) {
    alert(searchBundle.getString("enterstringandlocation"));
    return;
  }

  //
  // END RF MODIFICATIONS
  //

  // get search term
  var searchTerm = document.getElementById("sidebar-search-text").value;
  searchTerm = escape(searchTerm);

  // change page number
  if (direction > 0)
    ++gPageNumber;
  else
    --gPageNumber;

  // get qualified URL
  var searchService = Components.classes[ISEARCH_CONTRACTID].
                        getService(nsIInternetSearchService);
  var whichButtons = new Object;
  whichButtons.value = 0;

  var searchURL = searchService.GetInternetSearchURL(engineURI, searchTerm,
                    direction, gPageNumber, whichButtons);

  doNavButtonEnabling(whichButtons.value, searchService, gPageNumber);

  // load URL in navigator
  loadURLInContent(searchURL);
}
I included this code in my original bug comments.  I hadn't used the system
before and didn't know I could add it here.  Sorry if this caused any
confusion.
Marking NEW for patch loving.. Thanks for the patch..(though you might have to
start querying to make sure it gets in..also learn about cvs so you can generate
patch files for it).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: patch, review
Summary: previous/next buttons in sidebar search don't work as expected → [PATCH] previous/next buttons in sidebar search don't work as expected
Summary: [PATCH] previous/next buttons in sidebar search don't work as expected → previous/next buttons in sidebar search don't work as expected
Target Milestone: --- → Future
Blocks: 126089
*** Bug 149947 has been marked as a duplicate of this bug. ***
would this fix bug 57855? (see comment 9. Also bug 150102 and bug 164779)
*** Bug 164779 has been marked as a duplicate of this bug. ***
Blocks: 57855
*** Bug 158889 has been marked as a duplicate of this bug. ***
*** Bug 165140 has been marked as a duplicate of this bug. ***
*** Bug 167417 has been marked as a duplicate of this bug. ***
caillon, could you review the patch?
Attached patch Robert Fernandes' patch (obsolete) — Splinter Review
This patch does not mean I have reviewed it yet.  I just took the patch
provided from Robert, applied it, diffed it, removed the 'BEGIN MODIFICATIONS'
etc. comments, and replaced the alerts with debug() calls.  I will do an actual
code review of this shortly.
Attachment #86643 - Attachment is obsolete: true
Comment on attachment 98378 [details] [diff] [review]
Robert Fernandes' patch

Ok, so I had a look here.  I see some implementation issues:

1) In the case of clicking next with multiple search engines, why do we want to
even put up an alert that (effectively) says "Congratulations! You just clicked
on something that doesn't do anything"?  Would it not make more sense to
disable the prev/next buttons in that case?  Do we want to define a behavior
for that case (e.g. should those be enabled, and what should happen when they
are clicked)?  We should find an answer for that first before this patch goes
any further since this is basically one of the fundamental things behind this
patch.

2) That notwithstanding, I was a bit premature with the swapping of alert()s
into debug()s.	I see what you are trying to do there, but we should definitely
NOT be using window.alert() there: chrome should never call that.  Instead, use
nsIPromptService.

3) Somewhat minor, you are using way too many variables.  foundEngine will
always be false if engineURI is null; and it will always be true if engineURI
has a value.  The previous code relied on if (engineURI), and I see no reason
to change that.  Likewise, engineURIs is only used to see if we have multiple
URIs so we can fail.  That is unneeded.  If we do want to fail, all you really
need to check is when you want to add another URI, just check engineURI.  If we
have one, then we know we now have multiple and we can fail without the
variable.  But I suspect it may be better to just simply continue and not fail.

Other really minor nits: Use i for loop variables.  You _should_ use a variable
to store the value of engineBox.childNodes.length in your loop to avoid the
overhead of fetching it every time.  And handle the common case (basic mode)
first, then the less common case (advanced).
Attachment #98378 - Flags: needs-work+
*** Bug 149931 has been marked as a duplicate of this bug. ***
*** Bug 165495 has been marked as a duplicate of this bug. ***
*** Bug 168598 has been marked as a duplicate of this bug. ***
Richy,

The first two bugs (bug 149931 and bug 165495) you just marked as duplicates of this bug are duplicates of each other, but not of this bug. I'd suggest a more thorough reading, including taking a look at the proposed fixes.

Bug 149931 contains a fix (it just needs to be turned into a patch by someone that knows how), as well as suggestions for preventing this bug from re-appearing in future sherlock plugins.

The issue in those bugs is _solely_ what search-engine parameters are defined in the sherlock plugin and how they interact with the settings (usually the defaults) of the search-engine.

I've re-set my own bug as unresolved, but am unable to do so for the other one.

No idea about the third bug, though.

Sam
Depends on: 172122
Blocks: 172122
No longer depends on: 172122
Blocks: 146194
*** Bug 208148 has been marked as a duplicate of this bug. ***
Attached patch patch (obsolete) — Splinter Review
(In reply to comment #11)
1. When in advanced mode and one or more search engines support both Previous
and Next the Next button will be available with only the search engine that
supports the inputprev and inputnext sherlock elements showing the next page
results.
 
2. I changed the pre-existing alerts in this file to use the nsIPromptService.
As a side note, I don't believe that all of the existing alerts are necessary
and new ones should not be needed as well as described below.
 
The search text is now kept in gEncodedSearchTerm for use by the previous /
next code so an additional alert for non-existing search terms is not needed
since it is handled by the first check. Also, I don't believe the first check
is necessary since the button is disabled if there isn't a search term. This
also fixes the case where someone modifies the search text and clicks next or
previous in that it won't try to provide the results of page=x with a new
search term.
 
When changing engines in advanced mode the panel already collapses and thereby
hides the next / previous buttons so an alert should not be necessary.
 
When changing the engine in basic mode the previous . next buttons are already
disabled on change so an alert should not be necessary.
 
3. I believe I have addressed the comments but I have added a couple of global
vars to hold state.
 
I believe the code can / should be cleaned up a bit further but didn't do much
in order to keep this patch focused on the actual bug. For example, gURL does
not appear to be used anywhere... If you would like me to clean that up or any
of the other code and include it with this patch please let me know.

Thanks,
Robert
Assignee: samir_bugzilla → moz_bugzilla
Attachment #98378 - Attachment is obsolete: true
Status: NEW → ASSIGNED
I forgot to add that there is a search param that needs to be checked in
nsInternetSearchService.cpp for the advanced search next / previous to function
that I have in my tree. This additional change will not require any
modifications to the the code in this patch to implement so a review of the
current patch would be appreciated. so I know if the approach in search-panel.js
is acceptable. There is also another patch in bug 150337 awaiting review that
changes nsInternetSearchService.cpp. If it would be easier to review the patches
I have for search all at once I can create one patch that addresses several
bugs... either way is fine by me. Thank you for your time.
Attachment #175009 - Attachment is obsolete: true
Attachment #175009 - Flags: review?(caillon)
Assignee: rob_strong → search
Status: ASSIGNED → NEW
QA Contact: claudius
Product: Core → SeaMonkey
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
Status: NEW → UNCONFIRMED
MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but still has no comment since the inception of the SeaMonkey project 5 years ago.

Because of this, we're resolving the bug as EXPIRED.

If you still can reproduce the bug on SeaMonkey 2 or otherwise think it's still valid, please REOPEN it and if it is a platform or toolkit issue, move it to the according component.

Query tag for this change: EXPIRED-20100420
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → EXPIRED
You need to log in before you can comment on or make changes to this bug.