Closed Bug 1255504 Opened 6 years ago Closed 4 years ago

Provide a way to change the default search engine

Categories

(Instantbird :: Preferences, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: aleth, Assigned: durwasa.chakraborty, Mentored)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 4 obsolete files)

It doesn't seem possible to change the default engine in Instantbird. You can only reorder the engines in the list.
Whiteboard: [good first bug]
Mentor: aleth
Since, the top most search engine is of the index 0 I used the method `engineStore` to retrieve the name of the engine at the top and then by using the Serives.jsm method I called the getEngineByName method.
Attachment #8738585 - Flags: review?(aleth)
Comment on attachment 8738585 [details] [diff] [review]
Bug 1255504 Search from <Default Engine>

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

::: im/content/engineManager.js
@@ -311,5 @@
>                          getService(Ci.nsIBrowserSearchService);
>      var currentEngine = this._cloneEngine(searchService.currentEngine);
>      for (var i = 0; i < this._ops.length; i++)
>        this._ops[i].commit();
> -

Don't remove unrelated blank lines.

@@ +317,5 @@
>      // Restore currentEngine if it is a default engine that is still visible.
>      // Needed if the user deletes currentEngine and then restores it.
>      if (this._defaultEngines.some(this._isSameEngine, currentEngine) &&
>          !currentEngine.originalEngine.hidden)
> +      searchService.currentEngine = Services.search.getEngineByName(gEngineView._engineStore.engines[0].name);

Did you read the comment? Why are you putting your currentEngine change inside this if clause?

Also I'm pretty sure that if you look at Services.jsm you will find there is no difference between Services.search and searchService.
Attachment #8738585 - Flags: review?(aleth) → review-
(In reply to aleth [:aleth] from comment #2)
 
> Don't remove unrelated blank lines.
Okay... I was not wary about the best practices. Made the correction. :)

> Did you read the comment? Why are you putting your currentEngine change
> inside this if clause?
The comment over the if clause seemed to be an edge case and I thought it would be unwise to write it outside the scope of if clause. Also, I am not able to comprehend the relation with the comment - 

> It doesn't seem possible to change the default engine in Instantbird.
> You can only reorder the engines in the list.

Please help.

> Also I'm pretty sure that if you look at Services.jsm you will find there is
> no difference between Services.search and searchService.
Yes, there is no difference but since searchService was within the scope,something like a local variable, I decided to assign the value there. So, the ideal solution would be assigning the search engine to the value Services.currentEngine ?
(In reply to Durwasa Chakraborty from comment #3)
> > Did you read the comment? Why are you putting your currentEngine change
> > inside this if clause?
> The comment over the if clause seemed to be an edge case and I thought it
> would be unwise to write it outside the scope of if clause.

Unwise why? Please be more specific about your concern.

> Also, I am not able to comprehend the relation with the comment - 

Maybe this reordering will help?

    // Needed if the user deletes currentEngine and then restores it.
    if (this._defaultEngines.some(this._isSameEngine, currentEngine) && // if it is a default engine
        !currentEngine.originalEngine.hidden)  // that is still visible.
      searchService.currentEngine = currentEngine.originalEngine; // Restore currentEngine

> > It doesn't seem possible to change the default engine in Instantbird.
> > You can only reorder the engines in the list.
> Please help.

Since your patch works for you, you have clearly made some progress ;) 

> > Also I'm pretty sure that if you look at Services.jsm you will find there is
> > no difference between Services.search and searchService.
> Yes, there is no difference but since searchService was within the
> scope,something like a local variable, I decided to assign the value there.

Objects in Javascript are held by reference, so it's a local variable pointing at the same object as Services.search, as it results from the same getService() call.
Assignee: nobody → durwasa.chakraborty
Status: NEW → ASSIGNED
Attached patch Attachment for Bug 1255504 (obsolete) — Splinter Review
As per the conversation with Florian Quèze  ( http://log.bezut.info/instantbird/160408/#m149 ) he said that its okay if the state gets back to the default. 

As compared to the previous patch I have made one more change.

I think this shouldn't break because of the following reasons :
 
Case 1 If the default engine does not pass the .some() method and the current engine is  hidden implies that the top most engine is not there and in that case we need to make the current available engine as the next engine. 
Case 2 If the default Engine does pass the .some() method and the current engine is not hidden that means that the we have not hit the 'remove' button and therefore the current Engine is the top most engine.
Case 3 If the default Engine does not pass the .some() method that means that the original engine does not exist in the clone of the original engine which implies that original engine does not exist. In either case the topmost engine will be assigned as the default one.
Attachment #8739488 - Flags: review?(aleth)
Comment on attachment 8739488 [details] [diff] [review]
Attachment for Bug 1255504

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

::: im/content/engineManager.js
@@ +313,5 @@
>                          getService(Ci.nsIBrowserSearchService);
>      var currentEngine = this._cloneEngine(searchService.currentEngine);
>      for (var i = 0; i < this._ops.length; i++)
>        this._ops[i].commit();
> +    searchService.currentEngine=Services.search.getEngineByName(gEngineView._engineStore.engines[0].name);

What if engines[0] does not exist?

Still using Services.search despite me explaining to you why you don't need it here.

Please note the 80 character line length limit in the coding style guidelines.

@@ +319,5 @@
>      // Restore currentEngine if it is a default engine that is still visible.
>      // Needed if the user deletes currentEngine and then restores it.
>      if (this._defaultEngines.some(this._isSameEngine, currentEngine) &&
>          !currentEngine.originalEngine.hidden)
> +      searchService.currentEngine=Services.search.getEngineByName(gEngineView._engineStore.engines[0].name);

This makes no sense. You have written

do_something;
if (condition)
  do_the_same_thing_again;
Attachment #8739488 - Flags: review?(aleth) → review-
(In reply to aleth [:aleth] from comment #6)
> Comment on attachment 8739488 [details] [diff] [review]
> Attachment for Bug 1255504

> What if engines[0] does not exist?
This condition will never occur as the remove button will remove all search engine until one is left in the dialog.

> Still using Services.search despite me explaining to you why you don't need
> it here.

I am terribly sorry it just skipped my mind to make the changes but believe me it was in the back of my head to use searchService and terminate the overhead of loading the Services.jsm

> Please note the 80 character line length limit in the coding style
> guidelines.

Okay :)  

> @@ +319,5 @@
> >      // Restore currentEngine if it is a default engine that is still visible.
> >      // Needed if the user deletes currentEngine and then restores it.
> >      if (this._defaultEngines.some(this._isSameEngine, currentEngine) &&
> >          !currentEngine.originalEngine.hidden)
> > +      searchService.currentEngine=Services.search.getEngineByName(gEngineView._engineStore.engines[0].name);
> 
> This makes no sense. You have written
> 
> do_something;
> if (condition)
>   do_the_same_thing_again;

I'll try to work it around this thing but I somehow don't feel the need of the if (condition). The conditions that have satisfied by the if clause have been taken care otherwise. But I will brainstorm again try to find out some scenarios where the build might fail and the resubmit the next patch.


This patch was very disappointing for me to say the least but next patch I'll try to make it immaculate.
(In reply to Durwasa Chakraborty from comment #7)
> I'll try to work it around this thing but I somehow don't feel the need of
> the if (condition). The conditions that have satisfied by the if clause have
> been taken care otherwise. But I will brainstorm again try to find out some
> scenarios where the build might fail and the resubmit the next patch.

If you don't need it any more, then remove it, don't leave it in but have it do nothing ;)
Attached patch Attachment for Bug 1255504 (obsolete) — Splinter Review
Revisiting the previous three cases ::
Case 1,2 and 3 from previous comment (https://bugzilla.mozilla.org/show_bug.cgi?id=1255504#c5) 
Since, there shall always be a search engine at the top of the manage search engine dialog (thanks to the removeEngine() function) we can use it to address three scenarios ::

* The first engine becomes the Default Engine
* The first engine is removed(hidden) and therefore the next engine now becomes the engine with index 0 in the tree, and so the next engine becomes default engine.
* If all the engines are removed then there shall be one engine that will be left,and it will become the default search engine.


**Tested for all the above cases
Attachment #8738585 - Attachment is obsolete: true
Attachment #8739488 - Attachment is obsolete: true
Attachment #8739902 - Flags: review?(aleth)
Comment on attachment 8739902 [details] [diff] [review]
Attachment for Bug 1255504

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

::: im/content/engineManager.js
@@ +313,5 @@
>      for (var i = 0; i < this._ops.length; i++)
>        this._ops[i].commit();
> +    searchService.currentEngine=searchService
> +                                  .getEngineByName
> +                                  (gEngineView._engineStore.engines[0].name);

How about 
let topEngine = gEngineView._engineStore.engines[0]; 
searchService.currentEngine = searchService.getEngineByName(topEngine.name);
Attachment #8739902 - Flags: review?(aleth) → review?(florian)
(In reply to aleth [:aleth] from comment #10)
> How about 
> let topEngine = gEngineView._engineStore.engines[0]; 
> searchService.currentEngine = searchService.getEngineByName(topEngine.name);
Okay :) 
I named it as apexEngine first, but yes this will ensure the 80 character line length limit plus the readability will also increase. 
Ps :: Made the changes :) :)
Attached patch Patch for Bug 1255504 (obsolete) — Splinter Review
Created a block scope local variable called 'topEngine'.

* Increased readability of the code.
* Complying to the 80 character line length limit.
Attachment #8739902 - Attachment is obsolete: true
Attachment #8739902 - Flags: review?(florian)
Attachment #8739923 - Flags: review?(aleth)
Comment on attachment 8739923 [details] [diff] [review]
Patch for Bug 1255504

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

::: im/content/engineManager.js
@@ +309,5 @@
>    commit: function ES_commit() {
>      var searchService = Cc["@mozilla.org/browser/search-service;1"].
>                          getService(Ci.nsIBrowserSearchService);
>      var currentEngine = this._cloneEngine(searchService.currentEngine);
> +    let topEngine = gEngineView._engineStore.engines[0];

Why put this above the commits?
Attachment #8739923 - Flags: review?(aleth) → review-
Made the corrections.
Attachment #8739923 - Attachment is obsolete: true
Attachment #8739932 - Flags: review?(aleth)
Comment on attachment 8739932 [details] [diff] [review]
Attachment for Bug 1255504

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

(fwd)
Attachment #8739932 - Flags: review?(aleth) → review?(florian)
Hi Flo, new contributor review ping...
Flags: needinfo?(florian)
Flags: needinfo?(florian)
Attachment #8739932 - Flags: review?(florian)
Thunderbird is the next version of Instantbird, http://blog.queze.net/post/2017/10/18/Thunderbird-is-the-next-version-of-Instantbird

We are no longer maintaining this preferences window, so closing as wontfix. Sorry I didn't provide timely feedback here :-(.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.