564573 - A page that's opened in a tab should appear exactly once, as switch-to-tab

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Drew Willcoxon :adw]

Frequently I'll start typing the title of a page that I know to be opened in some tab, but the result that appears near the top of the awesomebar list is not switch-to-tab but just a normal result.  So I have to scroll down the list and find the switch-to-tab result.

If a page appears in the awesomebar results and it's opened in a tab, it should appear exactly once in the results, and that appearance should be switch-to-tab.

Comment 1

[wiiwaker]

(In reply to comment #0) 
> If a page appears in the awesomebar results and it's opened in a tab, it should
> appear exactly once in the results, and that appearance should be
> switch-to-tab.

What if I want to open the same tab twice? My opinion is that the switch-to-tab result should be on the top of the list, then follows other results including the same normal result.

Comment 2

[Drew Willcoxon :adw]

No one but advanced users ever opens the same tab twice.  I'm an advanced user too, and I occasionally open a page twice, but I've never used the awesomebar to do it (except by mistake).  Have you?  What percentage of the time do you use the awesomebar to switch-to-tab vs. to open a page that's already opened?

Comment 3

[wiiwaker]

I prefer using Ctrl-Tab to switch tabs...
Awesomebar is the awesomest tool to open any page in Firefox. I rarely use sidebar or place manager to do this task nowadays. If it only has switch-to-tab result no normal result, then there is no convenient way to open that page twice.

Just my 2 cents.

Comment 4

[Elmar Ludwig]

This is basically a duplicate of bug 555689, except for the normal result being on top of the switch-to-tab one, which I do not see here.

Comment 5

[Drew Willcoxon :adw]

Bug 555689 describes the same problem, but this bug proposes a specific solution, so they're not quite duplicates.

Comment 6

[Elmar Ludwig]

That's the only possible solution, since hiding the other match (switch-to-tab) would effectively undo bug 480350.

Comment 7

[Drew Willcoxon :adw]

Well, I've heard other ideas: show both, but always switch-to-tab above the normal one; show both, but group all switch-to-tabs at the top.  This bug makes a specific proposal, and if it's what you also meant, feel free to mark your bug as a dupe of this one.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Limi - thoughts?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> Bug 555689 describes the same problem, but this bug proposes a specific
> solution, so they're not quite duplicates.

I don't see much value in keeping them separate for the moment - we can reopen if that changes.

Comment 11

[Drew Willcoxon :adw]

Attachment: patch

Limi, what does UX think?

We're actually adding the same result twice; I'd assumed it was a consequence of some complicated SQL query.  So this patch is really simple.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 450286 [details] [diff] [review]
patch

>diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js
>--- a/toolkit/components/places/src/nsPlacesAutoComplete.js
>+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js
>@@ -954,18 +954,21 @@ nsPlacesAutoComplete.prototype = {
>     // If actions aren't enabled, avoid doing any additional work.
>     if (!this._enableActions) {
>       this._addToResults(entryId, escapedEntryURL, title, entryFavicon, style);
>       return true;
>     }
> 
>     // Add a special entry for an open-page match.
>     if ((this._hasBehavior("openpage") || this._hasBehavior("everything")) &&
>-        openPageCount > 0)
>-      this._addToResults(entryId, "moz-action:switchtab," + escapedEntryURL, title, entryFavicon, "action");
>+        openPageCount > 0) {
>+      this._addToResults(entryId, "moz-action:switchtab," + escapedEntryURL,
>+                         title, entryFavicon, "action");
>+      return true;
>+    }

we could maybe change the 3 ifs to 1, the path is not crystal clear but looks like the !onlyHasBehavior is already handled by the first if if you early return, and the previous/next ifs are doing the same addition. Worth a try?

if (this._enableActions && openPageCount > 0 &&
    (this._hasBehavior("openpage") || this._hasBehavior("everything"))) {
  this._addToResults(entryId, "moz-action:switchtab," + escapedEntryURL,
                     title, entryFavicon, "action");
  return true;
}

this._addToResults(entryId, escapedEntryURL, title, entryFavicon, style);
return true;

Comment 13

[Elmar Ludwig]

(In reply to comment #12)
> we could maybe change the 3 ifs to 1, the path is not crystal clear but looks
> like the !onlyHasBehavior is already handled by the first if if you early
> return, and the previous/next ifs are doing the same addition. Worth a try?

The !onlyHasBehavior is supposed to suppress other (normal) matches if the "openpage" behaviour is set without any other behaviours. That case is not (and cannot be) handled by the first if.

But the logic for "openpage" matches here is flawed anyway as described in bug 564395 and needs to be fixed an/or redone IMHO.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #13)
> The !onlyHasBehavior is supposed to suppress other (normal) matches if the
> "openpage" behaviour is set without any other behaviours.

and in that case the new code would early return since openpage is set

Comment 15

[Elmar Ludwig]

(In reply to comment #14)
> and in that case the new code would early return since openpage is set

But only if openPageCount is > 0.

Comment 16

[Drew Willcoxon :adw]

Attachment: WIP

Thanks Marco and Elmar.  Elmar, does this patch do what you're asking?  I think it should fix this bug, bug 564395, and bug 564676.

Comment 17

[Drew Willcoxon :adw]

Attachment: patch v2

This cleans up the previous patch.  It also replaces _hasBehavior("everything") with a _hasAnyBehavior getter.  The point of _hasBehavior is to determine if the given restriction is set, so it's weird to have to ask for a special "everything" when you actually want to know if no restrictions are set.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 450408 [details] [diff] [review]
patch v2

Net effect here is that we no longer show both entries when doing an unrestricted search, right? That may be annoying if your intention is to actually load the page again... though probably not as annoying as this bug? I dunno - limi should chime in!

The behavior flag changes make sense to me, but I think mak should sign off (and Elmar approval wouldn't hurt either).

(I've gone a bit flag crazy, but this will hopefully produce a better result than being in my queue for a while!)

Comment 19

[Drew Willcoxon :adw]

(In reply to comment #18)
> (From update of attachment 450408 [details] [diff] [review])
> Net effect here is that we no longer show both entries when doing an
> unrestricted search, right?

Yes, along with fixing the two other bugs mentioned in comment 16.

> That may be annoying if your intention is to
> actually load the page again... though probably not as annoying as this bug?

Yeah.  I think the more common case is to switch tabs, not to duplicate them, for most users anyway.  There are other ways to dupe tabs, so that ability is not lost.  (It might be cool if you could hold Ctrl or whatever while the autocomplete popup is open to change the switch-to-tab entry into a normal entry that would open in a new tab.)

> (I've gone a bit flag crazy, but this will hopefully produce a better result
> than being in my queue for a while!)

Sure, thanks Gavin.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 450408 [details] [diff] [review]
patch v2

>diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js

>+      function addSwitchToTab(self) {
>+        self._addToResults(entryId, "moz-action:switchtab," + escapedEntryURL,
>+                           title, entryFavicon, "action");
>+      }
>+
>+      // If restricting to open pages and the page is open, we've found a
>+      // result.  If the page isn't open, there are no results.
>+      if (this._hasBehavior("openpage")) {
>+        if (openPageCount > 0) {
>+          addSwitchToTab(this);
>+          return true;
>+        }
>+        return false;
>+      }
>+
>+      // If there are no restrictions and the page is open, then show only the
>+      // switch-to-tab result.
>+      if (!this._hasAnyBehavior && openPageCount > 0) {
>+        addSwitchToTab(this);
>+        return true;
>+      }

this still looks more verbose than needed, what about:

// If restricting to open pages but the page is not open, bail out.
if (this._hasBehavior("openpage") && openPageCount == 0)
  return false;

// Otherwise if the page is open and we are restricting on open pages or nothing,
// show only the switch-to-tab result.
let hasCompatibleBehavior = this._hasBehavior("openpage") || !this_hasAnyBehavior;
if (hasCompatibleBehavior && openPageCount > 0) {
  this._addToResults(entryId, "moz-action:switchtab," + escapedEntryURL,
                     title, entryFavicon, "action");
  return true;
}

remaining changes looks fine to me

Comment 21

[Drew Willcoxon :adw]

Attachment: patch v2.1

Thanks Marco.

I'd like to land this before June 18 so it makes the beta 1 freeze.

Comment 22

[Alex Limi (:limi) — Firefox UX Team]

lThere was already a bug on this that took the approach of boosting weighting for open tabs, hich I still think is a better behavior. 

I don't have a really strong opinion here, landing this for beta1 and reevaluate if it doesn't work out is fine with me. Testing it in the wild for a bit and all that 

Comment 23

[Elmar Ludwig]

Comment on attachment 450745 [details] [diff] [review]
patch v2.1

>+      // If restricting to open pages and the page isn't open, it's not a match.
>+      let showOnlyOpenPages = this._hasBehavior("openpage");
>+      if (showOnlyOpenPages && openPageCount == 0)
>+        return false;

Is there any reason why this is done here instead of in MatchAutoCompleteFunction::OnFunctionCall() in SQLFunctions.cpp? All the other behaviour filters are implemented there, IIRC.

>+      // If allowing open pages and the page is open, its switch-to-tab result
>+      // is the only match.
>+      let showOpenPages = showOnlyOpenPages || !this._hasAnyBehavior;
>+      if (showOpenPages && openPageCount > 0) {

The behaviour filter should only control whether a match is shown, not whether it acts as "switch to tab" or not, so I think we can drop showOpenPages and _hasAnyBehavior() altogether and just use:

+      // If the page is open, its switch-to-tab result is the only match.
+      if (openPageCount > 0) {

If we decide later on that we want a pref for displaying "switch to tab" vs. normal matches for currently open tabs, that should be separate from the filtering options.

feedback=me with the condition fixed.

Comment 24

[Elmar Ludwig]

(In reply to comment #19)
> There are other ways to dupe tabs, so that ability is not lost.  (It might
> be cool if you could hold Ctrl or whatever while the autocomplete popup is
> open to change the switch-to-tab entry into a normal entry that would open
> in a new tab.)

You can Ctrl-click or middle-click the switch-to-tab entry to reload the page in a new tab. Pressing Alt+Enter should also work, but this seems broken right now (need to file a bug on this, Ctrl+Enter is also broken).

Comment 25

[Drew Willcoxon :adw]

Attachment: patch v3

(In reply to comment #23)
> Is there any reason why this is done here instead of in
> MatchAutoCompleteFunction::OnFunctionCall() in SQLFunctions.cpp? All the other
> behaviour filters are implemented there, IIRC.

> The behaviour filter should only control whether a match is shown, not whether
> it acts as "switch to tab" or not, so I think we can drop showOpenPages and
> _hasAnyBehavior() altogether and just use:
>
> >+      // If allowing open pages and the page is open, its switch-to-tab result
> >+      // is the only match.
> >+      let showOpenPages = showOnlyOpenPages || !this._hasAnyBehavior;
> >+      if (showOpenPages && openPageCount > 0) {

Thanks Elmar, you're exactly right on both counts.

Marco, requesting review again since with these changes the patch is substantially different (but still fairly small!).  Also, something I missed from the previous patches was correcting the default empty search behavior.  As Elmar mentions in bug 564395 and here, tab matches was misusing its behavior flag.  Since this patch corrects that, it also reverts the default empty search behavior to what it was before tab matches.  (Without this change, some tests were failing; with it, all pass.)

Comment 26

[Drew Willcoxon :adw]

Whoops, I quoted the wrong code of comment 23 for Elmar's second point.  Just to be clear, this is the part I meant to quote (and agreed with):

(In reply to comment #23)
> The behaviour filter should only control whether a match is shown, not whether
> it acts as "switch to tab" or not, so I think we can drop showOpenPages and
> _hasAnyBehavior() altogether and just use:
> 
> +      // If the page is open, its switch-to-tab result is the only match.
> +      if (openPageCount > 0) {

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 450967 [details] [diff] [review]
patch v3

>diff --git a/toolkit/components/places/src/SQLFunctions.cpp b/toolkit/components/places/src/SQLFunctions.cpp

>     PRInt32 visitCount = aArguments->AsInt32(kArgIndexVisitCount);
>     bool typed = aArguments->AsInt32(kArgIndexTyped) ? true : false;
>     bool bookmark = aArguments->AsInt32(kArgIndexBookmark) ? true : false;
>     nsAutoString tags;
>     (void)aArguments->GetString(kArgIndexTags, tags);
>+    PRInt32 openPageCount = aArguments->AsInt32(kArgIndexOpenPageCount);

It's unfortunate we use ->AsIntXX helpers here, they are bogus and supposed to work only for NOT NULL fields, see Bug 547190 (for which I was unable to get an answer on the expected behavior, even if I think being helpers they should return 0 for NULL).
Both kArgIndexBookmark and kArgIndexOpenPageCount can be NULL, in such a case we return uninitialized memory (that in most cases will be 0, but we can't know).
We should use proper GetIntXX methods here, otherwise if you can get an answer on expected behavior of that bug, we should really fix it.


>diff --git a/toolkit/components/places/src/SQLFunctions.h b/toolkit/components/places/src/SQLFunctions.h

>@@ -75,16 +76,18 @@ namespace places {

>  * @param aMatchBehavior
>  *        The match behavior to use for this search.
>  * @param aSearchBehavior
>  *        A bitfield dictating the search behavior.
>+ * @param aOpenPageCount
>+ *        The number of tabs containing aURL.

We should avoid the warm possibility of using the word "tabs", the API used open pages exactly because tabs is how we use the feature in the UI, but toolkit does not depend on having tabs. "Number of open pages containing aURL" will be fine.


>diff --git a/toolkit/components/places/src/nsPlacesAutoComplete.js b/toolkit/components/places/src/nsPlacesAutoComplete.js
>--- a/toolkit/components/places/src/nsPlacesAutoComplete.js
>+++ b/toolkit/components/places/src/nsPlacesAutoComplete.js
>@@ -222,17 +222,18 @@ function nsPlacesAutoComplete()
>                   "h.frecency " +
>            "FROM " + aTableName + " h " +
>            "LEFT OUTER JOIN moz_favicons f ON f.id = h.favicon_id " +
>            "LEFT OUTER JOIN moz_openpages_temp t ON t.place_id = h.id " +
>            "WHERE h.frecency <> 0 " +
>            "AND AUTOCOMPLETE_MATCH(:searchString, h.url, " +
>                                   "IFNULL(bookmark, h.title), tags, " +
>                                   "h.visit_count, h.typed, parent, " +
>-                                  ":matchBehavior, :searchBehavior) " +
>+                                  ":matchBehavior, :searchBehavior, " +
>+                                  "t.open_count) " +

matchBehavior and searchBehavior are injected entries not related to tables, from a logic grouping point of view looks like a better position for t.open_count would be before them.


>+    // If tab matches are enabled and the page is open, add only the switch-to-
>+    // tab result.  Otherwise, add the normal result.

s/tab matches are enabled/actions are enabled/


>+    let [url, style] = this._enableActions && openPageCount > 0 ?
>+                       ["moz-action:switchtab," + escapedEntryURL, "action"] :
>+                       [escapedEntryURL, style];
>+    this._addToResults(entryId, url, title, entryFavicon, style);

Ideally we could support different type of actions, this code compression is nice but built around the single action we have today, maybe you could do something more generic like
let action;
if (this._enableActions)
  if (openPageCount > 0)
    action = "switchtab";
let [url,style] = action ? ["moz-action:" + action + "," + url ...
btw, we could even just leave this to future since we don't know whether future actions will need multiple entries.


r=me with the above addressed/answered
Elmar, thanks for following this deeper and finding where we were differing from other behaviors.

So, from what I got this also fixes bug 564395, since the filtering is now at sqlite level and it's not just always returning the results and appending accordingly to action. And this caused the needed change to the default behavior (before it was _allowing_ switch to tab entries, while now it would have been restricting to them).
I don't recall the reason to not handling this at db level, probably was just intended as a way to being able to disable these results, as it was I could remove "openpage" behavior and get old style results, with this implementation, regardless status of the behavior, I'll get switch-to-tab results with others, and if behavior is set, I get only those.
I guess if we have to care about providing a way to disable actions (beta will tell us, most likely).

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Drew noticed the sqlite bug is less scary than what appears to be, so we can keep on using AsIntXX helpers here. thanks.

Comment 29

[Drew Willcoxon :adw]

Attachment: patch v3.1

Thanks Marco.  I plan on landing this as soon as I can.

Comment 30

[Drew Willcoxon :adw]

http://hg.mozilla.org/mozilla-central/rev/4a1553a6c306