Fix for "change bookmark's icon" notification

RESOLVED FIXED

Status

RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: dc2, Assigned: p_ch)

Tracking

({fixed1.7})

Trunk
fixed1.7

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040421
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040421


The "change the bookmark's icon" bookmark notification option works in the
personal toolbar folder in the classic theme but not in the modern theme. Adding
one new .gif and a new entry in a .css file to modern.jar brings the modern
theme in line with the classic theme.

First the .gif - I created a new bookmark notification icon following a similar
design in
http://lxr.mozilla.org/seamonkey/source/themes/modern/global/filepicker/folder-new.gif
. It would need to be saved to
themes/modern/communicator/bookmarks/bookmark-item-updated.gif .

Second the .css - the following lines need to be inserted into
http://lxr.mozilla.org/seamonkey/source/themes/modern/communicator/bookmarks/bookmarks.css#67
at line 67. It is based on lines 66-68 from
http://lxr.mozilla.org/seamonkey/source/themes/classic/communicator/bookmarks/bookmarks.css#66
.

.bookmark-item[status="new"],
treechildren::-moz-tree-image(Name, status) {
  list-style-image:
url("chrome://communicator/skin/bookmarks/bookmark-item-updated.gif");
}

That leaves the remaining problem common to both themes, which is changing the
bookmark's icon works in the personal toolbar folder (and its drop-down menus)
but not in the bookmark manager or bookmark sidebar. Both of these use the
<bookmarks-tree> binding so it could be something in
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarksTree.xml
or
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/src/nsBookmarksService.cpp
stopping the change in status getting through to the .css, but I haven't got any
further as it's beyond my jedi skills.

Reproducible: Always
Steps to Reproduce:
(Reporter)

Comment 1

15 years ago
Created attachment 148564 [details]
Bookmark notification icon to save as themes/modern/communicator/bookmarks/bookmark-item-updated.gif

Updated

15 years ago
Summary: Partial fix for "change bookmark's icon" notification → Partial fix for "change bookmark's icon" notification
Thanks for pointing this out. The gif looks fine to me.
For the trees (bug 178412, marked dependency) try using (Name, new)
Adding a new image also requires a change to the themes/modern/jar.mn file.
Ideally you would provide a cvs diff or at least a diff of your changes.
Try reading http://www.mozilla.org/hacking/ and linked pages.
Blocks: 178412
(Reporter)

Comment 3

15 years ago
I think I've worked out why it's not working. Changing 'new' to 'status'
(thanks) seems to have fixed one problem but uncovered another.

The changed .css entry above and similar lines using e.g. -moz-tree-cell still
doesn't work, but something like this works (highlights a notified row in yellow)...

treechildren::-moz-tree-row(new) {
  background-color: yellow;
}

The definition of the bookmarks tree reads like this...

http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarksTree.xml#756

756             <rule>
757               <treechildren>
758                 <treeitem uri="rdf:*">
759                   <treerow
properties="rdf:http://www.w3.org/1999/02/22-rdf-syntax-ns#type
rdf:http://home.netscape.com/NC-rdf#loading
rdf:http://home.netscape.com/WEB-rdf#status">
760                     <treecell src="rdf:http://home.netscape.com/NC-rdf#Icon"
761                               label="rdf:http://home.netscape.com/NC-rdf#Name"/>
762                     <treecell label="rdf:http://home.netscape.com/NC-rdf#URL" />

I think rdf:http://home.netscape.com/WEB-rdf#status should be either moved or
copied from the row to the cell definition so 'new' in the properties can be
picked up.

I've read some of http://www.mozilla.org/hacking , I'll need to download cygwin
and so on so it may take me a few days to get my machine set up to compile Mozilla.
Since you don't need to compile anything to fix this bug I should have pointed
you directly to PatchMaker: http://www.gerv.net/software/patch-maker/
(Reporter)

Comment 5

15 years ago
I've edited bookmarks.xml in comm.jar and it works
('properties="rdf:http://home.netscape.com/WEB-rdf#status"' in the definition
for #Name, inserted after lines 760 and 864).

Unfortunately I can't give any more time to it till Thursday... I'll look at
Patch Maker then and hopefully have something ready to check in.
OS: Windows 2000 → All
Hardware: PC → All
Summary: Partial fix for "change bookmark's icon" notification → Fix for "change bookmark's icon" notification
(Reporter)

Comment 6

15 years ago
Sorry, bookmarksTree.xml.
(Reporter)

Comment 7

15 years ago
Created attachment 149055 [details] [diff] [review]
Patch to fix "change bookmark's icon" notification in modern and classic themes
(Reporter)

Comment 8

15 years ago
Comment on attachment 149055 [details] [diff] [review]
Patch to fix "change bookmark's icon" notification in modern and classic themes

Requesting code review from neil.parkwaycc.co.uk@myrealbox.com .
Attachment #149055 - Flags: review?(neil.parkwaycc.co.uk)
(Reporter)

Updated

15 years ago
Attachment #148564 - Attachment is patch: true
Attachment #148564 - Attachment mime type: image/gif → text/plain
(Reporter)

Comment 9

15 years ago
Comment on attachment 148564 [details]
Bookmark notification icon to save as themes/modern/communicator/bookmarks/bookmark-item-updated.gif

(Oops, now I know not to mark images as patches...)
Attachment #148564 - Attachment is patch: false
(Reporter)

Updated

15 years ago
Attachment #148564 - Attachment mime type: text/plain → image/gif
Comment on attachment 149055 [details] [diff] [review]
Patch to fix "change bookmark's icon" notification in modern and classic themes

Mostly ok, but a couple of nits:

You removed the space here, which was nice.
>- 
>+
> treechildren::-moz-tree-image(Name) {
>   margin-right: 2px;
> }
>  
But removing this one would have been nice too :-)

These styles are obsolete and should be removed. Tree became tree ;-)
> .tree-cell-icon, .tree-cell-primary-icon {
>   list-style-image: inherit;
>   height: 16px;
>   max-width: 32px;   /* 32 pixel hack to get too-large favicons to show; fix when tree becomes tree */
>   max-height: 16px;
> }
Similarly in the other file.
Attachment #149055 - Flags: review?(neil.parkwaycc.co.uk) → review+
(Reporter)

Comment 11

15 years ago
Created attachment 149138 [details] [diff] [review]
Revised patch

Okay, nits picked out. :)

If this is okay then you or someone else can check it in on my behalf, I don't
have write access to CVS.
(Reporter)

Updated

15 years ago
Attachment #149055 - Attachment is obsolete: true
Comment on attachment 149138 [details] [diff] [review]
Revised patch

Dan, you'll still need superreview; there's a list at
http://www.mozilla.org/hacking/reviewers.html from which I've semi-arbitrarily
picked someone but it can be a bit of a hit-and-miss affair.
Attachment #149138 - Flags: superreview?(alecf)
Attachment #149138 - Flags: review+

Comment 13

15 years ago
Comment on attachment 149138 [details] [diff] [review]
Revised patch

sr=alecf
Attachment #149138 - Flags: superreview?(alecf) → superreview+
Fix checked in.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Comment on attachment 149138 [details] [diff] [review]
Revised patch

Low-risk patch to make bookmark notification icons work in Modern theme and
also in sidebar and manager in all themes.
Attachment #149138 - Flags: approval1.7?
Comment on attachment 149138 [details] [diff] [review]
Revised patch

a=dveditz for 1.7
Attachment #149138 - Flags: approval1.7? → approval1.7+
Fix checked in to the 1.7 branch.
Keywords: fixed1.7
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.