Last Comment Bug 589601 - Provide a fast bookmarking button in the urlbar
: Provide a fast bookmarking button in the urlbar
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.1b1
Assigned To: Robert Kaiser
:
:
Mentors:
Depends on: SMPlacesBMarks
Blocks: 590105 591344
  Show dependency treegraph
 
Reported: 2010-08-22 13:50 PDT by Robert Kaiser
Modified: 2010-09-06 08:48 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1: implement it (Win/Linux default theme only) (14.56 KB, patch)
2010-08-22 14:56 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.1: address comments (14.50 KB, patch)
2010-08-27 07:41 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.2: address more comments (14.11 KB, patch)
2010-09-03 17:39 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.3: improve hover/active icons (14.49 KB, patch)
2010-09-04 13:33 PDT, Robert Kaiser
neil: review+
Details | Diff | Splinter Review
v2: include mac and Modern (22.88 KB, patch)
2010-09-05 11:39 PDT, Robert Kaiser
neil: review+
Details | Diff | Splinter Review
followup: revert and comment onItemRemoved change (1.00 KB, patch)
2010-09-06 08:30 PDT, Robert Kaiser
neil: review-
Details | Diff | Splinter Review

Description Robert Kaiser 2010-08-22 13:50:54 PDT
In the beginning, when starting places bookmarks work, I didn't believe the "star" button would be a good idea, but the more I think about it, I think it would be a good idea to have it after all - I just don't like the star symbol for it, I think it should be a variation of the bookmark symbol.
Comment 1 Robert Kaiser 2010-08-22 14:56:27 PDT
Created attachment 468174 [details] [diff] [review]
v1: implement it (Win/Linux default theme only)

This patch implements the button, but right now for the Win/Linux default theme only. Nothing against Mac or Modern users, but I'd like to have Neil's review on those icons first, before creating a patch that includes Mac and Modern.
Comment 2 Philip Chee 2010-08-22 19:05:06 PDT
If you aren't going to use the star symbol then you shouldn't name all your methods, functions, and identifiers foo*Star*Bar
Comment 3 Robert Kaiser 2010-08-22 19:16:15 PDT
(In reply to comment #2)
> If you aren't going to use the star symbol then you shouldn't name all your
> methods, functions, and identifiers foo*Star*Bar

Funny that *you* would want to talk me/us out of add-on compat measures. :P
Comment 4 Philip Chee 2010-08-22 21:15:46 PDT
> Funny that *you* would want to talk me/us out of add-on compat measures. :P

Well I don't like having a star or anything icon in the url bar so making it difficult to overlay is a plus point :D .
Comment 5 Robert Kaiser 2010-08-23 06:04:21 PDT
(In reply to comment #4)
> Well I don't like having a star or anything icon in the url bar

Interesting to hear that, as 1) we already have up to 2 icons there, depending on the visited page, and 2) it makes me wonder why you don't want a fast way to bookmark the currently visited page with one click and/ore see if the current page is in your bookmarks.
Comment 6 Jens Hatlak (:InvisibleSmiley) 2010-08-23 06:18:15 PDT
FWIW, I for one am seeing forward to retiring my Bookmark Indicator extension. And the new way is obviously much more elegant thanks to Places observers. :-)
Comment 7 neil@parkwaycc.co.uk 2010-08-23 07:16:04 PDT
Comment on attachment 468174 [details] [diff] [review]
v1: implement it (Win/Linux default theme only)

>+  PlacesStarButton.updateState();
>+
>   PlacesToolbarHelper.customizeDone();
What state gets lost?

>+  uninit: function PSB_uninit() {
>+    PlacesUtils.bookmarks.removeObserver(this);
Does this throw if the init failed?

>+    this._starred = uri && (PlacesUtils.getMostRecentBookmarkForURI(uri) != -1 ||
>+                            PlacesUtils.getMostRecentFolderForFeedURI(uri) != -1);
[If uri is null then we've got real problems!]

>+    // don't bubble to the textbox so that the address won't be selected
>+    aEvent.stopPropagation();
[Doorhangers needs this on Windows; Linux popups work differently.]

>+  onItemAdded: function PSB_onItemAdded(aItemId, aFolder, aIndex, aItemType) {
>+    if (!this._batching && !this._starred)
>+      this.updateState();
>+  },
>+
>+  onBeforeItemRemoved: function() {},
>+
>+  onItemRemoved: function PSB_onItemRemoved(aItemId, aFolder, aIndex,
>+                                            aItemType) {
>+    if (!this._batching)
&& this._starred
[Can't remove an unstarred URL!]

>diff --git a/suite/themes/classic/communicator/bookmarks/bookmark.png b/suite/themes/classic/communicator/bookmarks/bookmark.png
>new file mode 100644
This looks suspiciously like a star to me ;-)
[Well actually it looked like a blob, I had to zoom it in Paint first...]
Instead, you could have used:
bookmark-item-dis.png (not bookmarked)
bookmark-item.png (bookmarked)
bookmark-item-updated.png (hover)
If you're worried about confusion with the page proxy icon, then simply restore the old page proxy icons that you removed for some reason as part of bug 580660
Comment 8 neil@parkwaycc.co.uk 2010-08-23 16:32:08 PDT
Comment on attachment 468174 [details] [diff] [review]
v1: implement it (Win/Linux default theme only)

>             <image id="feedsButton" hidden="true" popup="feedsPopup"/>
>+            <image id="star-button"
>+                   onclick="PlacesStarButton.onClick(event);"/>
>             <image id="ev-button" hidden="true"
>                    onclick="if (event.button == 0) BrowserPageInfo(null, 'securityTab');"/>
Oh, and why *between* feeds and ev, of all places?
Comment 9 Robert Kaiser 2010-08-24 07:45:45 PDT
(In reply to comment #7)
> >+  PlacesStarButton.updateState();
> >+
> >   PlacesToolbarHelper.customizeDone();
> What state gets lost?

I didn't check myself, but copied that from the FF side - can't harm to have it here, can it?

> >diff --git a/suite/themes/classic/communicator/bookmarks/bookmark.png b/suite/themes/classic/communicator/bookmarks/bookmark.png
> >new file mode 100644
> This looks suspiciously like a star to me ;-)
> [Well actually it looked like a blob, I had to zoom it in Paint first...]

The idea was to have some kind of a glow around the icon when the bookmark is set, but a simple round one seemed too boring.

> Instead, you could have used:
> bookmark-item-dis.png (not bookmarked)
> bookmark-item.png (bookmarked)

That's actually what those icons are based on quite heavily and very intentionally, but those icons themselves looked too boring for me and didn't cover the hover and active states well enough.

And no, I won't restore any dithered NS4-style icon, those don't fit the current icon theme and were rightfully and intentionally removed from it (and, btw, Modern already did use bookmark-item for the default page proxy icon anyhow).

(In reply to comment #8)
> Oh, and why *between* feeds and ev, of all places?

Because I felt like that when I added it. I don't have any specific reason or preference for before/between/after right now - maybe after makes sense due to that never making it shift position, as it's always shown.
Comment 10 Robert Kaiser 2010-08-27 07:39:34 PDT
(In reply to comment #7)
> >+  PlacesStarButton.updateState();
> >+
> >   PlacesToolbarHelper.customizeDone();
> What state gets lost?

Well, are we sure URLs get updated and the state of the button updated when the urlbar isn't on the toolbar? If so, it sounds like we're wasting perf in that case. :P

> >+  uninit: function PSB_uninit() {
> >+    PlacesUtils.bookmarks.removeObserver(this);
> Does this throw if the init failed?

Fun, init must in fact never fail in practice, or Firefox would have seen a problem there.

> >+    // don't bubble to the textbox so that the address won't be selected
> >+    aEvent.stopPropagation();
> [Doorhangers needs this on Windows; Linux popups work differently.]

Just a personal remark, or something I need to react on?

> bookmark-item-updated.png (hover)
[wow, was that one ugly - though, sure made sense for updated, but much less for hovered.]

I don't really see a reason listed for changing the icons from what they are in my patch or a reasonable target to change to. Still, I'm holding off any work on mac or modern themes until I see if those icons make it for the default theme.

And not getting a reasonable feedback on the position, I'm just trying to get another one by your review and see what comes out of it.
Comment 11 Robert Kaiser 2010-08-27 07:41:29 PDT
Created attachment 469872 [details] [diff] [review]
v1.1: address comments

Here's a patch that addresses the comments as far as I could make sense of them.
Comment 12 Jens Hatlak (:InvisibleSmiley) 2010-08-27 14:48:43 PDT
(In reply to comment #10)
> And not getting a reasonable feedback on the position, I'm just trying to get
> another one by your review and see what comes out of it.

If that was a request for feedback from a broader audience: I'd prefer to have it always at the end (well, before the drop-down indicator of course) as well.
Comment 13 Igor Velkov 2010-08-27 15:08:03 PDT
+1
Comment 14 neil@parkwaycc.co.uk 2010-08-31 13:36:48 PDT
(In reply to comment #10)
> (In reply to comment #7)
> > >+  PlacesStarButton.updateState();
> > >+
> > >   PlacesToolbarHelper.customizeDone();
> > What state gets lost?
> Well, are we sure URLs get updated and the state of the button updated when the
> urlbar isn't on the toolbar? If so, it sounds like we're wasting perf in that
> case. :P
We always keep things lying around in the DOM whether they are customised or not. This generally reduces the amount of code we have to write (not having to check whether things are in the DOM, or fix them after customisation).

> > >+    // don't bubble to the textbox so that the address won't be selected
> > >+    aEvent.stopPropagation();
> > [Doorhangers needs this on Windows; Linux popups work differently.]
> Just a personal remark, or something I need to react on?
Not for this bug, but we might need it for the doorhangers bug.

> > bookmark-item-updated.png (hover)
> [wow, was that one ugly - though, sure made sense for updated, but much less
> for hovered.]
It wasn't ideal, but it was at least obvious. I had to squint to see the difference between your images.
Comment 15 neil@parkwaycc.co.uk 2010-08-31 13:44:06 PDT
(In reply to comment #14)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > >+    // don't bubble to the textbox so that the address won't be selected
> > > >+    aEvent.stopPropagation();
> > > [Doorhangers needs this on Windows; Linux popups work differently.]
> > Just a personal remark, or something I need to react on?
> Not for this bug, but we might need it for the doorhangers bug.
Oh, and I just noticed the EV button needs this too...
Comment 16 neil@parkwaycc.co.uk 2010-08-31 13:59:22 PDT
Comment on attachment 469872 [details] [diff] [review]
v1.1: address comments

>+    var starIcon = document.getElementById("star-button");
>+    if (!starIcon)
>+      return;
This is another artefact of Firefox toolbar customisation - they are never sure whether the icon exists in the document, whereas we can always assume it's there. r=me on the code changes with this removed.
Comment 17 neil@parkwaycc.co.uk 2010-08-31 16:34:47 PDT
So we can land this, let's just stick with the disabled (unbookmarked) and standard bookmark icons in all themes for now, and file a new bug for the flame war over what this icon should actually look like.
Comment 18 Robert Kaiser 2010-08-31 17:50:51 PDT
(In reply to comment #14)
> I had to squint to see the
> difference between your images.

It's only meant to have a feelable glow, not a substantially visible difference. So what you say actually just confirms that I reached the goals I intended.

(In reply to comment #17)
> So we can land this, let's just stick with the disabled (unbookmarked) and
> standard bookmark icons in all themes for now, and file a new bug for the flame
> war over what this icon should actually look like.

I don't think that I feel like landing this without the icon work. Either we do things decently, or do it without me.
Comment 19 neil@parkwaycc.co.uk 2010-09-01 03:16:33 PDT
Comment on attachment 469872 [details] [diff] [review]
v1.1: address comments

>             <image id="feedsButton" hidden="true" popup="feedsPopup"/>
>             <image id="ev-button" hidden="true"
>                    onclick="if (event.button == 0) BrowserPageInfo(null, 'securityTab');"/>
>+            <image id="star-button"
>+                   onclick="PlacesStarButton.onClick(event);"/>
/me wonders whether it would be better to attac the panel directly to the star-button (as per the feeds button).
Comment 20 Robert Kaiser 2010-09-03 17:39:05 PDT
Created attachment 472051 [details] [diff] [review]
v1.2: address more comments

OK, this one should address the new comments as well - with the exception of the icons, which I really see no reason for changing.
Comment 21 neil@parkwaycc.co.uk 2010-09-04 08:02:14 PDT
(In reply to comment #19)
>(From update of attachment 469872 [details] [diff] [review])
>>             <image id="feedsButton" hidden="true" popup="feedsPopup"/>
>>             <image id="ev-button" hidden="true"
>>                    onclick="if (event.button == 0) BrowserPageInfo(null, 'securityTab');"/>
>>+            <image id="star-button"
>>+                   onclick="PlacesStarButton.onClick(event);"/>
>/me wonders whether it would be better to attach the panel directly to the
>star-button (as per the feeds button).
No, it doesn't actually work (click the star button slowly to see why.)
Comment 22 neil@parkwaycc.co.uk 2010-09-04 08:12:08 PDT
The hover state is supposed to be brighter than the active state. But when the places star button is unstarred, the hover state is dimmer than the active state, and when it is starred, it seems to be the same as the active state.
Comment 23 Robert Kaiser 2010-09-04 13:21:26 PDT
(In reply to comment #22)
> The hover state is supposed to be brighter than the active state. But when the
> places star button is unstarred, the hover state is dimmer than the active
> state, and when it is starred, it seems to be the same as the active state.

When the places button is starred, the hover is definitely slightly lighter, very similar in effect to our main buttons. For the unstarred case, there is definitely no precedence case or rule for hovering over a disabled-looking icon which has an action to active what it stands for - if there is any, it is to make it "light up" in a way that it looks more similar to enabled - which is exactly what this hover icon does.

(In reply to comment #21)
> >/me wonders whether it would be better to attach the panel directly to the
> >star-button (as per the feeds button).
> No, it doesn't actually work

OK, removing this again locally.
Comment 24 Robert Kaiser 2010-09-04 13:33:48 PDT
Created attachment 472192 [details] [diff] [review]
v1.3: improve hover/active icons

OK, I took another look at the hover/active icons and improve them a bit so there is a visible difference between those two states.
Comment 25 neil@parkwaycc.co.uk 2010-09-05 09:43:34 PDT
Comment on attachment 472192 [details] [diff] [review]
v1.3: improve hover/active icons

Hover/active state for a bookmarked page now looks good, but you don't seem to have changed the hover/active state for an unbookmarked page. I would be happy with the unbookmarked hover and active states switched in the image, so if you are happy with that too, then r=me with that change.
Comment 26 Robert Kaiser 2010-09-05 11:39:26 PDT
Created attachment 472288 [details] [diff] [review]
v2: include mac and Modern

I'm re-requesting review here just because of the adding of the mac and Modern code. I adjusted the image for Classic once more to improve unmarked hover/active states, and Neil has agreed to that version over IRC. Now I added the same CSS for Mac and Modern, and created a Modern icon set based on its bookmark icons and using the same hue as for nav bar hover for the starry glow in the bookmarked state.
Comment 27 Robert Kaiser 2010-09-06 06:35:18 PDT
Pushed as http://hg.mozilla.org/comm-central/rev/831ed4c403f6
Comment 28 Robert Kaiser 2010-09-06 08:21:07 PDT
Hrm, I just realized I didn't backport my addressing of bug 591344 comment #2 to this patch, I'll attach a followup shortly.
Comment 29 Robert Kaiser 2010-09-06 08:30:13 PDT
Created attachment 472410 [details] [diff] [review]
followup: revert and comment onItemRemoved change

Here's the followup on Marco's comment about onItemRemoved.
Comment 30 neil@parkwaycc.co.uk 2010-09-06 08:48:45 PDT
Comment on attachment 472410 [details] [diff] [review]
followup: revert and comment onItemRemoved change

>-    if (!this._batching && this._starred)
Marco misread this, he thought it said !this._starred
(maybe it would be worth putting the starred check first in both cases?)

Note You need to log in before you can comment on or make changes to this bug.