243679 - Fix for "change bookmark's icon" notification

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Dan]

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:

Comment 1

[Dan]

Attachment: Bookmark notification icon to save as themes/modern/communicator/bookmarks/bookmark-item-updated.gif

Comment 2

[[e-mail]]

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.

Comment 3

[Dan]

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.

Comment 4

[[e-mail]]

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/

Comment 5

[Dan]

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.

Comment 6

[Dan]

Sorry, bookmarksTree.xml.

Comment 7

[Dan]

Attachment: Patch to fix "change bookmark's icon" notification in modern and classic themes

Comment 8

[Dan]

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

Requesting code review from [e-mail] .

Comment 9

[Dan]

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...)

Comment 10

[[e-mail]]

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.

Comment 11

[Dan]

Attachment: 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.

Comment 12

[[e-mail]]

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.

Comment 13

[Alec Flett]

Comment on attachment 149138 [details] [diff] [review]
Revised patch

sr=alecf

Comment 14

[[e-mail]]

Fix checked in.

Comment 15

[[e-mail]]

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.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 149138 [details] [diff] [review]
Revised patch

a=dveditz for 1.7

Comment 17

[[e-mail]]

Fix checked in to the 1.7 branch.