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)
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); }
Reporter | ||
Comment 1•22 years ago
|
||
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.
Comment 2•22 years ago
|
||
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).
Updated•22 years ago
|
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
Comment 3•22 years ago
|
||
*** 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)
Comment 5•22 years ago
|
||
*** Bug 164779 has been marked as a duplicate of this bug. ***
Comment 6•22 years ago
|
||
*** Bug 158889 has been marked as a duplicate of this bug. ***
Comment 7•22 years ago
|
||
*** Bug 165140 has been marked as a duplicate of this bug. ***
Comment 8•22 years ago
|
||
*** Bug 167417 has been marked as a duplicate of this bug. ***
Comment 9•22 years ago
|
||
caillon, could you review the patch?
Comment 10•22 years ago
|
||
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 11•22 years ago
|
||
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+
Comment 12•22 years ago
|
||
*** Bug 149931 has been marked as a duplicate of this bug. ***
Comment 13•22 years ago
|
||
*** Bug 165495 has been marked as a duplicate of this bug. ***
Comment 14•22 years ago
|
||
*** Bug 168598 has been marked as a duplicate of this bug. ***
Comment 15•22 years ago
|
||
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
Updated•22 years ago
|
Comment 16•21 years ago
|
||
*** Bug 208148 has been marked as a duplicate of this bug. ***
Comment 17•19 years ago
|
||
(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
Updated•19 years ago
|
Attachment #175009 -
Flags: review?(caillon)
Comment 18•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #175009 -
Attachment is obsolete: true
Attachment #175009 -
Flags: review?(caillon)
Updated•19 years ago
|
Assignee: rob_strong → search
Status: ASSIGNED → NEW
QA Contact: claudius
Updated•16 years ago
|
Product: Core → SeaMonkey
Comment 19•15 years ago
|
||
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
Comment 20•14 years ago
|
||
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.
Description
•