Closed Bug 589601 Opened 14 years ago Closed 14 years ago

Provide a fast bookmarking button in the urlbar

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1b1

People

(Reporter: kairo, Assigned: kairo)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #468174 - Flags: review?(neil)
If you aren't going to use the star symbol then you shouldn't name all your methods, functions, and identifiers foo*Star*Bar
(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
> 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 .
(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.
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 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 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?
Blocks: 590105
(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.
(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.
Attached patch v1.1: address comments (obsolete) — Splinter Review
Here's a patch that addresses the comments as far as I could make sense of them.
Attachment #468174 - Attachment is obsolete: true
Attachment #469872 - Flags: review?(neil)
Attachment #468174 - Flags: review?(neil)
Blocks: 591344
(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.
+1
(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.
(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 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.
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.
(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 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).
Attachment #469872 - Flags: review?(neil)
Attached patch v1.2: address more comments (obsolete) — Splinter Review
OK, this one should address the new comments as well - with the exception of the icons, which I really see no reason for changing.
Attachment #469872 - Attachment is obsolete: true
Attachment #472051 - Flags: review?(neil)
(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.)
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.
(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.
Attached patch v1.3: improve hover/active icons (obsolete) — Splinter Review
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.
Attachment #472051 - Attachment is obsolete: true
Attachment #472192 - Flags: review?(neil)
Attachment #472051 - Flags: review?(neil)
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.
Attachment #472192 - Flags: review?(neil) → review+
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.
Attachment #472192 - Attachment is obsolete: true
Attachment #472288 - Flags: review?(neil)
Attachment #472288 - Flags: review?(neil) → review+
Pushed as http://hg.mozilla.org/comm-central/rev/831ed4c403f6
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b1
Hrm, I just realized I didn't backport my addressing of bug 591344 comment #2 to this patch, I'll attach a followup shortly.
Here's the followup on Marco's comment about onItemRemoved.
Attachment #472410 - Flags: review?(neil)
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?)
Attachment #472410 - Flags: review?(neil) → review-
You need to log in before you can comment on or make changes to this bug.