Closed Bug 1109477 Opened 10 years ago Closed 7 years ago

Search in Add-ons manager should be context sensitive

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Version:
36 Branch
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: phlsa, Unassigned, Mentored)

References

Details

(Whiteboard: [qx:link:spec] [lang=js])

Attachments

(1 file)

bug1109477_searchAddonsFix.diff
1.20 KB, patch
Details | Diff | Splinter Review 
Searching in the add-ons manager can show either matches for installed add-ons and matches for add-ons on AMO. Currently, the page remembers the search context, which can lead to some unexpected results (e.g. see bug 1095905).

Instead, we should automatically search on AMO when the user initiates a search from the »Get Add-ons« section and search installed add-ons when starting the search from any other section.
Component: General → Add-ons Manager
Product: Firefox → Toolkit
If we start at this function:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1887
we can trace it down to the eventual call at:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#2284

Now, that function is getting a third parameter, which we're ignoring, but if we call it aState, then we can check the value of aState.previousView and set the value of this._filter.value appropriately.

The one final piece we need is that the "Get Add-ons" section has a previousView value of "addons://discover/".
Mentor: bwinton
Whiteboard: [qx] → [qx] [lang=js] [good-first-bug]
Okay so we want to return different results depending on which search bar the user uses. If the user searches for addons under 'Get Add-ons' section, we want the results to be from AMO(which I am assuming is some type of marketplace). Any other search on Add-ons Manager will return results of already installed Add-ons. I don't see any other section the search bar is, other than the one that has 'Search all add-ons' filled already.
We want to set this._filter.value depending on which (side-)tab the user is on when they search from the search bar at the top of the page.  Uh, lemme whip up some images…

So, if I'm on this "Get Add-ons" tab, and I search for "bugzilla":
  https://www.dropbox.com/s/um3fhdfaxs39r0c/Screenshot%202015-02-09%2022.02.56.png?dl=0
I should end up here, on the "available add-ons":
  https://www.dropbox.com/s/3ubwlv3j8asyp5u/Screenshot%202015-02-09%2022.03.23.png?dl=0

But, if I'm on this other "Extensions" tab, and I search for "bugzilla":
  https://www.dropbox.com/s/jaqg5ic8m7er8o3/Screenshot%202015-02-09%2022.04.21.png?dl=0
I should end up here, on the "my add-ons", instead:
  https://www.dropbox.com/s/w8dbgl9lh5xpugs/Screenshot%202015-02-09%2022.04.42.png?dl=0

Does that make sense?
Okay, I understand that part. My Add-ons Manager looks different compared to yours though.
https://www.dropbox.com/s/uafg8fabkx5hcg2/addons.png?dl=0

Maybe my local version is not the same as the main one?
I think they change the contents of that page based on something.  I wouldn't worry about it too much, since you have the search box and the tabs on the side.  ;)

I did notice something else we may want to change, though.  In the search box, where it says "Search all add-ons", we might want to make that say "Search my add-ons" or "Search add-ons" if we're not in the "Get Add-ons" tab.  But let's wait on that until we fix this first bug…
Assignee: nobody → jtungul53
Status: NEW → ASSIGNED
You refer to a function in the first link, but when I click on it, it highlights: 
var query = aEvent.target.value; which is not a function, 

unless you mean the line above:
function search_onCommand(aEvent)

but you 
said that the function is getting a third parameter, and this one is only receives one. Sorry, my Javascript isn't too good, and I am probably just not understanding the code correctly.
I could have been clearer there.

It's the function at the second link (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#2284 ) that's getting the third parameter.  (It only declares two, but if you add a third one in there, it'll magically have a value.  :)
I'm thinking we can put an if statement in the beginning and if aState.previousView =="addons://discover/", assign this.filter.value to...? Otherwise assign it to...

How can I figure out what the value of this.filter.value is when selecting different side menu items?
Nevermind. Just figured out how to use the debugger! The values are 'local' and 'remote', so we can just assign this.filter.value to either local or remote, depending on if aState.previousView == "addons://discover/"

i'll submit the diff once this build is done.
This patch makes the add-ons search results default to available add-ons when searched under "Get Add-ons". If the user is under any other tab and uses the add-ons search, the results will return installed add-ons.
Attachment #8562511 - Flags: review?(dtownsend)
Hello Blake! Let's keep this great partnership going! https://bugzilla.mozilla.org/show_bug.cgi?id=1123657 looks a bit too confusing for me(as I have no idea what it's asking for). Have any other suggestions? I'll have some free time while I'm looking for a job.
Flags: needinfo?(bwinton)
Sounds like a plan!  How about bug 995758?  There's most of a solution there, but it needs some investigation to see if the proposed solution will work…

(Or if any of the other green or white bugs from https://bwinton.github.io/d3Experiments/qx.html grabbed your fancy, I'ld be happy to help you drive them forward. :)
Flags: needinfo?(bwinton)
Comment on attachment 8562511 [details] [diff] [review]
bug1109477_searchAddonsFix.diff

Review of attachment 8562511 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start but there are a few more things we need to do here. Firstly there is a bug in the behaviour, if you go to "Get Add-ons" and perform a search you correctly see a list of results from AMO. If you then enter a new search term in the box it switches you to searching local add-ons. You'll need to avoid change the filter if the search view is already displayed.

Secondly this is causing the automated test browser_searching.js to fail. Looking through it it doesn't seem to have anything specifically checking that the filter value persists but it's likely that some of the tests in there make that assumption so you'll need to correct those cases. I'd also like you to add two specific tests that verify that searching from a local list and the "Get Add-ons" section set the filter appropriately and return the right results. This page has some details on browser-chrome testing: https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2286,5 @@
> +	if(aState.previousView == "addons://discover/")
> +	  this._filter.value = "remote";
> +    else
> +	  this._filter.value = "local";
> +	

Nit: you have some bad indentation going on here, make sure your editor is set to use spaces to indent.
Attachment #8562511 - Flags: review?(dtownsend)
Alright before we move on to other bugs, we need to fix this one! I managed to figure out the fix to the bug, but I need help making the two test cases that are being requested. I read over the link provided but I am still unsure about how to proceed. Where and what do I put in that test function?
Makes sense.  The first step is to reproduce the test failure on your computer.  Have you gotten that working?  (Uh, by which I mean failing. ;)

For the new tests, I think the idea is to call 'gCategoryUtilities.openType', passing in "extension" and "discover" (and probably "search", for completeness) like in https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_searching.js?from=browser_searching.js#529 and then do some searches like in https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_searching.js?from=browser_searching.js#540 and make sure this._filter has the value we expect it to have in each case.
Yes, I've checked what Dave mentioned, and it does in fact do that. I've already changed the code to where it doesn't do that though. Do I change it back to where it produces the error, and then make the tests? Or do I want to run the tests with the changes so they pass? I'm guessing the second.
So, in theory, I like to not make the changes, write the tests, make sure they fail, then make the changes and confirm that they make the tests pass.  But since you've already got the changes made, if you wanted to just run the new tests with the changes, I'm sure that would be fine.  :)
Okay so with my changes, some tests fail when running the tests in 
C:\mozilla-source\mozilla-central\toolkit\mozapps\extensions\test\browser

when I revert the changes, it passes. Maybe I am going about this in the wrong way. I changed my original patch to:

   if(aState.previousView == "addons://discover/")
	  this._filter.value = "remote";
    else if(aState.previousView != "addons://search/bugzilla")
	  this._filter.value = "local";

which fixes the behavior bug, but probably introduces other problems. Any ideas or suggestions on how to fix this?
Hey John, I'm so sorry I didn't get back to you sooner!

I think we should just have two cases in the first if statement, like:
if (aState.previousView == "addons://discover/" || aState.previousView == "addons://search/bugzilla")

How does that sound to you?

And can you post the changes you're going to make to the tests?

Thanks!
It is alright. I was out for the weekend so I would not have time to work on it. 

As for your first suggestion, I don't think that will work because it does not take care of the cases where we start a search after clicking the other tabs. When we start a search from "Extensions" or "Appearance" for example, we want to return local results, so we would need an else if somewhere in there.

I did not make the changes to the tests. I just ran the command: 
./mach mochitest-browser toolkit/mozapps/extentsions/test/

which did not successfully complete.
I think we need an "else", but it should handle all the other cases, so it shouldn't be an "else if", right?
I think you are right. There is a problem with the code that is not doing what I intended it to. The line:

aState.previousView == "addons://search/bugzilla"

only returns true if the user is within Search, and searches "bugzilla". So how do we make it so that it returns true if the user is in Search, and searches anything?
Ah!  I see what you mean!  Perhaps https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith will be useful…  ;)

(As an unrelated side note, have you seen the Browser Toolbox?  https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox  It's pretty cool.)
So I tried to have the combined if and 1 else like you suggested, but I think it does need to be else if. The reason being is that we have 3 scenarios. 
If we search from "Get Add-ons", we need to return remote results,
If we search from anywhere else, we want to return local results.
If we search from "Search", we want to return results from which ever was previously selected.(As in don't assign aState.previousView).

I reread Dave's comment and I think I misunderstood it the first time. I think the current tests are testing with the old behavior and I am supposed to go in the browser_searching.js file and fix these tests, and create 2 more tests. I've got the browser to do what the fix is asking for though.
Okay, so an if for "Get Add-ons", an else if for "Search", and an else for "anywhere else"?
//This if statement will take care of searching from the "Get Add-ons" tab, which we always want to return remote results.
if(aState.previousView == "addons://discover/")
  this._filter.value = "remote";

//This else if statement takes care of searching anywhere else EXCEPT "Search" tab. Which should always return local results
else if(!aState.previousView.startsWith("addons://search/"))
  this._filter.value = "local";

Now we have a third scenario, searching from "Search" tab. When we are searching from "Search" tab, Dave says that we do not want to change the results. If "Available Add-ons" is already selected, we want to return remote results. So for this, we do nothing. this._filter.value already contains a value, so we don't need to assign it in this case.
Oh, perfect!  I look forward to seeing your upcoming patch.  :)
How would I go about fixing the test cases in browser_searching.js? I am not sure which ones are failing when running the mochitest-browser command.
Hmmm, I thought it told you which ones are failing…
Can you run it, and copy the output to http://pastebin.mozilla.org/ and I'll see if I can find the failing tests and show you where they are?
https://pastebin.mozilla.org/8823177

there's probably even more lines, but my command line buffer is not set high enough.
Right, so the ones that have failed are the ones marked "TEST-UNEXPECTED-FAIL".  If we look at the first one, we see the string "Name in detail view should be correct".  Plugging that into dxr gives us the line:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_searching.js#464
which then points us at the search function here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_searching.js#124

Looking at all the calls, the only one that passes a value for the parameter aCategoryType is at line:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_searching.js#426

So, I think it would be best to change the search function so that if you don't get a value for that parameter, then you ensure you're on the "Get Addons" (or Search) tab before searching.

That way, you can add another test that tries to search from a different page, and passes in the appropriate value to ensure you're on the local search page.

How does that sound to you?
How would we make it to ensure we are on the "Get Addons" or "Search" tab before searching?
Well, I think we could call open_manager like this:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_searching.js#67

(I haven't tested it or anything, but it seems like it might do what we need…)
Hey John, how are things going?  Are you hitting any problems I can help with?
I don't know how to continue with this bug. With your suggestions to ensure we are in the Search tab before searching, I changed the line from:

aCategoryType = aCategoryType ? aCategoryType : "search";

to:
 
 aCategoryType = aCategoryType ? aCategoryType : 
    open_manager("addons://list/extension", function(aWindow) {
    gManagerWindow = aWindow;
    gCategoryUtilities = new CategoryUtilities(gManagerWindow);
    run_next_test();
  });

But I don't know if that's right. If we figure out how to fix one of them, I think the rest should be easy.
Hmm.  That doesn't seem quite right.  Can you post your full diff to pastebin, and I'll poke around with it for a bit and see what I can find?  (Also, what is the command line you use to run the tests?  I always forget how to run them…  ;)
https://pastebin.mozilla.org/8824315

The command line I am using is the one supplied in mozilla-build/start-shell-msvc2013.bat
with command:

./mach mochitest-browser toolkit/mozapps/extensions/test/browser/browser_searching.js
Okay, so I tried the open_manager, and couldn't get it working, so I'm now thinking we need to use something like https://pastebin.mozilla.org/8824351 to make sure we're in the correct view before we start searching.  Did you want to give that a try, and see how it goes?
(Do you understand what I'm doing there?)
So you are adding the EventUtils.synthesizeMouseAtCenter() function to ensure we are in the get add-ons tab before running each test?
Yep!  (I'm not sure if we should be in the Get Add-ons tab, or in another tab, or if it should change on a test-by-test basis.  Feel free to change that to what it should be.  ;)
Okay, I tried to add that line to the first test that was failing, and now I am getting more errors. This new error is in browser_bug562797.js. The old error still persists, which was:

"11434 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/test-w
indow/browser_searching.js | Name in detail view should be correct - Got , expec
ted PASS - b"

http://pastebin.com/CTWgbB7m

So for each test, how do I know which tab the user should start in?
(In reply to John Tungul from comment #41)
> So for each test, how do I know which tab the user should start in?

Mostly by inspection, I think…

Mossop, could you maybe step in and give us a hand here?  Thanks!
Flags: needinfo?(dtownsend)
(In reply to Blake Winton (:bwinton) from comment #38)
> Okay, so I tried the open_manager, and couldn't get it working, so I'm now
> thinking we need to use something like https://pastebin.mozilla.org/8824351
> to make sure we're in the correct view before we start searching.  Did you
> want to give that a try, and see how it goes?
> (Do you understand what I'm doing there?)

That pastebin has expired so I can't see what you were trying here.
browser_searching.js opens the manager once at the start and closes it at the end so you shouldn't need to use open_manager again. If you need to switch view use:

gCategoryUtils.openType(type, () => {
  // ...
});

(In reply to Blake Winton (:bwinton) from comment #42)
> (In reply to John Tungul from comment #41)
> > So for each test, how do I know which tab the user should start in?
> 
> Mostly by inspection, I think…
> 
> Mossop, could you maybe step in and give us a hand here?  Thanks!

Yeah, currently it doesn't matter which view the manager is showing for each part of the test so it doesn't bother to define it.

Here's where it looks like these tests are starting from inspection. The numbers are the line numbers of the different tests:

420: Starts in some random view
432: Starts in some random view
479: Expects to be in the search view
522: Expects to be in the search view
530: Switches to the extensions view and checks the search view is hidden
540: Starts in the extensions view
548: Starts in the search view
556: Starts in the search view
586: Switches to the extensions view before doing anything
603: Starts in the search view
619, 641, 686 and 691 all restart the manager before doing anything, I'm not sure what view that puts us in, I think extensions but you should check that.

All of the tests that use check_filtered_results manually flip the filter to choose the results so you shouldn't have to worry about this.

Is that what you need?
Flags: needinfo?(dtownsend)
I think that helps.  John?
ok, lets focus on the first failed test, which is test on 432:

name in detail view should be correct - Got , expected PASS - b"

name gets initialized on line 464:
var name = gManagerWindow.document.getElementById("detail-name").textContent;

Why is this failing? It has something to do with me changing the results being displayed. This may take a lot longer than expected if we have to do this for every test case. I'll do what I can though!
Hey John,

I just found out that if you use the command line:
./mach mochitest-browser toolkit/mozapps/extensions/test/browser/browser_searching.js --jsdebugger
then it will launch a javascript debugger before running the tests, and you can add the line:
  debugger;
(or better:
  if (name != item.mAddon.name)
    debugger;
;) in the test file, and the debugger will automatically break at that line!

So that should let you debug what's failing, I hope!  (I tried it, and seemed to run into other errors, but perhaps it will work better for you.  ;)
(Whoops, it didn't work because I forgot to build your patch.  Now it's totally working for me, and I'm hitting the breakpoint, which should mean that you're ready to start debugging!  ;)
After adding the --jsdebugger in command line, i get error:

FATAL ERROR: Non-local network connections are disabled and a connection attempt
 to www.mozilla.org (63.245.217.105) was made.
You should only access hostnames available via the test networking proxy (if run
ning mochitests) or from a test-specific httpd.js server (if running xpcshell te
sts). Browser services should be disabled or redirected to a local server.

It runs into fatal error before running the tests. Any idea what this is?
Wow, no, I haven't seen that before…  So you're using the exact command line I pasted above, and then when the browser windows pop up, you're clicking the "Run toolkit/mozapps/extensions/test/browser tests" button?
yep, i even copied and pasted your command. It only shows that error when the --jsdebugger command is added too, so I am not sure why that would trigger a network error.
Weird!  Does it look like the browser is trying to load up a web page?  https://bugzilla.mozilla.org/show_bug.cgi?id=1056769 seems familiar, so perhaps you could search in about:config of the browser for "www.mozilla.org".

Also, if you can just ignore the error, and continue, that might be another path forward.
John, does your machine use a proxy in its system configuration?
Whiteboard: [qx] [lang=js] [good-first-bug] → [qx:link:spec] [lang=js] [good-first-bug]
Whiteboard: [qx:link:spec] [lang=js] [good-first-bug] → [qx:link:spec] [lang=js] [good first bug]
Assignee: jtungul53 → nobody
Status: ASSIGNED → NEW
Whiteboard: [qx:link:spec] [lang=js] [good first bug] → [qx:link:spec] [lang=js]
We are planning on replacing the whole search page with AMO in bug 1263313. Whilst I don't want to discourage a patch, it might just make sense to focus on the new stuff. Since there hasn't been movement on this for 14+ months, things might have stalled.
Bug 1263313 makes this irrelevant.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: