Closed
Bug 243679
Opened 21 years ago
Closed 21 years ago
Fix for "change bookmark's icon" notification
Categories
(SeaMonkey :: Bookmarks & History, defect)
SeaMonkey
Bookmarks & History
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: dc2, Assigned: p_ch)
References
Details
(Keywords: fixed1.7)
Attachments
(2 files, 1 obsolete file)
Bookmark notification icon to save as themes/modern/communicator/bookmarks/bookmark-item-updated.gif
207 bytes,
image/gif
|
Details | |
11.39 KB,
patch
|
neil
:
review+
alecf
:
superreview+
dveditz
:
approval1.7+
|
Details | Diff | Splinter Review |
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:
Updated•21 years ago
|
Summary: Partial fix for "change bookmark's icon" notification → Partial fix for "change bookmark's icon" notification
Comment 2•21 years ago
|
||
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
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•21 years ago
|
||
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/
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
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)
Attachment #148564 -
Attachment is patch: true
Attachment #148564 -
Attachment mime type: image/gif → text/plain
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
Attachment #148564 -
Attachment mime type: text/plain → image/gif
Comment 10•21 years ago
|
||
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•21 years ago
|
||
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.
Attachment #149055 -
Attachment is obsolete: true
Comment 12•21 years ago
|
||
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•21 years ago
|
||
Comment on attachment 149138 [details] [diff] [review]
Revised patch
sr=alecf
Attachment #149138 -
Flags: superreview?(alecf) → superreview+
Comment 14•21 years ago
|
||
Fix checked in.
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 15•21 years ago
|
||
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 16•21 years ago
|
||
Comment on attachment 149138 [details] [diff] [review]
Revised patch
a=dveditz for 1.7
Attachment #149138 -
Flags: approval1.7? → approval1.7+
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•