From 28842c90b606eb0300add8cbd2a03cd4ceb8349b Mon Sep 17 00:00:00 2001 From: Marshall Lochbaum Date: Fri, 22 Jul 2016 22:46:30 -0400 Subject: [PATCH 1/3] Add --toggle flag to bookmark-add (fixes #1667) --- qutebrowser/browser/commands.py | 10 +++++++--- qutebrowser/browser/urlmarks.py | 14 ++++++++++++-- tests/end2end/features/urlmarks.feature | 6 ++++++ 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 7838e0138..f86faa0da 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1120,7 +1120,8 @@ class CommandDispatcher: raise cmdexc.CommandError("Quickmark '{}' not found!".format(name)) @cmdutils.register(instance='command-dispatcher', scope='window') - def bookmark_add(self, url=None, title=None): + @cmdutils.argument('toggle', flag='t') + def bookmark_add(self, url=None, title=None, toggle=False): """Save the current page as a bookmark, or a specific url. If no url and title are provided, then save the current page as a @@ -1134,6 +1135,8 @@ class CommandDispatcher: Args: url: url to save as a bookmark. If None, use url of current page. title: title of the new bookmark. + toggle: remove the bookmark instead of raising an error if it + already exists. """ if url and not title: raise cmdexc.CommandError('Title must be provided if url has ' @@ -1149,12 +1152,13 @@ class CommandDispatcher: if not title: title = self._current_title() try: - bookmark_manager.add(url, title) + if_added = bookmark_manager.add(url, title, toggle) except urlmarks.Error as e: raise cmdexc.CommandError(str(e)) else: + mes = "Bookmarked {}!" if if_added else "Removed bookmark {}!" message.info(self._win_id, - "Bookmarked {}!".format(url.toDisplayString())) + mes.format(url.toDisplayString())) @cmdutils.register(instance='command-dispatcher', scope='window', maxsplit=0) diff --git a/qutebrowser/browser/urlmarks.py b/qutebrowser/browser/urlmarks.py index 89bec3d9a..9889c8536 100644 --- a/qutebrowser/browser/urlmarks.py +++ b/qutebrowser/browser/urlmarks.py @@ -272,12 +272,17 @@ class BookmarkManager(UrlMarkManager): elif len(parts) == 1: self.marks[parts[0]] = '' - def add(self, url, title): + def add(self, url, title, toggle=False): """Add a new bookmark. + Return True if the bookmark was added, and False if it was + removed (which only happens if toggle is True). + Args: url: The url to add as bookmark. title: The title for the new bookmark. + toggle: remove the bookmark instead of raising an error if it + already exists. """ if not url.isValid(): errstr = urlutils.get_errstring(url) @@ -286,8 +291,13 @@ class BookmarkManager(UrlMarkManager): urlstr = url.toString(QUrl.RemovePassword | QUrl.FullyEncoded) if urlstr in self.marks: - raise AlreadyExistsError("Bookmark already exists!") + if toggle: + del self.marks[urlstr] + return False + else: + raise AlreadyExistsError("Bookmark already exists!") else: self.marks[urlstr] = title self.changed.emit() self.added.emit(title, urlstr) + return True diff --git a/tests/end2end/features/urlmarks.feature b/tests/end2end/features/urlmarks.feature index 3490cc4a0..383cb492a 100644 --- a/tests/end2end/features/urlmarks.feature +++ b/tests/end2end/features/urlmarks.feature @@ -97,6 +97,12 @@ Feature: quickmarks and bookmarks And I run :bookmark-del Then the bookmark file should not contain "http://localhost:*/data/numbers/6.txt " + Scenario: Toggling a bookmark + When I open data/numbers/7.txt + And I run :bookmark-add + And I run :bookmark-add --toggle + Then the bookmark file should not contain "http://localhost:*/data/numbers/7.txt " + ## quickmarks Scenario: Saving a quickmark (:quickmark-add) From ae7fe2ee33f466f592ae42e0de91e46a5ecb8dcb Mon Sep 17 00:00:00 2001 From: Marshall Lochbaum Date: Tue, 26 Jul 2016 09:14:51 -0400 Subject: [PATCH 2/3] Update number files --- tests/end2end/data/numbers/18.txt | 1 + tests/end2end/features/urlmarks.feature | 80 ++++++++++++------------- 2 files changed, 41 insertions(+), 40 deletions(-) create mode 100644 tests/end2end/data/numbers/18.txt diff --git a/tests/end2end/data/numbers/18.txt b/tests/end2end/data/numbers/18.txt new file mode 100644 index 000000000..63affa473 --- /dev/null +++ b/tests/end2end/data/numbers/18.txt @@ -0,0 +1 @@ +eighteen diff --git a/tests/end2end/features/urlmarks.feature b/tests/end2end/features/urlmarks.feature index 383cb492a..4054e3f15 100644 --- a/tests/end2end/features/urlmarks.feature +++ b/tests/end2end/features/urlmarks.feature @@ -106,30 +106,30 @@ Feature: quickmarks and bookmarks ## quickmarks Scenario: Saving a quickmark (:quickmark-add) - When I run :quickmark-add http://localhost:(port)/data/numbers/7.txt seven - Then the quickmark file should contain "seven http://localhost:*/data/numbers/7.txt" - - Scenario: Saving a quickmark (:quickmark-save) - When I open data/numbers/8.txt - And I run :quickmark-save - And I wait for "Entering mode KeyMode.prompt (reason: question asked)" in the log - And I press the keys "eight" - And I press the keys "" + When I run :quickmark-add http://localhost:(port)/data/numbers/8.txt eight Then the quickmark file should contain "eight http://localhost:*/data/numbers/8.txt" - Scenario: Saving a duplicate quickmark (without override) - When I run :quickmark-add http://localhost:(port)/data/numbers/9.txt nine - And I run :quickmark-add http://localhost:(port)/data/numbers/9_2.txt nine - And I wait for "Entering mode KeyMode.yesno (reason: question asked)" in the log - And I run :prompt-no + Scenario: Saving a quickmark (:quickmark-save) + When I open data/numbers/9.txt + And I run :quickmark-save + And I wait for "Entering mode KeyMode.prompt (reason: question asked)" in the log + And I press the keys "nine" + And I press the keys "" Then the quickmark file should contain "nine http://localhost:*/data/numbers/9.txt" - Scenario: Saving a duplicate quickmark (with override) + Scenario: Saving a duplicate quickmark (without override) When I run :quickmark-add http://localhost:(port)/data/numbers/10.txt ten And I run :quickmark-add http://localhost:(port)/data/numbers/10_2.txt ten And I wait for "Entering mode KeyMode.yesno (reason: question asked)" in the log + And I run :prompt-no + Then the quickmark file should contain "ten http://localhost:*/data/numbers/10.txt" + + Scenario: Saving a duplicate quickmark (with override) + When I run :quickmark-add http://localhost:(port)/data/numbers/11.txt eleven + And I run :quickmark-add http://localhost:(port)/data/numbers/11_2.txt eleven + And I wait for "Entering mode KeyMode.yesno (reason: question asked)" in the log And I run :prompt-yes - Then the quickmark file should contain "ten http://localhost:*/data/numbers/10_2.txt" + Then the quickmark file should contain "eleven http://localhost:*/data/numbers/11_2.txt" Scenario: Adding a quickmark with an empty name When I run :quickmark-add about:blank "" @@ -141,38 +141,38 @@ Feature: quickmarks and bookmarks Scenario: Loading a quickmark Given I have a fresh instance - When I run :quickmark-add http://localhost:(port)/data/numbers/11.txt eleven - And I run :quickmark-load eleven - Then data/numbers/11.txt should be loaded + When I run :quickmark-add http://localhost:(port)/data/numbers/12.txt twelve + And I run :quickmark-load twelve + Then data/numbers/12.txt should be loaded And the following tabs should be open: - - data/numbers/11.txt (active) + - data/numbers/12.txt (active) Scenario: Loading a quickmark in a new tab Given I open about:blank When I run :tab-only - And I run :quickmark-add http://localhost:(port)/data/numbers/12.txt twelve - And I run :quickmark-load -t twelve - Then data/numbers/12.txt should be loaded + And I run :quickmark-add http://localhost:(port)/data/numbers/13.txt thirteen + And I run :quickmark-load -t thirteen + Then data/numbers/13.txt should be loaded And the following tabs should be open: - about:blank - - data/numbers/12.txt (active) + - data/numbers/13.txt (active) Scenario: Loading a quickmark in a background tab Given I open about:blank When I run :tab-only - And I run :quickmark-add http://localhost:(port)/data/numbers/13.txt thirteen - And I run :quickmark-load -b thirteen - Then data/numbers/13.txt should be loaded + And I run :quickmark-add http://localhost:(port)/data/numbers/14.txt fourteen + And I run :quickmark-load -b fourteen + Then data/numbers/14.txt should be loaded And the following tabs should be open: - about:blank (active) - - data/numbers/13.txt + - data/numbers/14.txt Scenario: Loading a quickmark in a new window Given I open about:blank When I run :tab-only - And I run :quickmark-add http://localhost:(port)/data/numbers/14.txt fourteen - And I run :quickmark-load -w fourteen - And I wait until data/numbers/14.txt is loaded + And I run :quickmark-add http://localhost:(port)/data/numbers/15.txt fifteen + And I run :quickmark-load -w fifteen + And I wait until data/numbers/15.txt is loaded Then the session should look like: windows: - tabs: @@ -184,15 +184,15 @@ Feature: quickmarks and bookmarks - active: true history: - active: true - url: http://localhost:*/data/numbers/14.txt + url: http://localhost:*/data/numbers/15.txt Scenario: Loading a quickmark which does not exist When I run :quickmark-load -b doesnotexist Then the error "Quickmark 'doesnotexist' does not exist!" should be shown Scenario: Loading a quickmark with -t and -b - When I run :quickmark-add http://localhost:(port)/data/numbers/15.txt fifteen - When I run :quickmark-load -t -b fifteen + When I run :quickmark-add http://localhost:(port)/data/numbers/16.txt sixteen + When I run :quickmark-load -t -b sixteen Then the error "Only one of -t/-b/-w can be given!" should be shown Scenario: Deleting a quickmark which does not exist @@ -200,9 +200,9 @@ Feature: quickmarks and bookmarks Then the error "Quickmark 'doesnotexist' not found!" should be shown Scenario: Deleting a quickmark - When I run :quickmark-add http://localhost:(port)/data/numbers/16.txt sixteen - And I run :quickmark-del sixteen - Then the quickmark file should not contain "sixteen http://localhost:*/data/numbers/16.txt " + When I run :quickmark-add http://localhost:(port)/data/numbers/17.txt seventeen + And I run :quickmark-del seventeen + Then the quickmark file should not contain "seventeen http://localhost:*/data/numbers/17.txt " Scenario: Deleting the current page's quickmark if it has none When I open about:blank @@ -210,10 +210,10 @@ Feature: quickmarks and bookmarks Then the error "Quickmark for 'about:blank' not found!" should be shown Scenario: Deleting the current page's quickmark - When I open data/numbers/17.txt - And I run :quickmark-add http://localhost:(port)/data/numbers/17.txt seventeen + When I open data/numbers/18.txt + And I run :quickmark-add http://localhost:(port)/data/numbers/18.txt eighteen And I run :quickmark-del - Then the quickmark file should not contain "seventeen http://localhost:*/data/numbers/17.txt" + Then the quickmark file should not contain "eighteen http://localhost:*/data/numbers/18.txt" Scenario: Listing quickmarks When I run :quickmark-add http://localhost:(port)/data/numbers/15.txt fifteen From d9247c15a4172d812b3a51b8b2b10df294dba16e Mon Sep 17 00:00:00 2001 From: Marshall Lochbaum Date: Tue, 26 Jul 2016 09:48:35 -0400 Subject: [PATCH 3/3] Coding style fixes --- qutebrowser/browser/commands.py | 7 +++---- qutebrowser/browser/urlmarks.py | 9 +++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index f86faa0da..3b0e0bc51 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1120,7 +1120,6 @@ class CommandDispatcher: raise cmdexc.CommandError("Quickmark '{}' not found!".format(name)) @cmdutils.register(instance='command-dispatcher', scope='window') - @cmdutils.argument('toggle', flag='t') def bookmark_add(self, url=None, title=None, toggle=False): """Save the current page as a bookmark, or a specific url. @@ -1152,13 +1151,13 @@ class CommandDispatcher: if not title: title = self._current_title() try: - if_added = bookmark_manager.add(url, title, toggle) + was_added = bookmark_manager.add(url, title, toggle=toggle) except urlmarks.Error as e: raise cmdexc.CommandError(str(e)) else: - mes = "Bookmarked {}!" if if_added else "Removed bookmark {}!" + msg = "Bookmarked {}!" if was_added else "Removed bookmark {}!" message.info(self._win_id, - mes.format(url.toDisplayString())) + msg.format(url.toDisplayString())) @cmdutils.register(instance='command-dispatcher', scope='window', maxsplit=0) diff --git a/qutebrowser/browser/urlmarks.py b/qutebrowser/browser/urlmarks.py index 9889c8536..4280f0cd9 100644 --- a/qutebrowser/browser/urlmarks.py +++ b/qutebrowser/browser/urlmarks.py @@ -272,17 +272,18 @@ class BookmarkManager(UrlMarkManager): elif len(parts) == 1: self.marks[parts[0]] = '' - def add(self, url, title, toggle=False): + def add(self, url, title, *, toggle=False): """Add a new bookmark. - Return True if the bookmark was added, and False if it was - removed (which only happens if toggle is True). - Args: url: The url to add as bookmark. title: The title for the new bookmark. toggle: remove the bookmark instead of raising an error if it already exists. + + Return: + True if the bookmark was added, and False if it was + removed (only possible if toggle is True). """ if not url.isValid(): errstr = urlutils.get_errstring(url)