Closed
Bug 346782
Opened 18 years ago
Closed 18 years ago
Bookmark toggle behavior is broken
Categories
(Camino Graveyard :: Bookmarks, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: stuart.morgan+bugzilla, Assigned: froodian)
References
Details
(Keywords: regression, verified1.8.0.7, verified1.8.1)
Attachments
(2 files, 4 obsolete files)
5.57 KB,
patch
|
stuart.morgan+bugzilla
:
review+
mikepinkerton
:
superreview+
|
Details | Diff | Splinter Review |
5.60 KB,
patch
|
alqahira
:
review+
mikepinkerton
:
superreview+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•18 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•18 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•18 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•18 years ago
|
||
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)
Comment 8•18 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•18 years ago
|
||
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•18 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•18 years ago
|
Attachment #231976 -
Flags: review?(stuart.morgan)
Comment 11•18 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•18 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•18 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•18 years ago
|
||
Attachment #231976 -
Attachment is obsolete: true
Attachment #232275 -
Flags: review?(stuart.morgan)
Comment 16•18 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•18 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•18 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•18 years ago
|
||
Attachment #232275 -
Attachment is obsolete: true
Attachment #234702 -
Flags: review?(stuart.morgan)
Reporter | ||
Comment 20•18 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•18 years ago
|
||
Attachment #234702 -
Attachment is obsolete: true
Attachment #234721 -
Flags: review?(stuart.morgan)
Reporter | ||
Comment 22•18 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 23•18 years ago
|
||
Comment on attachment 234721 [details] [diff] [review] Patch sr=pink
Attachment #234721 -
Flags: superreview?(mikepinkerton) → superreview+
Updated•18 years ago
|
Whiteboard: [needs checkin]
Comment 24•18 years ago
|
||
Checked into trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Closed: 18 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•18 years ago
|
||
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 28•18 years ago
|
||
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)
Comment 29•18 years ago
|
||
Pending the SR, this bug is approved for 1.0.3.
Flags: camino1.0.3? → camino1.0.3+
Keywords: regression
Comment 30•18 years ago
|
||
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+
Updated•18 years ago
|
Whiteboard: [needs checkin 180branch]
Assignee | ||
Updated•18 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•18 years ago
|
Keywords: verified1.8.1
Assignee | ||
Updated•18 years ago
|
Keywords: fixed1.8.1
Someone please verify tomorrow and changed fixed1.8.0.7 to verified1.8.0.7.
Keywords: fixed1.8.0.7
Assignee | ||
Updated•18 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.
Description
•