Rocketbar: e.me Search Suggestions

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: amirn, Assigned: amirn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Show Everything.me auto-complete suggestions in the Rocketbar
(Assignee)

Updated

5 years ago
Blocks: 948317
(Assignee)

Updated

5 years ago
No longer blocks: 948317
(Assignee)

Updated

5 years ago
Blocks: 948317
(Assignee)

Comment 1

5 years ago
Created attachment 8351186 [details] [review]
Redirect to PR

This patch includes a working version of Everything.me suggestions in the rocketbar
Attachment #8351186 - Flags: review?(ran)
Attachment #8351186 - Flags: review?(kgrandon)
(Assignee)

Comment 2

5 years ago
(In reply to Amir Nissim (Everything.me) from comment #1)
> Created attachment 8351186 [details] [review]
> Redirect to PR
> 
> This patch includes a working version of Everything.me suggestions in the
> rocketbar

A better approach is to create a shared Evme object owning the connection, and have the Evme search and suggestion providers use it.
When I tried approach everything stopped working.

Kevin - can you please have a look at this patch? it looks like some weird IAC voodoo :)
https://github.com/EverythingMe/gaia/commit/d9a7905c10bc6f7c44591e47eeee1d155c88a893
Flags: needinfo?(kgrandon)
(Assignee)

Updated

5 years ago
Assignee: nobody → amirn
(In reply to Amir Nissim (Everything.me) from comment #2)
> 
> A better approach is to create a shared Evme object owning the connection,
> and have the Evme search and suggestion providers use it.
> When I tried approach everything stopped working.

It should be possible, but hard to say what was wrong without seeing a patch. I think something like this is probably fine for now though. I'll take a look today.
Flags: needinfo?(kgrandon)
Comment on attachment 8351186 [details] [review]
Redirect to PR

Ok, looks mostly good to me. Before I give R+, had a few questions.

1 - Results appear to not be clearing for me currently. Sometimes there can be multiple levels of results. I think you need to clear them before you append more.

2 - Results are showing up for the same input currently. We should probably just not show results in this case.

3 - The search input is currently receiving the annotated query with brackets in the [q]uery. Should this be the case?

4 - Annotated examples will duplicate the exact search sometimes. E.g., if I search for 'britney spears', I will see a result, `[britney][spears]`. It seems like we should filter out exact matches, and that spaces should be considered in the annotated text.
Attachment #8351186 - Flags: review?(kgrandon)
(Assignee)

Comment 5

5 years ago
Thanks Kevin.

Just to make sure, did you see the patch at https://github.com/EverythingMe/gaia/commit/d9a7905c10bc6f7c44591e47eeee1d155c88a893 ?
I am planning to change the implementation which will hopefully resolve some of the issues.

(In reply to Kevin Grandon :kgrandon from comment #4)
> 1 - Results appear to not be clearing for me currently. Sometimes there can
> be multiple levels of results. I think you need to clear them before you
> append more.
Do you mean suggestions? it is cleared on every new search:
https://github.com/EverythingMe/gaia/blob/8e22c1dc08e948d2cd64476ef37919204f641aab/apps/search/js/providers/eme_suggestions.js#L28 

> 2 - Results are showing up for the same input currently. We should probably
> just not show results in this case.
I do not understand what do you mean by "same input". Can you please explain?
 
> 3 - The search input is currently receiving the annotated query with
> brackets in the [q]uery. Should this be the case?
The annotation are received from E.me api but are cleaned before sent out to the search app
https://github.com/EverythingMe/gaia/blob/8e22c1dc08e948d2cd64476ef37919204f641aab/apps/homescreen/everything.me/js/search/suggestion.js#L7
Where do you see the annotations?

> 4 - Annotated examples will duplicate the exact search sometimes. E.g., if I
> search for 'britney spears', I will see a result, `[britney][spears]`. It
> seems like we should filter out exact matches, and that spaces should be
> considered in the annotated text.
The API returns the searched string as the 2nd suggestions. This is by design, but we can filter it out if needed.
(In reply to Amir Nissim (Everything.me) from comment #5)
> Thanks Kevin.
> 
> Just to make sure, did you see the patch at
> https://github.com/EverythingMe/gaia/commit/
> d9a7905c10bc6f7c44591e47eeee1d155c88a893 ?
> I am planning to change the implementation which will hopefully resolve some
> of the issues.
> 
> (In reply to Kevin Grandon :kgrandon from comment #4)
> > 1 - Results appear to not be clearing for me currently. Sometimes there can
> > be multiple levels of results. I think you need to clear them before you
> > append more.
> Do you mean suggestions? it is cleared on every new search:
> https://github.com/EverythingMe/gaia/blob/
> 8e22c1dc08e948d2cd64476ef37919204f641aab/apps/search/js/providers/
> eme_suggestions.js#L28 

You need to clear it before appending, or abort in-progress requests. Currently if I type three letters, and a request takes longer than our search delay, we end up showing multiple rows of suggestions.

> > 2 - Results are showing up for the same input currently. We should probably
> > just not show results in this case.
> I do not understand what do you mean by "same input". Can you please explain?

If I type 'omfgwtfbbq', the suggestion row shows a single suggestion of the same text. Seems like we should not have a suggestion row in this case.

> > 3 - The search input is currently receiving the annotated query with
> > brackets in the [q]uery. Should this be the case?
> The annotation are received from E.me api but are cleaned before sent out to
> the search app
> https://github.com/EverythingMe/gaia/blob/
> 8e22c1dc08e948d2cd64476ef37919204f641aab/apps/homescreen/everything.me/js/
> search/suggestion.js#L7
> Where do you see the annotations?

My query is updated after I click on a result with annotations. By annotation, I mean brackets surrounding the match terms. It doesn't seem like we should put the brackets into the input field.

> > 4 - Annotated examples will duplicate the exact search sometimes. E.g., if I
> > search for 'britney spears', I will see a result, `[britney][spears]`. It
> > seems like we should filter out exact matches, and that spaces should be
> > considered in the annotated text.
> The API returns the searched string as the 2nd suggestions. This is by
> design, but we can filter it out if needed.

I see this first in some cases, and it seems wrong. We should probably filter this out.


Please let me know if any of these don't make sense, I could upload a screenshot where necessary.
(Assignee)

Updated

5 years ago
Blocks: 953058
(Assignee)

Comment 7

5 years ago
updated PR with refactored code that introduces a global eme object owning the connection.

RE [1]:
Canceling requests is a provider-wide issue so IMO the search app should handle it.

RE [3]:
where do you see the annotations? Can you provide a screenshot? also attaching a screenshot from my device.

RE issues [2] and [4]:
This is how E.me API works (return the searched query as a one of the suggestions).
IMO the eme client should not modify the API results. If we want to filter some suggestions out, I think a better place for that is in the search app.

Thanks.
(Assignee)

Comment 8

5 years ago
Created attachment 8351376 [details]
screenshot-no-annotations.png
Flags: needinfo?(kgrandon)
Comment on attachment 8351186 [details] [review]
Redirect to PR

Marking R+ for the search app part of things. It's not perfect, but we can make follow-up patches no problem. It's early enough that we can iterate quickly, and we should do so.
Attachment #8351186 - Flags: review+
Flags: needinfo?(kgrandon)
Comment on attachment 8351186 [details] [review]
Redirect to PR

Got a few comments on Github. Rock on.
Attachment #8351186 - Flags: review?(ran) → review+
(Assignee)

Comment 11

5 years ago
landed: https://github.com/mozilla-b2g/gaia/commit/db150e5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 953121
You need to log in before you can comment on or make changes to this bug.