Bookmark toggle behavior is broken

VERIFIED FIXED

Status

Camino Graveyard
Bookmarks
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Stuart Morgan, Assigned: froodian (Ian Leue))

Tracking

({regression, verified1.8.0.7, verified1.8.1})

1.8 Branch
PowerPC
Mac OS X
regression, verified1.8.0.7, verified1.8.1
Bug Flags:
camino1.0.3 +

Details

Attachments

(2 attachments, 4 obsolete attachments)

5.57 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
5.60 KB, patch
Smokey Ardisson (offline for a while; not following bugs - do not email)
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
The current bookmark toggle behavior is broken (on recent branch at least; I haven't tested trunk):

1) Open some page
2) Press Command-B or the bookmark toolbar button
3) Press Command-Y
4) Press Command-B or the bookmark toolbar button

This should take you back to the page from step 1), per the "Show/Hide bookmarks is always a bookmark manager toggle" model, but instead takes you to about:bookmarks. I'm pretty sure this is a regression, but again I haven't tested.

(Please refrain from discussion of whether or not you find the current model distasteful (a la bug 311781); that is supposed to be the model at least for the present time, and it should work correctly to minimize confusion.)
(Reporter)

Updated

11 years ago
Blocks: 235863
(Assignee)

Comment 1

11 years ago
We do this in 1.0.2 at least, and I can't ever remember it not working like this.  I agree it's "broken" by our current paradigm, not I'm not sure it ever did anything different.
It's broken on both trunk *and* 1.0.2, too.  When in the name of pink panthers did this regress?
Keywords: qawanted
So far I have this narrowed down to breaking between 1 Mar 2005 and 26 Jun 2005.  I may or may not get any closer soon.
Keywords: regression
So, the closest I can get this based on builds I have is somewhere between 3 Mar 2005 and 1 Apr 2005; bonsai: http://tinyurl.com/owdy7

Lots of fun checkins that whole period, so grab builds and have fun....
(Reporter)

Comment 5

11 years ago
It's fairly straightforward to fix this, so there's no real need to find the exact date/checkin that regressed it.
(Assignee)

Comment 6

11 years ago
Interestingly enough, the patch that broke it (from bug 215235) did it on purpose:

 // - manageBookmarks:
 //
-// Toggle the bookmark manager in all cases. This is what users
-// expect to happen. When the manager is displayed, retain the
-// last selected collection, regardless of what it is.
+// Load the bookmarks in the frontmost tab or window.
 //
 -(IBAction)manageBookmarks: (id)aSender
 {
-  [self toggleBookmarkManager: self];
+  [self loadURL:@"about:bookmarks" referrer:nil activate:YES allowPopups:YES];
 }

What support we have for bookmarks toggle now was cobbled back together after that.  Taking.
Assignee: nobody → stridey
(Assignee)

Comment 7

11 years ago
Created attachment 231961 [details] [diff] [review]
Recursive Solution

I thought about retaining the current page before we load bookmarks, but there are a lot of ways for bookmarks to get loaded (if it's a bookmark, for instance), and I think this will work more reliably.  Also, I know this patch is only in BWC, but yes, everything will still work for the menu items in MainController.  We're that modular.  ;)
Attachment #231961 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Keywords: qawanted

Comment 8

11 years ago
Comment on attachment 231961 [details] [diff] [review]
Recursive Solution

I would prefer a non-recursive solution to this, especially since it spans between two methods.
(Assignee)

Comment 9

11 years ago
Created attachment 231976 [details] [diff] [review]
Non-recursive Solution

Since recursion got the #camino slapdown, a non-recursive solution (which is actually far more elegant in addition to being more efficient).
Attachment #231961 - Attachment is obsolete: true
Attachment #231976 - Flags: review?(nick.kreeger)
Attachment #231961 - Flags: review?(stuart.morgan)

Comment 10

11 years ago
Comment on attachment 231976 [details] [diff] [review]
Non-recursive Solution

Looks ok to me.
Attachment #231976 - Flags: review?(nick.kreeger) → review+
(Assignee)

Updated

11 years ago
Attachment #231976 - Flags: review?(stuart.morgan)

Comment 11

11 years ago
(in attachment 231976 [details] [diff] [review], manageBookmarks:)
+    [self loadURL:[self pageBeforeBookmarksManager] referrer:nil activate:YES allowPopups:NO];

This worries me a little bit.  We're reloading the page with no referrer no matter what.  That could be hazardous in some cases.  I'm assuming that [self back:] is a little more intelligent than that, and preserves more state.

Not that I can offer a better solution off the top of my head, and it's not a terribly damning flaw.
In most cases the page should come out of bfacache, right, and not be reloaded at all?  That doesn't necessarily excuse the issue....
(Assignee)

Comment 13

11 years ago
Wait, are you just worried because I send "referrer:nil"?  Almost all ways of loading a page other than following a link (location bar + return, loading a bookmark, hitting the "home" button) send referrer:nil.
(Reporter)

Comment 14

11 years ago
Comment on attachment 231976 [details] [diff] [review]
Non-recursive Solution

You don't want to load the page, you want to go back to the page; it's not the same thing.  See for example the implementation of historyItemAction:
Attachment #231976 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 15

11 years ago
Created attachment 232275 [details] [diff] [review]
Loads page from session history
Attachment #231976 - Attachment is obsolete: true
Attachment #232275 - Flags: review?(stuart.morgan)

Comment 16

11 years ago
(In reply to comment #15)
> Created an attachment (id=232275) [edit]
> Loads page from session history

Only one little nit to pick.  relativeHistoryIndexOfPageBeforeBookmarksManager returns nil if !pageURL, which will happen when ![self canHideBookmarks].  Isn't this an error condition?  We assume that [self canHideBookmarks] in manageBookmarks:, and validate the interface so manageBookmarks: is only called if it's true.  If it were called when ![self canHideBookmarks], manageBookmarks: would simply return silently, and the user would be left wondering what happened.  Instead, we should probably throw an exception or log to the console or something.  How do we usually handle such conditions?

Also (I know I said only one), rHIOPBBM has a rogue colon on its end in the comment above it.  If there's a new patch, it should change that.  No biggie.
(Assignee)

Comment 17

11 years ago
(In reply to comment #16)
> Only one little nit to pick.  relativeHistoryIndexOfPageBeforeBookmarksManager
> returns nil if !pageURL, which will happen when ![self canHideBookmarks]. 
> Isn't this an error condition?  We assume that [self canHideBookmarks] in
> manageBookmarks:, and validate the interface so manageBookmarks: is only called
> if it's true.  If it were called when ![self canHideBookmarks],
> manageBookmarks: would simply return silently, and the user would be left
> wondering what happened.  Instead, we should probably throw an exception or log
> to the console or something.  How do we usually handle such conditions?

Probably #if DEBUG NSLog(), but I don't see how it would ever happen.  As you say, the UI is validated so that |manageBookmarks| is only called if [self canHideBookmarks] is true (or we're not showing the manager yet), so rHIOPBBM should never return nil to it - I just put the "fail silently" if statement there to be paranoid.

That said, if people think I should log (or something else), I'm happy to do it.
(Reporter)

Comment 18

11 years ago
Comment on attachment 232275 [details] [diff] [review]
Loads page from session history

>   else if (action == @selector(manageBookmarks:))
>-    return [[mBrowserView getBrowserView] canGoBack] || (![self bookmarkManagerIsVisible]);
>+    return ([self canHideBookmarks] || ![self bookmarkManagerIsVisible]);

Reverse the order of these checks, since [self bookmarkManagerIsVisible] is significantly lighter-weight than [self canHideBookmarks]

> -(IBAction)manageBookmarks:(id)aSender
> {
>-  if ([self bookmarkManagerIsVisible])
>-    [self back:aSender];
>+  if ([self bookmarkManagerIsVisible]) {
>+    nsIWebNavigation* webNav = [self currentWebNavigation];
>+    if (!webNav)
>+      return;
>+
>+    nsCOMPtr<nsISHistory> sessionHistory;
>+    webNav->GetSessionHistory(getter_AddRefs(sessionHistory));
>+    if (!sessionHistory)
>+      return;
>+
>+    PRInt32 curEntryIndex;
>+    sessionHistory->GetIndex(&curEntryIndex);
>+
>+    // Go to the index in session history of the last page before the manager was shown.
>+    PRInt32 relativeHistoryIndex = [self relativeHistoryIndexOfPageBeforeBookmarksManager];
>+    if (!relativeHistoryIndex)
>+      return;
>+    webNav->GotoIndex(curEntryIndex + relativeHistoryIndex);
>+  }
>   else
>     [self loadURL:@"about:bookmarks" referrer:nil activate:YES allowPopups:NO];
> 
>   [[NSApp delegate] delayedAdjustBookmarksMenuItemsEnabling];
> }

I don't like having all this raw Gecko stuff in such a high-level function. Since having just the relative index doesn't really do anything useful, make the function historyIndexOfPageBeforeBookmarkManager--returning the index directly, or -1 on failure--and move most of this code into it. Then use goToSessionHistoryIndex: with the result.

Since at that point you'll have the raw nsISHistory pointer anyway, you might want to loop through history URL directly instead of using sessionHistoryItemAtRelativeOffset:, to make it more efficient (otherwise you are doing the overhead of the setup every time, and are doing unnecessary URL extractions).

>+- (int)relativeHistoryIndexOfPageBeforeBookmarksManager
>+{
>+  if (![self bookmarkManagerIsVisible])
>+    return nil;

It won't matter for the next version, but nil is a pointer value, and should never be returned from an (int) function.


>+  while (pageURL && ([pageURL isEqualToString:@"about:bookmarks"] || [pageURL isEqualToString:@"about:history"])) {
>+    // the URL at a relative session history of 0 is always about:bookmarks/history, so start at -1 to save a useless loop iteration.
>+    // Also, note that order is important here to ensure that |relativeSessionHistoryCounter| is correct when we leave the loop.
>+    --relativeSessionHistoryCounter;
>+    [self sessionHistoryItemAtRelativeOffset:relativeSessionHistoryCounter forWrapper:[self getBrowserWrapper] title:&pageTitle URL:&pageURL];
>+  }

If you keep this, or do something similar with the raw nsIHistory, use a do {...} while (<condition>) to avoid the useless pageURL initialization and first comparison.
Attachment #232275 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 19

11 years ago
Created attachment 234702 [details] [diff] [review]
Patch
Attachment #232275 - Attachment is obsolete: true
Attachment #234702 - Flags: review?(stuart.morgan)
(Reporter)

Comment 20

11 years ago
Comment on attachment 234702 [details] [diff] [review]
Patch

Much better.  Just a couple small tweaks:

>   if ([self bookmarkManagerIsVisible])
>-    [self back:aSender];
>+    [[[self getBrowserWrapper] getBrowserView] goToSessionHistoryIndex:[self historyIndexOfPageBeforeBookmarkManager]];

Check that the return value from historyIndexOf... is not -1 before passing it into goToSessionHistoryIndex.

>+  if (curEntryIndex == 0)
>+    return -1;
>+
>+  nsCOMPtr<nsIHistoryEntry> entry;
>+  NSString* pageURL;
>+  int sessionHistoryCounter = curEntryIndex;
>+  do {
>+    --sessionHistoryCounter;
>+    sessionHistory->GetEntryAtIndex(sessionHistoryCounter, PR_FALSE, getter_AddRefs(entry));
>+
>+    nsCAutoString uriSpec;
>+    nsCOMPtr<nsIURI> entryURI;
>+    entry->GetURI(getter_AddRefs(entryURI));
>+    if (entryURI)
>+      entryURI->GetSpec(uriSpec);
>+    pageURL = [NSString stringWithUTF8String:uriSpec.get()];
>+  } while ((sessionHistoryCounter > 0) && ([pageURL isEqualToString:@"about:bookmarks"] || [pageURL isEqualToString:@"about:history"]));
>+
>+  if (!entry)
>+    return -1;
>+
>+  return sessionHistoryCounter;

First, use EqualsLiteral with the entryURI, rather than converting each time to an NSString to do the comparison.  Second, the exit handling isn't correct, since entry will be non-NULL regardless of whether or not a non-about:history/bookmarks page was found; I think you'll have an easier time replacing everything quoted above with a structure like:

  for (int i; i=curEntryIndex-1; --i) {
    <get entryURI>
    if (!(<entrtyURI equals about:bookmarks> || <entryURI equals about:history>))
      return i;
  }

  return -1;
Attachment #234702 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 21

11 years ago
Created attachment 234721 [details] [diff] [review]
Patch
Attachment #234702 - Attachment is obsolete: true
Attachment #234721 - Flags: review?(stuart.morgan)
(Reporter)

Comment 22

11 years ago
Comment on attachment 234721 [details] [diff] [review]
Patch

Looks good, r=me.

Thanks for your patience (and for translating my garbage for-loop into what I actually meant; my brain and hands don't seem to be connected recently).
Attachment #234721 - Flags: superreview?(mikepinkerton)
Attachment #234721 - Flags: review?(stuart.morgan)
Attachment #234721 - Flags: review+
Comment on attachment 234721 [details] [diff] [review]
Patch

sr=pink
Attachment #234721 - Flags: superreview?(mikepinkerton) → superreview+

Updated

11 years ago
Whiteboard: [needs checkin]

Comment 24

11 years ago
Checked into trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: regression → fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
Since this regressed before 1.0.x shipped, we should consider taking this for 1.0.3 (if it's safe/simple)
Flags: camino1.0.3?
(Assignee)

Comment 26

11 years ago
Created attachment 235037 [details] [diff] [review]
Patch ported for 1.8.0 branch

This patch is ported to the 1.8.0 branch, and *should* work fine, though I haven't built with it.  Requesting review from smokey for testing.
Attachment #235037 - Flags: review?(alqahira)
Comment on attachment 235037 [details] [diff] [review]
Patch ported for 1.8.0 branch

r=ardissone in that I've tested the port of the patch to 1.8.0 and it works properly and appears to have no unintended consequences.

Given that, I think we want to get this on 180branch for 1.0.3.
Attachment #235037 - Flags: review?(alqahira) → review+
Comment on attachment 235037 [details] [diff] [review]
Patch ported for 1.8.0 branch

Mike, can you please SR this patch (which is very similar to the one above, which you've already SRed) to be sure that it doesn't have any bad consequences and is safe for the 1.8.0 branch and Camino 1.0.3?
Attachment #235037 - Flags: superreview?(mikepinkerton)
Pending the SR, this bug is approved for 1.0.3.
Flags: camino1.0.3? → camino1.0.3+
Keywords: regression
Comment on attachment 235037 [details] [diff] [review]
Patch ported for 1.8.0 branch

sr=pink. looks fine to me.
Attachment #235037 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [needs checkin 180branch]
(Assignee)

Updated

11 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Updated

11 years ago
Keywords: verified1.8.1
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1

Comment 31

11 years ago
Landed on the MOZILLA_1_8_0_BRANCH
Whiteboard: [needs checkin 180branch]
Someone please verify tomorrow and changed fixed1.8.0.7 to verified1.8.0.7.
Keywords: fixed1.8.0.7
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.7 → verified1.8.0.7
You need to log in before you can comment on or make changes to this bug.