From b3a637834bf7181276e4dcd1c6c9231a85b614dc Mon Sep 17 00:00:00 2001 From: Wojciech Siewierski Date: Sat, 26 Jan 2019 00:22:10 +0100 Subject: container.bookmarks: Remove invalid bookmarks only on access Previously ranger was validating the bookmarks (whether they are existing directories) when loading the bookmark file. It caused the remote filesystems like autofs to wake up unnecessarily and in general caused more problems than solved. Fixes #1365. --- ranger/container/bookmarks.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ranger/container/bookmarks.py b/ranger/container/bookmarks.py index cbfc541a..e1a88fa2 100644 --- a/ranger/container/bookmarks.py +++ b/ranger/container/bookmarks.py @@ -88,7 +88,12 @@ class Bookmarks(FileManagerAware): if key == '`': key = "'" if key in self.dct: - return self.dct[key] + value = self.dct[key] + if os.path.isdir(value.path): + return value + else: + del self[key] + raise KeyError("Invalid Bookmark: `%s'!" % key) else: raise KeyError("Nonexistant Bookmark: `%s'!" % key) @@ -229,7 +234,7 @@ class Bookmarks(FileManagerAware): for line in fobj: if self.load_pattern.match(line): key, value = line[0], line[2:-1] - if key in ALLOWED_KEYS and not os.path.isfile(value): + if key in ALLOWED_KEYS: dct[key] = self.bookmarktype(value) fobj.close() return dct -- cgit 1.4.1-2-gfad0 From 981945a9afe4cca3b2e93ee0a57a22460b68f794 Mon Sep 17 00:00:00 2001 From: Wojciech Siewierski Date: Sat, 26 Jan 2019 22:54:22 +0100 Subject: container.bookmarks: Don't remove invalid bookmarks at all Non-existent bookmarks may be only temporarily unreachable (eg. pendrive, NFS), no point in removing them. --- ranger/container/bookmarks.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ranger/container/bookmarks.py b/ranger/container/bookmarks.py index e1a88fa2..20809c10 100644 --- a/ranger/container/bookmarks.py +++ b/ranger/container/bookmarks.py @@ -92,8 +92,7 @@ class Bookmarks(FileManagerAware): if os.path.isdir(value.path): return value else: - del self[key] - raise KeyError("Invalid Bookmark: `%s'!" % key) + raise KeyError("Cannot open bookmark: `%s'!" % key) else: raise KeyError("Nonexistant Bookmark: `%s'!" % key) -- cgit 1.4.1-2-gfad0 From 8374ae53f1604f887255bd6214aae2b40b03c6f8 Mon Sep 17 00:00:00 2001 From: Wojciech Siewierski Date: Sun, 27 Jan 2019 02:46:25 +0100 Subject: Make it easier to test bookmarks by optionally disabling validation The test bookmarks were intentionally bogus as we cannot reliably predict valid paths on the test system, so validation doesn't make any sense there. --- ranger/container/bookmarks.py | 7 ++++--- tests/ranger/container/test_bookmarks.py | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/ranger/container/bookmarks.py b/ranger/container/bookmarks.py index 20809c10..25e81097 100644 --- a/ranger/container/bookmarks.py +++ b/ranger/container/bookmarks.py @@ -12,7 +12,7 @@ from ranger.core.shared import FileManagerAware ALLOWED_KEYS = string.ascii_letters + string.digits + "`'" -class Bookmarks(FileManagerAware): +class Bookmarks(FileManagerAware): # pylint: disable=too-many-instance-attributes """Bookmarks is a container which associates keys with bookmarks. A key is a string with: len(key) == 1 and key in ALLOWED_KEYS. @@ -29,7 +29,7 @@ class Bookmarks(FileManagerAware): load_pattern = re.compile(r"^[\d\w']:.") def __init__(self, bookmarkfile, bookmarktype=str, autosave=False, - nonpersistent_bookmarks=()): + validate=True, nonpersistent_bookmarks=()): """Initializes Bookmarks. specifies the path to the file where @@ -41,6 +41,7 @@ class Bookmarks(FileManagerAware): self.path = bookmarkfile self.bookmarktype = bookmarktype self.nonpersistent_bookmarks = set(nonpersistent_bookmarks) + self.validate = validate def load(self): """Load the bookmarks from path/bookmarks""" @@ -89,7 +90,7 @@ class Bookmarks(FileManagerAware): key = "'" if key in self.dct: value = self.dct[key] - if os.path.isdir(value.path): + if not self.validate or os.path.isdir(str(value)): return value else: raise KeyError("Cannot open bookmark: `%s'!" % key) diff --git a/tests/ranger/container/test_bookmarks.py b/tests/ranger/container/test_bookmarks.py index 64192c06..8c62bd23 100644 --- a/tests/ranger/container/test_bookmarks.py +++ b/tests/ranger/container/test_bookmarks.py @@ -12,7 +12,7 @@ def testbookmarks(tmpdir): # Bookmarks point to directory location and allow fast access to # 'favorite' directories. They are persisted to a bookmark file, plain text. bookmarkfile = tmpdir.join("bookmarkfile") - bmstore = Bookmarks(str(bookmarkfile)) + bmstore = Bookmarks(str(bookmarkfile), validate=False) # loading an empty bookmark file doesnot crash bmstore.load() @@ -33,7 +33,7 @@ def testbookmarks(tmpdir): # We can persist bookmarks to disk and restore them from disk bmstore.save() - secondstore = Bookmarks(str(bookmarkfile)) + secondstore = Bookmarks(str(bookmarkfile), validate=False) secondstore.load() assert "'" in secondstore assert secondstore["'"] == "the milk" -- cgit 1.4.1-2-gfad0 From 2c83766eec99bbcb577de78c8ebc39c83ab15b57 Mon Sep 17 00:00:00 2001 From: Wojciech Siewierski Date: Sun, 27 Jan 2019 03:02:23 +0100 Subject: Refactor the bookmark validation code Now it's trivial for the test module to mock the bookmark validation without introducing too much logic about this mocking to the actual class. --- ranger/container/bookmarks.py | 10 ++++++---- tests/ranger/container/test_bookmarks.py | 9 +++++++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/ranger/container/bookmarks.py b/ranger/container/bookmarks.py index 25e81097..7b5e56b7 100644 --- a/ranger/container/bookmarks.py +++ b/ranger/container/bookmarks.py @@ -12,7 +12,7 @@ from ranger.core.shared import FileManagerAware ALLOWED_KEYS = string.ascii_letters + string.digits + "`'" -class Bookmarks(FileManagerAware): # pylint: disable=too-many-instance-attributes +class Bookmarks(FileManagerAware): """Bookmarks is a container which associates keys with bookmarks. A key is a string with: len(key) == 1 and key in ALLOWED_KEYS. @@ -29,7 +29,7 @@ class Bookmarks(FileManagerAware): # pylint: disable=too-many-instance-attribut load_pattern = re.compile(r"^[\d\w']:.") def __init__(self, bookmarkfile, bookmarktype=str, autosave=False, - validate=True, nonpersistent_bookmarks=()): + nonpersistent_bookmarks=()): """Initializes Bookmarks. specifies the path to the file where @@ -41,7 +41,6 @@ class Bookmarks(FileManagerAware): # pylint: disable=too-many-instance-attribut self.path = bookmarkfile self.bookmarktype = bookmarktype self.nonpersistent_bookmarks = set(nonpersistent_bookmarks) - self.validate = validate def load(self): """Load the bookmarks from path/bookmarks""" @@ -90,7 +89,7 @@ class Bookmarks(FileManagerAware): # pylint: disable=too-many-instance-attribut key = "'" if key in self.dct: value = self.dct[key] - if not self.validate or os.path.isdir(str(value)): + if self._validate(value): return value else: raise KeyError("Cannot open bookmark: `%s'!" % key) @@ -258,3 +257,6 @@ class Bookmarks(FileManagerAware): # pylint: disable=too-many-instance-attribut def _update_mtime(self): self.last_mtime = self._get_mtime() + + def _validate(self, value): # pylint: disable=no-self-use + return os.path.isdir(str(value)) diff --git a/tests/ranger/container/test_bookmarks.py b/tests/ranger/container/test_bookmarks.py index 8c62bd23..84ac42c2 100644 --- a/tests/ranger/container/test_bookmarks.py +++ b/tests/ranger/container/test_bookmarks.py @@ -8,11 +8,16 @@ import pytest from ranger.container.bookmarks import Bookmarks +class NotValidatedBookmarks(Bookmarks): + def _validate(self, value): + return True + + def testbookmarks(tmpdir): # Bookmarks point to directory location and allow fast access to # 'favorite' directories. They are persisted to a bookmark file, plain text. bookmarkfile = tmpdir.join("bookmarkfile") - bmstore = Bookmarks(str(bookmarkfile), validate=False) + bmstore = NotValidatedBookmarks(str(bookmarkfile)) # loading an empty bookmark file doesnot crash bmstore.load() @@ -33,7 +38,7 @@ def testbookmarks(tmpdir): # We can persist bookmarks to disk and restore them from disk bmstore.save() - secondstore = Bookmarks(str(bookmarkfile), validate=False) + secondstore = NotValidatedBookmarks(str(bookmarkfile)) secondstore.load() assert "'" in secondstore assert secondstore["'"] == "the milk" -- cgit 1.4.1-2-gfad0