-
-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[jarun#711] fixed profile detection for multiple Firefox installs #716
Conversation
profiles = firefox_profile or get_firefox_profile_names(default_ff_folder) | ||
if profiles: | ||
ff_bm_db_paths = {s: '~/.mozilla/firefox/{}/places.sqlite'.format(s) | ||
for s in profiles} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keys are printed in prompt (when more than 1 profiles were found).
Dicts retain insertion order (since Python 3.7).
1ff6ed9
to
b7ce595
Compare
if not os.path.exists(bookmarks_database): | ||
raise FileNotFoundError | ||
self.load_chrome_database(bookmarks_database, newtag, add_parent_folder_as_tag) | ||
if os.path.isfile(os.path.expanduser(gc_bm_db_path)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a file existence check to avoid asking about nonexistent bookmark files.
(expanduser()
is required to handle ~
in paths)
if self.chatty: | ||
profile = ('' if len(ff_bm_db_paths) < 2 else | ||
f' profile {name} [{idx}/{len(ff_bm_db_paths)}]') | ||
resp = input(f'Import bookmarks from Firefox{profile}? (y/n): ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking which profile to import from (when multiple were detected).
if not os.path.exists(bookmarks_database): | ||
raise FileNotFoundError | ||
self.load_firefox_database(bookmarks_database, newtag, add_parent_folder_as_tag) | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one Firefox profile is imported, others are skipped (this means falling back to original behaviour in library mode).
except NoOptionError: | ||
pass | ||
if profiles: | ||
return profiles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When any installs are found, their default profiles are returned, in order.
|
||
profiles_names = [section for section in config.sections() if section.startswith('Profile')] | ||
if not profiles_names: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a noop anyway.
profile_path = config.get(name, 'path') | ||
return profile_path | ||
profiles += [config.get(name, 'path')] | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not adding the same profile twice
except NoOptionError: | ||
pass | ||
try: | ||
# alternative way to detect default profile | ||
if config.get(name, 'name').lower() == "default": | ||
profile_path = config.get(name, 'path') | ||
return profile_path | ||
profiles += [config.get(name, 'path')] | ||
except NoOptionError: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When no installs were found, all profiles detected as "default" are returned.
|
||
# There are no default profiles | ||
LOGDBG('get_firefox_profile_names(): {} does not exist'.format(path)) | ||
return profiles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty list is both valid and falsey.
@@ -2304,7 +2306,7 @@ class BukuDb: | |||
|
|||
for item in sublist: | |||
if item['type'] == 'folder': | |||
next_folder_name = folder_name + ',' + item['name'] | |||
next_folder_name = folder_name + DELIM + strip_delim(item['name']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stripping commas from imported bookmark folder tags
@@ -5986,7 +6002,7 @@ POSITIONAL ARGUMENTS: | |||
|
|||
# Import bookmarks from browser | |||
if args.ai: | |||
bdb.auto_import_from_browser() | |||
bdb.auto_import_from_browser(firefox_profile=os.environ.get('FIREFOX_PROFILE')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overriding Firefox profile detection with an environment variable (if found).
b7ce595
to
a4accd2
Compare
<DL><p> | ||
<DT><A HREF="http://example.com/"></A>""" | ||
exp_res = ("http://example.com/", None, ",1s,", None, 0, True, False) | ||
exp_res = ("http://example.com/", None, ",1s (blah blah),", None, 0, True, False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensuring that a bookmark folder with commas won't be split into multiple tags
@@ -287,7 +287,7 @@ | |||
"date_added": "13149362306507580", | |||
"date_modified": "13149362306507581", | |||
"id": "41", | |||
"name": "Imported From Firefox", | |||
"name": "Imported From Firefox (2011-09-02, 06:03:50)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actual example of a comma in a bookmark folder name.
if profile: | ||
ff_bm_db_path = ('~/Library/Application Support/Firefox/' | ||
'{}/places.sqlite'.format(profile)) | ||
profiles = firefox_profile or get_firefox_profile_names(default_ff_folder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only Firefox? Tomorrow another users may request the same for another browser. Why not do it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK no other browser has multiple switchable profiles that the import code has to pick one from (and may do so incorrectly).
For all other browsers, the bookmark files are always in fixed location – that is certainly the case according to buku code.
Thank you! |
@LeXofLeviafan can we make a release? |
@jarun I was thinking of dealing with #702 first – it's been open for a while, come to think of it. Also, you'll need to fork (and release on PyPI) the package that's been blocking buku from releasing updates on PyPI (it should be maintained by you to avoid repeating the issue). …Or alternatively its code could be included in bukuserver files – thus removing the external dependency altogether. Additionally, the fix could also be applied to v4.8 (e.g. in a separate branch) to publish a post-release to PyPI as well (i.e. |
Sure thing!
This works for me. Please raise the PRs. |
Would you be able to make the next release? I can add you to Pypi. |
@LeXofLeviafan tagging in case you have missed my responses above. |
fixes #711:
FIREFOX_PROFILE
) in CLI & parameter (firefox_profile
) in APIalso: