1299535 - Add the ability to add custom search engines via url from the settings

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Farhan Patel [:farhan]]

Attachment: Screenshot 2016-08-31 09.39.21.png

The current custom search engine adder in iOS is limited to fields with forms using GET requests. To support more search engines allow users to add search engines via url strings.

I've added mocks of where this design should exist in the settings.

Comment 1

[Farhan Patel [:farhan]]

Attachment: Screenshot 2016-08-31 09.39.14.png

Comment 2

[Nikolay]

Hi, can I take this one?

Comment 3

[Farhan Patel [:farhan]]

Hi Nikolay, 

I'd love for you to work on this bug. This is my most wanted feature. I wish I had the time to work on it but sadly I'm too busy with other stuff. I'd love if you'd take it on and finally get this done!

To assign a bug to yourself, you need to click the "take" link next to the "Assigned to" section. And then press save. This lets everyone else know that you are working on this feature and avoids cases of two people working on the same thing. 

As for where to start, The screenshots should be helpful and should give you a good starting place. The current search settings code is here https://github.com/mozilla/firefox-ios/blob/f8b7729d0446214dfae32a67fd0615cf200336a4/Client/Frontend/Settings/SearchSettingsTableViewController.swift

If you need any help feel free to reach out to me on IRC. I'm in the #mobile channel -> @farhan

Comment 4

[Pushkar]

Hey Farhan, 
Looks like no one has been working on this issue yet. I would love to take it up, but for some reason I see no `take` link anywhere near `Assigned To: Nobody; OK to take it and work on it`.

Also to begin with adding the 'Add Custom Search Engine' row, should I add another option to model.orderedEngines itself and handle it in an else? 
Or should I add an `else if indexPath.item < model.orderedEngines.count - 1{` condition for the Quick-Search engines, with the 'Add Custom Search Engine' option handled in an `else` in the UITableViewDataSource methods in the SearchSettingsTableViewController?

I would really appreciate opinions on how I should move forward with this.

Regards,
Pushkar

Comment 5

[Farhan Patel [:farhan]]

Hi Pushkar!
I know the screenshot doesn't suggest this but I'd recommend you make a separate section with only the "Add custom engine button" 

Also add a title to the section similar to the 2 existing sections. Something like "Custom Search engines"
Then in that section you'll have one item called "Add Custom Search engine"

In the cellforRowAtIndexPath you'd do something like 
else if indexPath.section == CustomEngineSection
look at how the "SectionDefault" is done for an idea.

Also if you are up for it. I'd love if you could refactor the cellforRowAtIndexPath function. Having a large and growing if statement like that make for messy code haha. 

in that section you'd create your cell that links to the new form that lets you add a new search engine.
If you are more comfortable with having me follow along send me a link on IRC to your branch (@farhan) and you can ping me as you work on it for help :)  I'd recommend first writing the additions to SearchSettingsTableView before writing the new CustomSearchAddController. 

If you are up for it. Let me know and I can assign this bug to you.

Comment 6

[Pushkar]

Hey Farhan,
Of course I'm up for it.
And yep, a separate section makes sense. I'll see what can be done about refactoring it too.
I'll begin work on the SearchSettingsTableViewController in a while, and will send you the link to my branch once I do.

Comment 7

[bbell]

Attachment: Settings - [e-mail]

Here's the latest Mockup of the Search Settings Page.

Comment 8

[bbell]

Attachment: Settings - Search - Add.png

Latest mock for the add form.

Comment 9

[bbell]

Attachment: Settings - Search.png

Search Settings Mockup

Comment 10

[bbell]

Attachment: Measurements - Search - Prefs.png

here are some helpful measurements needed to implement the mock-ups

Comment 11

[Farhan Patel [:farhan]]

master https://github.com/mozilla-mobile/firefox-ios/commit/c4bccd6fc3e90decc7e6e900d6aca227740d5d4f

Thank you for all your work Pushkar! I can't wait to use this.

Comment 12

[Aaron Train [:aaronmt]]

I suspect bug 1334689 is a crash triggered with this recently added code.

Comment 13

[Catalin Suciu [:csuciu]]

This feature it's not behind a feature flag. Are you planning to ship it in 7.0? 

Please note that there are still a few bugs open, one of them being a crash which is fairly easy to reproduce.

Comment 14

[Stephan Leroux [:sleroux]]

I think having it ship in beta is fine for now but we should make it a priority to add a feature flag to put this behind so we can turn it off if we don't feel comfortable shipping it in release.