31623 - Location of Bookmarks file cannot be changed

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P2)

Description

[beryan]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; N; NT; en-US) Mozilla/m13
BuildID:    2000022820

I'd like to, and I think it would be very useful, to change the location
of the bookmark file to a file other than the profile directory.
eg, UNC, web page, other browser's bookmark.
I know I can do it in NS, I looked in moz's prefs.js and the bookmark
location is not stored in there - I don't think it dhould be hard coded.
I may be wrong about all this, could someone plz check it out?
thx



Reproducible: Always
Steps to Reproduce:
1. Try to define your own location for the bookmark file
2.
3.

Comment 1

[Gervase Markham [:gerv]]

Confirming; at least, I can't see how to do it. And there should be UI.

Gerv

Comment 2

[leger]

Putting on [nsbeta2-] radar. Not critical to beta2.

Comment 3

[don]

Move to M20 target milestone.

Comment 4

[Gervase Markham [:gerv]]

Nominating for nsbeta3. NS 6 cannot ship without this - it's 4xp, for a start. 

Gerv

Comment 5

[Claudius Gayle]

reporter or Gerv, could somebody please clarify here what exactly the desired feature is? What would be
the steps in 4.x? I seem to be missing something here and I just don't get it.

Comment 6

[Gervase Markham [:gerv]]

The bookmarks file for a given profile is (under Mozilla) stored in 
Users50\profilename\bookmarks.html (or similar). There is a way in NS 4.x (Save 
Bookmarks As...) which allows you to change the path where your bookmarks for 
your profile are stored - if you share them between Windows and Linux versions 
of Navigator, or store them somewhere network-accessible, for example. 

A simple pref to set the bookmarks file path would do the trick. It would be 
nice to have UI, but not essential :-)

Gerv

Comment 7

[Claudius Gayle]

I see now, thanks. It's definitely 4xp, but may I suggest adding some more justification if you truly believe this to be nsbeta2 or 
even 3 and not just an RFE.

Comment 8

[Gervase Markham [:gerv]]

The justification is that there are many people who want to store their 
bookmarks somewhere else - to back them up, to share them with another install, 
or whatever. It's a very useful feature for a variety of users, and it's 4xp, 
and so it should be implemented :-) 

Also, it probably wouldn't be too hard - when you load and save bookmarks, 
replace the hard-coded relative URL with a call to find a pref.

Gerv

Comment 9

[beryan]

re: gerv's notes,
definitely. it's the single barrier preventing me from migrating to mozilla.
I can't rely on mozilla yet, so I'd like to use both, mozilla for non-time 
critical browsing, and use netscape when I can't afford a crash or other thing 
stuffing me up. the problem is bookmarks - with a 150kb bookmark file (those 
are just the ones I haven't memorised) and links to all my important sites for 
work, I need to seamlessly access my store of bookmarks from both browsers. I 
can't speak for anyone else, but I know I find this a notable omission.
Others would encounter problems in network installs, shared bookmark 
environments, remote bookmark storage, etc. Just adds flexibility to a browser 
that needs all the flexibility it can get to thrash MSIE.. and besides, NS and 
(i think) MSIE have variables storing the bookmark path, so there :)
thx!

Comment 10

[verah (gone)]

Nav triage team: [nsbeta3-]; changed summary to be more accurate

Comment 11

[Blake Ross]

Reassigning 79 Bookmarks bugs to Ben.  I was told this was going to be done 
shortly about two months ago, but it clearly hasn't been.  I think that's long 
enough for all these bugs to remain assigned to nobody.

Feel free to filter all this spam into the trashcan by looking for this string 
in the message body: ducksgoquack

Comment 12

[Gervase Markham [:gerv]]

Nominating for mozilla1.0. This is very useful for people who run more than one 
OS.

Gerv

Comment 13

[Paul Chen]

nav triage team:

not a beta stopper, though we wouldn't mind a patch from the net ;-)

Comment 14

[Gervase Markham [:gerv]]

You what? I am now _highly_ confused. nsbeta_1_?

Gerv

Comment 15

[Asa Dotzler [:asa]]

adding helpwanted keyword, cleaning up status whiteboard.

Comment 16

[Henrik Gemal]

First go at fixing this one could be to add "bookmarks.html"s location to the 
prefs.js file. This way to mozilla profiles could share bookmarks.

Comment 17

[timeless]

*** Bug 70390 has been marked as a duplicate of this bug. ***

Comment 18

[Paul Chen]

Marking nsbeta1- bugs as future to get off the radar.

Comment 19

[Gervase Markham [:gerv]]

I could probably do this in several hacky ways - but then my patch would get 
rejected. Is there any chance of someone familiar with the bookmarks code giving 
me some idea of at what point in retrieving the bookmarks filename this should 
be implemented? We are in the area of
http://lxr.mozilla.org/mozilla/source/xpfe/components/bookmarks/src/nsBookmarksS
ervice.cpp#3971

waterson: seems you originally wrote this code. Any chance of a tip? 

Gerv

Comment 20

[home]

Actually, I wonder if it is possible to extend this functionality so that
bookmarks can be saved to two separate file at the same time. The reason for
this is that if you open bookmarks in NS4.x, and then close NS, the file
structure is preserved, but the ShortCutURL="?" tags are all lost. I think it is
important to be able to specify a shared bookmark file, but to effectively lose
the new shortcut functionality would be too high a price to pay (better to just
have better export functionality).

Simpler, perhaps two bookmark menu items "Export(default)" and "Export..." would
be the answer.

Comment 21

[timeless]

Excuse me? export has nothing to do with this bug, if you have concerns about 
it please file a new bug.

3978 nsBookmarksService::GetBookmarksFile(nsFileSpec* aResult)

change it so that it gets a preference (preferably the same one nc4 used) and 
then loads that file (w/ a fallback to the current behavior)

Comment 22

[Gervase Markham [:gerv]]

Timeless: the problem with that is that if there is ever UI for this, it makes 
it difficult to display the "default" bookmarks location just by getting the 
pref, and also difficult to unset the pref if the user puts the bookmarks back 
in the default place. Or not?

Gerv

Comment 23

[Gervase Markham [:gerv]]

Attachment: patch

Comment 24

[Gervase Markham [:gerv]]

Ok, people - user-defined bookmarks location is a reality! This is definitely a
much-requested feature. Patch attached. It uses the old 4.x pref of
browser.bookmarks_file for max. compatibility when migrating prefs.

Clearing nsbeta1- nomination in view of this - hoping that, if you don't have
resource to do the patch, you have resource to review it and check it in :-) 

Looking for r= and sr=.

Gerv

Comment 25

[Gervase Markham [:gerv]]

By the way, [e-mail] does have a point - if what he says is true, 
sharing bookmarks with NS 4.x will be incompatible with any new bookmarks 
features - most notably, the Personal Keywords feature - because NS 4.x will eat 
the attributes.

Gerv

Comment 26

[timeless]

For now we could include a warning in profile migration, but that's beyond the 
scope of this bug, as is the fact that the code in bookmarks is using char* 
instead of something else. (i'll test the code and give you an r= instead of 
complaining about preexisting style).

Comment 27

[timeless]

r=timeless

Comment 28

[Peter "jag" Annema]

+  if (NS_SUCCEEDED(rv) && (prefServ)) {
+    char *prefVal = nsnull;
+    
+    if (NS_SUCCEEDED(rv = prefServ->CopyCharPref("browser.bookmark_file",
&prefVal)) && 
+        prefVal) {
+       *aResult = (const char *)prefVal;
+    }
+    else {
+      // Pref is not present
+      rv = NS_ERROR_FAILURE;
+    }
+  }

You could replace that with:

+  if (NS_SUCCEEDED(rv)) {
+    nsXPIDLCString prefVal;
+    rv = prefServ->CopyCharPref("browser.bookmark_file",
+                                getter_Copies(prefVal));
+    if (NS_SUCCEEDED(rv)) {
+      *aResult = prefVal;
+    }
+  }

This makes setting a breakpoint on the if (but after the CopyCharPref) easier.
In the original code you were leaking the char* prefVal. CopyCharPref hands you
a char* which you own and need to free. nsXPIDLCString and getter_Copies take
care of that for you.

This makes the assumption that neither prefServ nor prefVal will ever be nsnull
if rv indicates success. I think it's a fair assumption, some people don't. If
you want to test for nsnull, make sure you set rv to NS_ERROR_FAILURE in both
the else cases (you cover only one in your patch). However, that throws away the
rv value if !NS_SUCCEEDED(rv) so you should really split up that if and only set
rv to NS_ERROR_FAILURE on the elses of |if (prefServ)| and |if (prefVal)|


+  if(NS_FAILED(rv)) {

Nit: keep the same space-after-if style as for: if (NS_SUCCEEDED(rv) && ... )
(Or remove the space there too, just be consistent...)

+       // or nsILocalFile, this conversion from nsiFile to

Minor case nit

+       nsXPIDLCString pathBuf;
+       rv = bookmarksFile->GetPath(getter_Copies(pathBuf));
+       if (NS_SUCCEEDED(rv))
+           *aResult = (const char *)pathBuf;

I think you can drop the (const char *) here... See above :-) If not, use:
  *aResult = pathBuf.get();

You seem to be mixing 2 and 4 space indent. And the file is mostly 8 space, as
also confirmed by the emacs mode line. Ugh! If Ben doesn't mind you owning that
code, could you fix that to all be 8 space (or with his permission, all 4 or 2
space)?

Comment 29

[Chris Waterson]

We should remove the nsFileSpec usage. This makes me want to throw up:

+  if (NS_SUCCEEDED(rv)) {
+    nsXPIDLCString prefVal;
+    rv = prefServ->CopyCharPref("browser.bookmark_file",
+                                getter_Copies(prefVal));
+    if (NS_SUCCEEDED(rv)) {
+      *aResult = prefVal;

Surprise! There's an *assignment* operator that constructs an nsFileSpec from a 
const char*! This is just way too clever.

(N.B., I'm not criticizing the author of the patch: you did what you had to do. 
The nsFileSpec APIs are what blow choad.) How much extra work would it be to 
replace this stuff with nsIFile and friends?

Comment 30

[Peter "jag" Annema]

*LOL*

I ranted about that in #mozilla to Gerv too. I'll look into what needs to be
done to move this all to nsIFile, though I suspect it to be non-trivial...

Comment 31

[Gervase Markham [:gerv]]

Well, what I'd hope you'd allow is to check it in as-is (with other review 
changes made.) Then, when it all gets moved to nsIFile, this will go with it...

Moving Bookmarks to nsIFile is a long-term job - bug 36974, it would seem. This 
is futured, and is blocked by the fact that Bookmarks use nsInputFileStream, 
which doesn't work with anything apart from nsIFileSpec. That's bug 46394 which, 
amusingly, is titled "necko file streams should be moved from necko to 
xpcom" and marked helpwanted. There has been much argument, and DougT concludes 
with "not going to happen soon". :-(

In the mean time, this patch provides much-requested functionality ;-)

Gerv

Comment 32

[Gervase Markham [:gerv]]

Attachment: Patch 2 - incorporating review comments

Comment 33

[Chris Waterson]

I don't think this comment is correct anymore:

+               // Otherwise, we look for bookmarks.html in the current profile
+               // directory. This is as convoluted as it seems because we
+               // want to 1) not break viewer (which has no profiles), and 2)
+               // still deal reasonably (in the short term) when no
+               // bookmarks.html is installed in the profile directory.

We just rely on the magic directory service to find the bookmarks, right? Fix 
the comment, or explain why it's still appropriate, and sr=waterson.

Also, find and/or file a bug on converting nsBookmarksService.cpp to using 
nsIFile instead of nsFileSpec, and note that bug# here.

thanks!

Comment 34

[Gervase Markham [:gerv]]

The conversion bug is bug bug 36974, blocked by (futured, helpwanted) bug 46394.

The comment isn't mine :-) But I'll try and find out what it should say.

Would:
// Otherwise, we look for bookmarks.html in the current profile
// directory using the magic directory service.
do? :-)

Gerv

Comment 35

[Chris Waterson]

Wonderful!

Comment 36

[Gervase Markham [:gerv]]

Attachment: Patch 3 - r=timeless

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just pulled a tree that had this checked in (there weren't any comments above,
but it was checked in by timeless%mac.com at 2001-04-20 14:14 PDT).

Since my profile was imported from 4.x, it had a vestigial pref from 4.x
pointing to my 4.x bookmarks file (which is pretty much empty).  When I pulled
Mozilla with this patch, Mozilla suddenly stopped using my bookmarks and started
using the ones I had in 4.x (practically none).

Maybe we should name the pref something different from what it was in 4.x so
this doesn't happen?  I tend to think we shouldn't share a bookmarks file with
4.x by default since we have otherwise separate profiles.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do we want to change this for 0.9?  I wouldn't be surprised if we see lots of
"my bookmarks disappeared" comments if we don't.

Comment 39

[Chris Waterson]

Probably not. timeless, please back this out and we'll get it in for 
mozilla-0.9.1.

Comment 40

[Gervase Markham [:gerv]]

He's away for the weekend. He asked me to sort out a backout if necessary. I'll 
get on IRC and do it when everyone wakes up.

Is there not some sort of third solution here? The reason for having the pref 
name the same is so that migration Does The Right Thing, in line with all the 
user's other 4.x prefs. (Note that this pref will only be set if the user set up 
a non-default bookmarks file, which is a small minority of users.)

If this pref is not going to be auto-migrated with the rest (i.e. we go out of 
our way to break this) we need to explain why we've honoured the rest of the 
user's 4.x prefs and not this one.

To look at it another way, if the user's 4.x bookmarks are in their profile 
directory, then they get them in Mozilla. Why should they not get them if they 
happen to be elsewhere?

Gerv

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I never set up a non-default bookmarks file in 4.x.  Are you sure that pref
isn't set all the time, at least on the Unix version?

Comment 42

[Christopher Blizzard (:blizzard)]

Gerv, please back this out for 0.9.

Comment 43

[tor]

Not only is mozilla suddenly looking at an old set of bookmarks, but when
I run 4.x and mozilla simultaneously 4.x notices that the bookmark file
is being periodically written and keeps popping up dialogs stating
"Bookmarks have changed on disk and are being reloaded."

This should be backed out for now.

Comment 44

[Gervase Markham [:gerv]]

Hurricane has backed this out. 

I noticed the 4.x messages - I thought that
a) It won't be seen by many users as very few run two browsers at once
b) Why on earth are we periodically writing the bookmarks in the first place?

Unless we fix Moz to be more intelligent about this (which we should) then this 
is a problem to be expected if you have two apps using the same file. It's not 
specific to this fix.

<rant>Why on earth does NS 4.x not silently reload bookmarks which have changed 
if it hasn't changed them since it loaded them last?</rant>

As regards when the pref is set, I think it is set by using the "Save As..." 
command in the File menu of the Bookmarks Manager. So, I suppose that if you do 
a Save As..., even if you save on top of your bookmarks file in the prefs, the 
pref will then be set. 

Possible solutions to this:
a) Leave it as-is, because it Does The Right Thing for most people.
b) Rename the pref. This means that the user who upgrades from NS4 to 
Mozilla and wants to continue using the same bookmarks (a reasonable request) 
has to edit prefs.js to set that up as we currently don't have any UI for 
setting it. It breaks migration.
c) Some funky migration thang where it says "You are currently using file X for 
your NS 4 bookmarks. Do you want to keep using it?"

Any other ideas?

Gerv

Comment 45

[tor]

Backed out for real this time.

Comment 46

[Brendan Eich [:brendan]]

How did timeless check in without driver approval?

/be

Comment 47

[Gervase Markham [:gerv]]

I got a=asa on IRC. I don't have a log, though. I sort-of assumed a comment 
would appear here. The checkin comment correctly records his approval.

Gerv

Comment 48

[timeless]

Ok, filed:
Bugzilla Bug 77133 Why on earth are we periodically writing the bookmarks file

I suggest gerv or someone write a newsgroup post asking for ideas about how to 
deal with multiple applications contending for the bookmarks file.

Well.. whenever we do implement this feature and its migration behavior, we'll 
have to relnote it.

Does mozilla have a first run property? netscape4 used to have one [it's a 
js preference setting] which we used to see if you had ever accepted the 
license for the current (or later) version.  If we tagged mozilla preferences 
and checked to see if the gecko engine is newer than the specific build where 
this feature was added... We can prompt once (the first time a user runs with 
this new feature), and afterwards the bookmarks preference will be set 
correctly so there will be no concern.

I'm sure no one likes this solution, but there it is.

Comment 49

[Gervase Markham [:gerv]]

Having discussed this on IRC with shaver, we may well be writing last-visited 
information or something like that. This is not necessarily the right thing to 
do, but it may be why it's happening.

The problem we have here is that 4.x gives an (annoying and pointless) dialog 
_even_ if you haven't changed the bookmarks with 4.x. It says "Bookmarks have 
changed on disk and are being reloaded. [OK]." Who on earth thought of that? 
Therefore, there is no way of getting 4.x to play nice with simultaneous 
bookmark file sharing. 

However, this is not the point of this bug, really. Surely the most common use 
case is for people to run 2 Mozillas on different platforms, or Mozilla at one 
time and NS 4.x at the other. In this case, there is no problem, and this is a 
useful feature. So, we could change the name of the pref. - but then that 
removes the convenience of the thing. <sigh>

Gerv

Comment 50

[Gervase Markham [:gerv]]

tor, dbaron, brendan: 

If we change the pref name, can we still have this for 0.9? That way, users have
to set it up for themselves by editing prefs.js, but at least the feature is
_there_. 

My crystal ball tells me that some Mozilla contributors will make releases from
the 0.9 branch, and this is a much-requested feature. Changing the pref name
means that it won't clash and unexpected things won't happen (as dbaron and tor
reported.)

Gerv

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems reasonable to do for 0.9.1 but I'm not sure whether it's worth pushing for
0.9 at this point.  (Sorry for not responding sooner.)

Comment 52

[Gervase Markham [:gerv]]

So, what are we doing about this for 0.9.1?

I really want this capability to get in for that milestone in some form, even if
it does involve the user editing their prefs.js (although I had a go at
providing UI for it a while back - I might be able to resurrect that. However,
it's a different bug.) 

If there are going to be a significant number of people running the Netscape 4
and Mozilla alongside one another at the same time, then using the same pref
name and auto-migrating it will cause them problems, because the two apps will
contend for the shared bookmark file, and we can't patch 4.x to be smart about
it.

If there's nothing we can do about that in a 0.9.1 timeframe, I'd rather change
the name of the pref and just check the code in. Anyone got a good name?
browser.bookmarks.file?

Gerv

Comment 53

[Gervase Markham [:gerv]]

Attachment: Patch using browser.bookmarks.file as pref name

Comment 54

[Gervase Markham [:gerv]]

The above patch has no changes but the pref name. This patch already has
r=timeless and sr=waterson (it doesn't say that explicitly here, but he did give
it :-) So, it just needs checking in.

Gerv

Comment 55

[Gervase Markham [:gerv]]

Checked in.

Gerv

Comment 56

[Chad Austin]

tested and verified

Comment 57

[Stephen Hassard]

is it possible to redirect mozilla to a URL for getting it's bookmark file? I 
have a web based database format that can output in mozilla compatible 
bookmarks file and would love to have mozilla use this file as it's default 
bookmark file.

Comment 58

[Gervase Markham [:gerv]]

Have you actually tried setting the pref to a URL?

Gerv

Comment 59

[Stephen Hassard]

yeah, and my bookmarks menu is blank. I don't seem to get any visible error 
though ..

Comment 60

[Gervase Markham [:gerv]]

Then it's not possible :-)

As I remember, when I implemented this we thought about it but doing it meant
migrating the bookmarks code from nsIFileSpec to nsIFile (or the other way) so
it would transparently support URLs, and that was hairy and horrible.

Gerv

Comment 61

[Gilles Durys]

*** Bug 147531 has been marked as a duplicate of this bug. ***