From 828b7d744a6c5fa0f54853ec13342fdcfce65862 Mon Sep 17 00:00:00 2001 From: Daniel Karbach Date: Tue, 18 Oct 2016 11:40:17 +0200 Subject: [PATCH 01/44] setting values tabs->select-on-remove left -> prev right -> next previous -> last-used refs #1619 --- doc/help/settings.asciidoc | 8 +++---- qutebrowser/config/config.py | 5 ++++ qutebrowser/config/configdata.py | 2 +- qutebrowser/config/configtypes.py | 14 ++++++----- tests/end2end/features/tabs.feature | 36 ++++++++++++++--------------- 5 files changed, 36 insertions(+), 29 deletions(-) diff --git a/doc/help/settings.asciidoc b/doc/help/settings.asciidoc index d738a68ca..64a65848d 100644 --- a/doc/help/settings.asciidoc +++ b/doc/help/settings.asciidoc @@ -1029,11 +1029,11 @@ Which tab to select when the focused tab is removed. Valid values: - * +left+: Select the tab on the left. - * +right+: Select the tab on the right. - * +previous+: Select the previously selected tab. + * +prev+: Select the tab which came before the closed one (left in horizontal, above in vertical). + * +next+: Select the tab which came after the closed one (right in horizontal, below in vertical). + * +last-used+: Select the previously selected tab. -Default: +pass:[right]+ +Default: +pass:[next]+ [[tabs-new-tab-position]] === new-tab-position diff --git a/qutebrowser/config/config.py b/qutebrowser/config/config.py index 8ee27918a..f04066db3 100644 --- a/qutebrowser/config/config.py +++ b/qutebrowser/config/config.py @@ -412,6 +412,11 @@ class ConfigManager(QObject): ('content', 'cookies-accept'): _get_value_transformer({'default': 'no-3rdparty'}), ('tabs', 'position'): _transform_position, + ('tabs', 'select-on-remove'): + _get_value_transformer({ + 'left': 'prev', + 'right': 'next', + 'previous': 'last-used'}), ('ui', 'downloads-position'): _transform_position, ('ui', 'remove-finished-downloads'): _get_value_transformer({'false': '-1', 'true': '1000'}), diff --git a/qutebrowser/config/configdata.py b/qutebrowser/config/configdata.py index ea3939a44..e76bc6468 100644 --- a/qutebrowser/config/configdata.py +++ b/qutebrowser/config/configdata.py @@ -579,7 +579,7 @@ def data(readonly=False): "background."), ('select-on-remove', - SettingValue(typ.SelectOnRemove(), 'right'), + SettingValue(typ.SelectOnRemove(), 'next'), "Which tab to select when the focused tab is removed."), ('new-tab-position', diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 4fb40bfbb..5cec34630 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -1379,18 +1379,20 @@ class SelectOnRemove(MappingType): """Which tab to select when the focused tab is removed.""" MAPPING = { - 'left': QTabBar.SelectLeftTab, - 'right': QTabBar.SelectRightTab, - 'previous': QTabBar.SelectPreviousTab, + 'prev': QTabBar.SelectLeftTab, + 'next': QTabBar.SelectRightTab, + 'last-used': QTabBar.SelectPreviousTab, } def __init__(self, none_ok=False): super().__init__( none_ok, valid_values=ValidValues( - ('left', "Select the tab on the left."), - ('right', "Select the tab on the right."), - ('previous', "Select the previously selected tab."))) + ('prev', "Select the tab which came before the closed one " + "(left in horizontal, above in vertical)."), + ('next', "Select the tab which came after the closed one " + "(right in horizontal, below in vertical)."), + ('last-used', "Select the previously selected tab."))) class ConfirmQuit(FlagList): diff --git a/tests/end2end/features/tabs.feature b/tests/end2end/features/tabs.feature index b138397c5..717f39a73 100644 --- a/tests/end2end/features/tabs.feature +++ b/tests/end2end/features/tabs.feature @@ -35,8 +35,8 @@ Feature: Tab management - data/numbers/2.txt - data/numbers/3.txt (active) - Scenario: :tab-close with select-on-remove = right - When I set tabs -> select-on-remove to right + Scenario: :tab-close with select-on-remove = next + When I set tabs -> select-on-remove to next And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab @@ -46,8 +46,8 @@ Feature: Tab management - data/numbers/1.txt - data/numbers/3.txt (active) - Scenario: :tab-close with select-on-remove = left - When I set tabs -> select-on-remove to left + Scenario: :tab-close with select-on-remove = prev + When I set tabs -> select-on-remove to prev And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab @@ -57,8 +57,8 @@ Feature: Tab management - data/numbers/1.txt (active) - data/numbers/3.txt - Scenario: :tab-close with select-on-remove = previous - When I set tabs -> select-on-remove to previous + Scenario: :tab-close with select-on-remove = last-used + When I set tabs -> select-on-remove to last-used And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab @@ -70,8 +70,8 @@ Feature: Tab management - data/numbers/3.txt - data/numbers/4.txt (active) - Scenario: :tab-close with select-on-remove = left and --right - When I set tabs -> select-on-remove to left + Scenario: :tab-close with select-on-remove = prev and --right + When I set tabs -> select-on-remove to prev And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab @@ -81,8 +81,8 @@ Feature: Tab management - data/numbers/1.txt - data/numbers/3.txt (active) - Scenario: :tab-close with select-on-remove = right and --left - When I set tabs -> select-on-remove to right + Scenario: :tab-close with select-on-remove = next and --left + When I set tabs -> select-on-remove to next And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab @@ -92,8 +92,8 @@ Feature: Tab management - data/numbers/1.txt (active) - data/numbers/3.txt - Scenario: :tab-close with select-on-remove = left and --opposite - When I set tabs -> select-on-remove to left + Scenario: :tab-close with select-on-remove = prev and --opposite + When I set tabs -> select-on-remove to prev And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab @@ -103,8 +103,8 @@ Feature: Tab management - data/numbers/1.txt - data/numbers/3.txt (active) - Scenario: :tab-close with select-on-remove = right and --opposite - When I set tabs -> select-on-remove to right + Scenario: :tab-close with select-on-remove = next and --opposite + When I set tabs -> select-on-remove to next And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab @@ -114,13 +114,13 @@ Feature: Tab management - data/numbers/1.txt (active) - data/numbers/3.txt - Scenario: :tab-close with select-on-remove = previous and --opposite - When I set tabs -> select-on-remove to previous + Scenario: :tab-close with select-on-remove = last-used and --opposite + When I set tabs -> select-on-remove to last-used And I run :tab-close --opposite - Then the error "-o is not supported with 'tabs->select-on-remove' set to 'previous'!" should be shown + Then the error "-o is not supported with 'tabs->select-on-remove' set to 'last-used'!" should be shown Scenario: :tab-close should restore selection behavior - When I set tabs -> select-on-remove to right + When I set tabs -> select-on-remove to next And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab From 845298ae4117a898b84ca2d010ff3529d5922c04 Mon Sep 17 00:00:00 2001 From: Daniel Karbach Date: Tue, 18 Oct 2016 12:35:37 +0200 Subject: [PATCH 02/44] :tab-close option names --left -> --prev --right -> --next --- doc/help/commands.asciidoc | 6 +++--- qutebrowser/browser/commands.py | 22 +++++++++++----------- qutebrowser/config/configdata.py | 5 +++++ tests/end2end/features/tabs.feature | 10 +++++----- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/doc/help/commands.asciidoc b/doc/help/commands.asciidoc index 093b6f2c6..c54c4e04b 100644 --- a/doc/help/commands.asciidoc +++ b/doc/help/commands.asciidoc @@ -779,13 +779,13 @@ Duplicate the current tab. [[tab-close]] === tab-close -Syntax: +:tab-close [*--left*] [*--right*] [*--opposite*]+ +Syntax: +:tab-close [*--prev*] [*--next*] [*--opposite*]+ Close the current/[count]th tab. ==== optional arguments -* +*-l*+, +*--left*+: Force selecting the tab to the left of the current tab. -* +*-r*+, +*--right*+: Force selecting the tab to the right of the current tab. +* +*-p*+, +*--prev*+: Force selecting the tab before the current tab. +* +*-n*+, +*--next*+: Force selecting the tab after the current tab. * +*-o*+, +*--opposite*+: Force selecting the tab in the opposite direction of what's configured in 'tabs->select-on-remove'. diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index dc17c598f..87885d4cc 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -179,12 +179,12 @@ class CommandDispatcher: raise cmdexc.CommandError("Last focused tab vanished!") self._set_current_index(idx) - def _get_selection_override(self, left, right, opposite): + def _get_selection_override(self, prev, next_, opposite): """Helper function for tab_close to get the tab to select. Args: - left: Force selecting the tab to the left of the current tab. - right: Force selecting the tab to the right of the current tab. + prev: Force selecting the tab before the current tab. + next_: Force selecting the tab after the current tab. opposite: Force selecting the tab in the opposite direction of what's configured in 'tabs->select-on-remove'. @@ -192,10 +192,10 @@ class CommandDispatcher: QTabBar.SelectLeftTab, QTabBar.SelectRightTab, or None if no change should be made. """ - cmdutils.check_exclusive((left, right, opposite), 'lro') - if left: + cmdutils.check_exclusive((prev, next_, opposite), 'pno') + if prev: return QTabBar.SelectLeftTab - elif right: + elif next_: return QTabBar.SelectRightTab elif opposite: conf_selection = config.get('tabs', 'select-on-remove') @@ -206,7 +206,7 @@ class CommandDispatcher: elif conf_selection == QTabBar.SelectPreviousTab: raise cmdexc.CommandError( "-o is not supported with 'tabs->select-on-remove' set to " - "'previous'!") + "'last-used'!") else: # pragma: no cover raise ValueError("Invalid select-on-remove value " "{!r}!".format(conf_selection)) @@ -214,12 +214,12 @@ class CommandDispatcher: @cmdutils.register(instance='command-dispatcher', scope='window') @cmdutils.argument('count', count=True) - def tab_close(self, left=False, right=False, opposite=False, count=None): + def tab_close(self, prev=False, next_=False, opposite=False, count=None): """Close the current/[count]th tab. Args: - left: Force selecting the tab to the left of the current tab. - right: Force selecting the tab to the right of the current tab. + prev: Force selecting the tab before the current tab. + next_: Force selecting the tab after the current tab. opposite: Force selecting the tab in the opposite direction of what's configured in 'tabs->select-on-remove'. count: The tab index to close, or None @@ -228,7 +228,7 @@ class CommandDispatcher: if tab is None: return tabbar = self._tabbed_browser.tabBar() - selection_override = self._get_selection_override(left, right, + selection_override = self._get_selection_override(prev, next_, opposite) if selection_override is None: self._tabbed_browser.close_tab(tab) diff --git a/qutebrowser/config/configdata.py b/qutebrowser/config/configdata.py index e76bc6468..a8c5fed88 100644 --- a/qutebrowser/config/configdata.py +++ b/qutebrowser/config/configdata.py @@ -1781,4 +1781,9 @@ CHANGED_KEY_COMMANDS = [ (re.compile(r'^prompt-yes$'), r'prompt-accept yes'), (re.compile(r'^prompt-no$'), r'prompt-accept no'), + + (re.compile(r'^tab-close -l$'), r'tab-close --prev'), + (re.compile(r'^tab-close --left$'), r'tab-close --prev'), + (re.compile(r'^tab-close -r$'), r'tab-close --next'), + (re.compile(r'^tab-close --right$'), r'tab-close --next'), ] diff --git a/tests/end2end/features/tabs.feature b/tests/end2end/features/tabs.feature index 717f39a73..de8742579 100644 --- a/tests/end2end/features/tabs.feature +++ b/tests/end2end/features/tabs.feature @@ -70,24 +70,24 @@ Feature: Tab management - data/numbers/3.txt - data/numbers/4.txt (active) - Scenario: :tab-close with select-on-remove = prev and --right + Scenario: :tab-close with select-on-remove = prev and --next When I set tabs -> select-on-remove to prev And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab And I run :tab-focus 2 - And I run :tab-close --right + And I run :tab-close --next Then the following tabs should be open: - data/numbers/1.txt - data/numbers/3.txt (active) - Scenario: :tab-close with select-on-remove = next and --left + Scenario: :tab-close with select-on-remove = next and --prev When I set tabs -> select-on-remove to next And I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab And I run :tab-focus 2 - And I run :tab-close --left + And I run :tab-close --prev Then the following tabs should be open: - data/numbers/1.txt (active) - data/numbers/3.txt @@ -126,7 +126,7 @@ Feature: Tab management And I open data/numbers/3.txt in a new tab And I open data/numbers/4.txt in a new tab And I run :tab-focus 2 - And I run :tab-close --left + And I run :tab-close --prev And I run :tab-focus 2 And I run :tab-close Then the following tabs should be open: From 7eafa300840110fcc93d3f196d358f22c07d9ba6 Mon Sep 17 00:00:00 2001 From: Daniel Karbach Date: Tue, 18 Oct 2016 12:55:58 +0200 Subject: [PATCH 03/44] :tab-close option names --left -> --prev --right -> --next --- doc/help/commands.asciidoc | 6 +++--- qutebrowser/browser/commands.py | 12 ++++++------ qutebrowser/config/configdata.py | 5 +++++ tests/end2end/features/tabs.feature | 14 +++++++------- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/doc/help/commands.asciidoc b/doc/help/commands.asciidoc index c54c4e04b..e69e7da14 100644 --- a/doc/help/commands.asciidoc +++ b/doc/help/commands.asciidoc @@ -841,13 +841,13 @@ How many tabs to switch forward. [[tab-only]] === tab-only -Syntax: +:tab-only [*--left*] [*--right*]+ +Syntax: +:tab-only [*--prev*] [*--next*]+ Close all tabs except for the current one. ==== optional arguments -* +*-l*+, +*--left*+: Keep tabs to the left of the current. -* +*-r*+, +*--right*+: Keep tabs to the right of the current. +* +*-p*+, +*--prev*+: Keep tabs before the current. +* +*-n*+, +*--next*+: Keep tabs after the current. [[tab-prev]] === tab-prev diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 87885d4cc..8ea978496 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -801,20 +801,20 @@ class CommandDispatcher: message.info("Zoom level: {}%".format(level)) @cmdutils.register(instance='command-dispatcher', scope='window') - def tab_only(self, left=False, right=False): + def tab_only(self, prev=False, next_=False): """Close all tabs except for the current one. Args: - left: Keep tabs to the left of the current. - right: Keep tabs to the right of the current. + prev: Keep tabs before the current. + next_: Keep tabs after the current. """ - cmdutils.check_exclusive((left, right), 'lr') + cmdutils.check_exclusive((prev, next_), 'pn') cur_idx = self._tabbed_browser.currentIndex() assert cur_idx != -1 for i, tab in enumerate(self._tabbed_browser.widgets()): - if (i == cur_idx or (left and i < cur_idx) or - (right and i > cur_idx)): + if (i == cur_idx or (prev and i < cur_idx) or + (next_ and i > cur_idx)): continue else: self._tabbed_browser.close_tab(tab) diff --git a/qutebrowser/config/configdata.py b/qutebrowser/config/configdata.py index a8c5fed88..64bef47b1 100644 --- a/qutebrowser/config/configdata.py +++ b/qutebrowser/config/configdata.py @@ -1786,4 +1786,9 @@ CHANGED_KEY_COMMANDS = [ (re.compile(r'^tab-close --left$'), r'tab-close --prev'), (re.compile(r'^tab-close -r$'), r'tab-close --next'), (re.compile(r'^tab-close --right$'), r'tab-close --next'), + + (re.compile(r'^tab-only -l$'), r'tab-only --prev'), + (re.compile(r'^tab-only --left$'), r'tab-only --prev'), + (re.compile(r'^tab-only -r$'), r'tab-only --next'), + (re.compile(r'^tab-only --right$'), r'tab-only --next'), ] diff --git a/tests/end2end/features/tabs.feature b/tests/end2end/features/tabs.feature index de8742579..76dd9ddfd 100644 --- a/tests/end2end/features/tabs.feature +++ b/tests/end2end/features/tabs.feature @@ -143,29 +143,29 @@ Feature: Tab management Then the following tabs should be open: - data/numbers/3.txt (active) - Scenario: :tab-only with --left + Scenario: :tab-only with --prev When I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab And I run :tab-focus 2 - And I run :tab-only --left + And I run :tab-only --prev Then the following tabs should be open: - data/numbers/1.txt - data/numbers/2.txt (active) - Scenario: :tab-only with --right + Scenario: :tab-only with --next When I open data/numbers/1.txt And I open data/numbers/2.txt in a new tab And I open data/numbers/3.txt in a new tab And I run :tab-focus 2 - And I run :tab-only --right + And I run :tab-only --next Then the following tabs should be open: - data/numbers/2.txt (active) - data/numbers/3.txt - Scenario: :tab-only with --left and --right - When I run :tab-only --left --right - Then the error "Only one of -l/-r can be given!" should be shown + Scenario: :tab-only with --prev and --next + When I run :tab-only --prev --next + Then the error "Only one of -p/-n can be given!" should be shown # :tab-focus From ef3968c165d51e3a88f9878c92cbe1e8c90afc11 Mon Sep 17 00:00:00 2001 From: Daniel Karbach Date: Tue, 18 Oct 2016 14:46:16 +0200 Subject: [PATCH 04/44] setting values tabs->new-tab-position[-explicit] left -> prev right -> next --- doc/help/settings.asciidoc | 18 +++++++++--------- qutebrowser/config/config.py | 8 ++++++++ qutebrowser/config/configdata.py | 2 +- qutebrowser/config/configtypes.py | 8 ++++---- qutebrowser/mainwindow/tabbedbrowser.py | 10 +++++----- tests/end2end/features/open.feature | 8 ++++---- tests/end2end/features/tabs.feature | 8 ++++---- 7 files changed, 35 insertions(+), 27 deletions(-) diff --git a/doc/help/settings.asciidoc b/doc/help/settings.asciidoc index 64a65848d..b6e5b5d1b 100644 --- a/doc/help/settings.asciidoc +++ b/doc/help/settings.asciidoc @@ -1041,12 +1041,12 @@ How new tabs are positioned. Valid values: - * +left+: On the left of the current tab. - * +right+: On the right of the current tab. - * +first+: At the left end. - * +last+: At the right end. + * +prev+: Before the current tab. + * +next+: After the current tab. + * +first+: At the beginning. + * +last+: At the end. -Default: +pass:[right]+ +Default: +pass:[next]+ [[tabs-new-tab-position-explicit]] === new-tab-position-explicit @@ -1054,10 +1054,10 @@ How new tabs opened explicitly are positioned. Valid values: - * +left+: On the left of the current tab. - * +right+: On the right of the current tab. - * +first+: At the left end. - * +last+: At the right end. + * +prev+: Before the current tab. + * +next+: After the current tab. + * +first+: At the beginning. + * +last+: At the end. Default: +pass:[last]+ diff --git a/qutebrowser/config/config.py b/qutebrowser/config/config.py index f04066db3..81d8015f2 100644 --- a/qutebrowser/config/config.py +++ b/qutebrowser/config/config.py @@ -411,6 +411,14 @@ class ConfigManager(QObject): CHANGED_OPTIONS = { ('content', 'cookies-accept'): _get_value_transformer({'default': 'no-3rdparty'}), + ('tabs', 'new-tab-position'): + _get_value_transformer({ + 'left': 'prev', + 'right': 'next'}), + ('tabs', 'new-tab-position-explicit'): + _get_value_transformer({ + 'left': 'prev', + 'right': 'next'}), ('tabs', 'position'): _transform_position, ('tabs', 'select-on-remove'): _get_value_transformer({ diff --git a/qutebrowser/config/configdata.py b/qutebrowser/config/configdata.py index 64bef47b1..edf5abded 100644 --- a/qutebrowser/config/configdata.py +++ b/qutebrowser/config/configdata.py @@ -583,7 +583,7 @@ def data(readonly=False): "Which tab to select when the focused tab is removed."), ('new-tab-position', - SettingValue(typ.NewTabPosition(), 'right'), + SettingValue(typ.NewTabPosition(), 'next'), "How new tabs are positioned."), ('new-tab-position-explicit', diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 5cec34630..346b5a8d7 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -1436,10 +1436,10 @@ class NewTabPosition(BaseType): def __init__(self, none_ok=False): super().__init__(none_ok) self.valid_values = ValidValues( - ('left', "On the left of the current tab."), - ('right', "On the right of the current tab."), - ('first', "At the left end."), - ('last', "At the right end.")) + ('prev', "Before the current tab."), + ('next', "After the current tab."), + ('first', "At the beginning."), + ('last', "At the end.")) class IgnoreCase(Bool): diff --git a/qutebrowser/mainwindow/tabbedbrowser.py b/qutebrowser/mainwindow/tabbedbrowser.py index 6efec0851..3bcadc333 100644 --- a/qutebrowser/mainwindow/tabbedbrowser.py +++ b/qutebrowser/mainwindow/tabbedbrowser.py @@ -61,8 +61,8 @@ class TabbedBrowser(tabwidget.TabWidget): _filter: A SignalFilter instance. _now_focused: The tab which is focused now. _tab_insert_idx_left: Where to insert a new tab with - tabbar -> new-tab-position set to 'left'. - _tab_insert_idx_right: Same as above, for 'right'. + tabbar -> new-tab-position set to 'prev'. + _tab_insert_idx_right: Same as above, for 'next'. _undo_stack: List of UndoEntry namedtuples of closed tabs. shutting_down: Whether we're currently shutting down. _local_marks: Jump markers local to each page @@ -410,14 +410,14 @@ class TabbedBrowser(tabwidget.TabWidget): pos = config.get('tabs', 'new-tab-position-explicit') else: pos = config.get('tabs', 'new-tab-position') - if pos == 'left': + if pos == 'prev': idx = self._tab_insert_idx_left # On first sight, we'd think we have to decrement # self._tab_insert_idx_left here, as we want the next tab to be # *before* the one we just opened. However, since we opened a tab - # *to the left* of the currently focused tab, indices will shift by + # *before* of the currently focused tab, indices will shift by # 1 automatically. - elif pos == 'right': + elif pos == 'next': idx = self._tab_insert_idx_right self._tab_insert_idx_right += 1 elif pos == 'first': diff --git a/tests/end2end/features/open.feature b/tests/end2end/features/open.feature index 45012e639..3f6715e6a 100644 --- a/tests/end2end/features/open.feature +++ b/tests/end2end/features/open.feature @@ -75,8 +75,8 @@ Feature: Opening pages Scenario: Opening in a new tab (explicit) Given I open about:blank - When I set tabs -> new-tab-position-explicit to right - And I set tabs -> new-tab-position to left + When I set tabs -> new-tab-position-explicit to next + And I set tabs -> new-tab-position to prev And I run :tab-only And I run :open -t http://localhost:(port)/data/numbers/7.txt And I wait until data/numbers/7.txt is loaded @@ -86,8 +86,8 @@ Feature: Opening pages Scenario: Opening in a new tab (implicit) Given I open about:blank - When I set tabs -> new-tab-position-explicit to right - And I set tabs -> new-tab-position to left + When I set tabs -> new-tab-position-explicit to next + And I set tabs -> new-tab-position to prev And I run :tab-only And I run :open -t -i http://localhost:(port)/data/numbers/8.txt And I wait until data/numbers/8.txt is loaded diff --git a/tests/end2end/features/tabs.feature b/tests/end2end/features/tabs.feature index 76dd9ddfd..1663dd8d7 100644 --- a/tests/end2end/features/tabs.feature +++ b/tests/end2end/features/tabs.feature @@ -821,8 +821,8 @@ Feature: Tab management - data/hints/html/simple.html (active) - data/hello.txt - Scenario: opening tab with tabs->new-tab-position left - When I set tabs -> new-tab-position to left + Scenario: opening tab with tabs->new-tab-position prev + When I set tabs -> new-tab-position to prev And I set tabs -> background-tabs to false And I open about:blank And I open data/hints/html/simple.html in a new tab @@ -833,8 +833,8 @@ Feature: Tab management - data/hello.txt (active) - data/hints/html/simple.html - Scenario: opening tab with tabs->new-tab-position right - When I set tabs -> new-tab-position to right + Scenario: opening tab with tabs->new-tab-position next + When I set tabs -> new-tab-position to next And I set tabs -> background-tabs to false And I open about:blank And I open data/hints/html/simple.html in a new tab From 70e390a2e8f0056a5aa7eb7e6bb1cbe73a0c8c4a Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 14:44:08 +0100 Subject: [PATCH 05/44] downloads: Simplify redirect handling --- qutebrowser/browser/webkit/downloads.py | 47 +++++++++---------------- 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index d25c363ea..62858defb 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -291,9 +291,6 @@ class DownloadItem(QObject): cancelled: The download was cancelled. error: An error with the download occurred. arg: The error message as string. - redirected: Signal emitted when a download was redirected. - arg 0: The new QNetworkRequest. - arg 1: The old QNetworkReply. do_retry: Emitted when a download is retried. arg 0: The new DownloadItem remove_requested: Emitted when the removal of this download was @@ -305,7 +302,6 @@ class DownloadItem(QObject): finished = pyqtSignal() error = pyqtSignal(str) cancelled = pyqtSignal() - redirected = pyqtSignal(QNetworkRequest, QNetworkReply) do_retry = pyqtSignal(object) # DownloadItem remove_requested = pyqtSignal() @@ -332,7 +328,7 @@ class DownloadItem(QObject): self.successful = False self.fileobj = None self._filename = None - self.init_reply(reply) + self._init_reply(reply) self._win_id = win_id self.raw_headers = {} self._dead = False @@ -395,7 +391,7 @@ class DownloadItem(QObject): """Abort the download and emit an error.""" assert not self.successful # Prevent actions if calling _die() twice. This might happen if the - # error handler correctly connects, and the error occurs in init_reply + # error handler correctly connects, and the error occurs in _init_reply # between reply.error.connect and the reply.error() check. In this # case, the connected error handlers will be called twice, once via the # direct error.emit() and once here in _die(). The stacks look like @@ -403,7 +399,7 @@ class DownloadItem(QObject): # -> on_reply_error -> _die -> # self.error.emit() # and - # [init_reply -> ->] -> + # [_init_reply -> ->] -> # self.error.emit() # which may lead to duplicate error messages (and failing tests) if self._dead: @@ -432,7 +428,7 @@ class DownloadItem(QObject): except OSError: log.downloads.exception("Error while closing file object") - def init_reply(self, reply): + def _init_reply(self, reply): """Set a new reply and connect its signals. Args: @@ -737,8 +733,8 @@ class DownloadItem(QObject): if redirect is None or redirect.isEmpty(): return False new_url = self._reply.url().resolved(redirect) - request = self._reply.request() - if new_url == request.url(): + new_request = self._reply.request() + if new_url == new_request.url(): return False if self._redirects > self._MAX_REDIRECTS: @@ -748,15 +744,20 @@ class DownloadItem(QObject): log.downloads.debug("{}: Handling redirect".format(self)) self._redirects += 1 - request.setUrl(new_url) - reply = self._reply - reply.finished.disconnect(self._on_reply_finished) + new_request.setUrl(new_url) + old_reply = self._reply + old_reply.finished.disconnect(self._on_reply_finished) self._read_timer.stop() self._reply = None if self.fileobj is not None: self.fileobj.seek(0) - self.redirected.emit(request, reply) # this will change self._reply! - reply.deleteLater() # the old one + + log.downloads.debug("redirected: {} -> {}".format( + old_reply.url(), new_request.url())) + new_reply = old_reply.manager().get(new_request) + self._init_reply(new_reply) + + old_reply.deleteLater() return True def uses_nam(self, nam): @@ -940,8 +941,6 @@ class DownloadManager(QObject): download.data_changed.connect( functools.partial(self._on_data_changed, download)) download.error.connect(self._on_error) - download.redirected.connect( - functools.partial(self._on_redirect, download)) download.basename = suggested_filename idx = len(self.downloads) download.index = idx + 1 # "Human readable" index @@ -1023,20 +1022,6 @@ class DownloadManager(QObject): return download.open_file(cmdline) - @pyqtSlot(QNetworkRequest, QNetworkReply) - def _on_redirect(self, download, request, reply): - """Handle an HTTP redirect of a download. - - Args: - download: The old DownloadItem. - request: The new QNetworkRequest. - reply: The old QNetworkReply. - """ - log.downloads.debug("redirected: {} -> {}".format( - reply.url(), request.url())) - new_reply = reply.manager().get(request) - download.init_reply(new_reply) - @pyqtSlot(DownloadItem) def _on_data_changed(self, download): """Emit data_changed signal when download data changed.""" From 34b4dcf0d54321536065191a3cefdc9888892d68 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 14:48:28 +0100 Subject: [PATCH 06/44] Make DownloadItem._retry_info private --- qutebrowser/browser/webkit/downloads.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index 62858defb..9a2984ba5 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -273,7 +273,7 @@ class DownloadItem(QObject): autoclose: Whether to close the associated file if the download is done. fileobj: The file object to download the file to. - retry_info: A _RetryInfo instance. + _retry_info: A _RetryInfo instance. raw_headers: The headers sent by the server. _filename: The filename of the download. _redirects: How many time we were redirected already. @@ -312,7 +312,7 @@ class DownloadItem(QObject): reply: The QNetworkReply to download. """ super().__init__(parent) - self.retry_info = None + self._retry_info = None self.done = False self.stats = DownloadItemStats(self) self.index = 0 @@ -443,8 +443,8 @@ class DownloadItem(QObject): reply.error.connect(self._on_reply_error) reply.readyRead.connect(self._on_ready_read) reply.metaDataChanged.connect(self._on_meta_data_changed) - self.retry_info = _RetryInfo(request=reply.request(), - manager=reply.manager()) + self._retry_info = _RetryInfo(request=reply.request(), + manager=reply.manager()) if not self.fileobj: self._read_timer.start() # We could have got signals before we connected slots to them. @@ -521,7 +521,7 @@ class DownloadItem(QObject): assert not self.successful download_manager = objreg.get('download-manager', scope='window', window=self._win_id) - new_reply = self.retry_info.manager.get(self.retry_info.request) + new_reply = self._retry_info.manager.get(self._retry_info.request) new_download = download_manager.fetch( new_reply, suggested_filename=self.basename) self.do_retry.emit(new_download) @@ -765,7 +765,7 @@ class DownloadItem(QObject): running_nam = self._reply is not None and self._reply.manager() is nam # user could request retry after tab is closed. retry_nam = (self.done and (not self.successful) and - self.retry_info.manager is nam) + self._retry_info.manager is nam) return running_nam or retry_nam From 352f83b95e15cb65070e279fefd1d4c3aa031371 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 14:51:04 +0100 Subject: [PATCH 07/44] Rename DownloadItem.do_retry --- qutebrowser/browser/webkit/downloads.py | 9 +++++---- qutebrowser/browser/webkit/network/networkmanager.py | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index 9a2984ba5..1fe6d643b 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -291,8 +291,9 @@ class DownloadItem(QObject): cancelled: The download was cancelled. error: An error with the download occurred. arg: The error message as string. - do_retry: Emitted when a download is retried. - arg 0: The new DownloadItem + adopt_download: Emitted when a download is retried and should be adopted + by the QNAM if needed.. + arg 0: The new DownloadItem remove_requested: Emitted when the removal of this download was requested. """ @@ -302,7 +303,7 @@ class DownloadItem(QObject): finished = pyqtSignal() error = pyqtSignal(str) cancelled = pyqtSignal() - do_retry = pyqtSignal(object) # DownloadItem + adopt_download = pyqtSignal(object) # DownloadItem remove_requested = pyqtSignal() def __init__(self, reply, win_id, parent=None): @@ -524,7 +525,7 @@ class DownloadItem(QObject): new_reply = self._retry_info.manager.get(self._retry_info.request) new_download = download_manager.fetch( new_reply, suggested_filename=self.basename) - self.do_retry.emit(new_download) + self.adopt_download.emit(new_download) self.cancel() @pyqtSlot() diff --git a/qutebrowser/browser/webkit/network/networkmanager.py b/qutebrowser/browser/webkit/network/networkmanager.py index 7024e7635..6eb6c4ba7 100644 --- a/qutebrowser/browser/webkit/network/networkmanager.py +++ b/qutebrowser/browser/webkit/network/networkmanager.py @@ -414,7 +414,7 @@ class NetworkManager(QNetworkAccessManager): log.downloads.debug("Adopted download, {} adopted.".format( self.adopted_downloads)) download.destroyed.connect(self.on_adopted_download_destroyed) - download.do_retry.connect(self.adopt_download) + download.adopt_download.connect(self.adopt_download) def set_referer(self, req, current_url): """Set the referer header.""" From 2c94efbf8a87e46f2c862b334c8f44eb031dd03a Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 16:19:15 +0100 Subject: [PATCH 08/44] First big download refactoring chunk --- qutebrowser/browser/downloads.py | 501 ++++++++++++++++++++++++ qutebrowser/browser/webkit/downloads.py | 425 +------------------- 2 files changed, 519 insertions(+), 407 deletions(-) create mode 100644 qutebrowser/browser/downloads.py diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py new file mode 100644 index 000000000..b1cf085e1 --- /dev/null +++ b/qutebrowser/browser/downloads.py @@ -0,0 +1,501 @@ +# vim: ft=python fileencoding=utf-8 sts=4 sw=4 et: + +# Copyright 2014-2016 Florian Bruhin (The Compiler) +# +# This file is part of qutebrowser. +# +# qutebrowser is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# qutebrowser is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with qutebrowser. If not, see . + +"""Shared QtWebKit/QtWebEngine code for downloads.""" + +import sys +import shlex +import html +import os.path +import collections + +from PyQt5.QtCore import pyqtSlot, pyqtSignal, Qt, QObject, QUrl +from PyQt5.QtGui import QDesktopServices + +from qutebrowser.config import config +from qutebrowser.utils import usertypes, standarddir, utils, message, log +from qutebrowser.misc import guiprocess + + +_DownloadPath = collections.namedtuple('_DownloadPath', ['filename', + 'question']) + + +ModelRole = usertypes.enum('ModelRole', ['item'], start=Qt.UserRole, + is_int=True) + + +# Remember the last used directory +last_used_directory = None + + +# All REFRESH_INTERVAL milliseconds, speeds will be recalculated and downloads +# redrawn. +_REFRESH_INTERVAL = 500 + + +class UnsupportedAttribute: + + """Class which is used to create attributes which are not supported. + + This is used for attributes like "fileobj" for downloads which are not + supported with QtWebengine. + """ + + pass + + +def download_dir(): + """Get the download directory to use.""" + directory = config.get('storage', 'download-directory') + remember_dir = config.get('storage', 'remember-download-directory') + + if remember_dir and last_used_directory is not None: + return last_used_directory + elif directory is None: + return standarddir.download() + else: + return directory + + +def _path_suggestion(filename): + """Get the suggested file path. + + Args: + filename: The filename to use if included in the suggestion. + """ + suggestion = config.get('completion', 'download-path-suggestion') + if suggestion == 'path': + # add trailing '/' if not present + return os.path.join(download_dir(), '') + elif suggestion == 'filename': + return filename + elif suggestion == 'both': + return os.path.join(download_dir(), filename) + else: # pragma: no cover + raise ValueError("Invalid suggestion value {}!".format(suggestion)) + + +def create_full_filename(basename, filename): + """Create a full filename based on the given basename and filename. + + Args: + basename: The basename to use if filename is a directory. + filename: The path to a folder or file where you want to save. + + Return: + The full absolute path, or None if filename creation was not possible. + """ + # Remove chars which can't be encoded in the filename encoding. + # See https://github.com/The-Compiler/qutebrowser/issues/427 + encoding = sys.getfilesystemencoding() + filename = utils.force_encoding(filename, encoding) + basename = utils.force_encoding(basename, encoding) + if os.path.isabs(filename) and os.path.isdir(filename): + # We got an absolute directory from the user, so we save it under + # the default filename in that directory. + return os.path.join(filename, basename) + elif os.path.isabs(filename): + # We got an absolute filename from the user, so we save it under + # that filename. + return filename + return None + + +def ask_for_filename(suggested_filename, *, url, parent=None, + prompt_download_directory=None): + """Prepare a question for a download-path. + + If a filename can be determined directly, it is returned instead. + + Returns a (filename, question)-namedtuple, in which one component is + None. filename is a string, question is a usertypes.Question. The + question has a special .ask() method that takes no arguments for + convenience, as this function does not yet ask the question, it + only prepares it. + + Args: + suggested_filename: The "default"-name that is pre-entered as path. + url: The URL the download originated from. + parent: The parent of the question (a QObject). + prompt_download_directory: If this is something else than None, it + will overwrite the + storage->prompt-download-directory setting. + """ + if prompt_download_directory is None: + prompt_download_directory = config.get('storage', + 'prompt-download-directory') + + if not prompt_download_directory: + return _DownloadPath(filename=download_dir(), question=None) + + encoding = sys.getfilesystemencoding() + suggested_filename = utils.force_encoding(suggested_filename, encoding) + + q = usertypes.Question(parent) + q.title = "Save file to:" + q.text = "Please enter a location for {}".format( + html.escape(url.toDisplayString())) + q.mode = usertypes.PromptMode.text + q.completed.connect(q.deleteLater) + q.default = _path_suggestion(suggested_filename) + + q.ask = lambda: message.global_bridge.ask(q, blocking=False) + return _DownloadPath(filename=None, question=q) + + +class DownloadItemStats(QObject): + + """Statistics (bytes done, total bytes, time, etc.) about a download. + + Class attributes: + SPEED_AVG_WINDOW: How many seconds of speed data to average to + estimate the remaining time. + + Attributes: + done: How many bytes there are already downloaded. + total: The total count of bytes. None if the total is unknown. + speed: The current download speed, in bytes per second. + _speed_avg: A rolling average of speeds. + _last_done: The count of bytes which where downloaded when calculating + the speed the last time. + """ + + SPEED_AVG_WINDOW = 30 + + def __init__(self, parent=None): + super().__init__(parent) + self.total = None + self.done = 0 + self.speed = 0 + self._last_done = 0 + samples = int(self.SPEED_AVG_WINDOW * (1000 / _REFRESH_INTERVAL)) + self._speed_avg = collections.deque(maxlen=samples) + + def update_speed(self): + """Recalculate the current download speed. + + The caller needs to guarantee this is called all _REFRESH_INTERVAL ms. + """ + if self.done is None: + # this can happen for very fast downloads, e.g. when actually + # opening a file + return + delta = self.done - self._last_done + self.speed = delta * 1000 / _REFRESH_INTERVAL + self._speed_avg.append(self.speed) + self._last_done = self.done + + def finish(self): + """Set the download stats as finished.""" + self.done = self.total + + def percentage(self): + """The current download percentage, or None if unknown.""" + if self.done == self.total: + return 100 + elif self.total == 0 or self.total is None: + return None + else: + return 100 * self.done / self.total + + def remaining_time(self): + """The remaining download time in seconds, or None.""" + if self.total is None or not self._speed_avg: + # No average yet or we don't know the total size. + return None + remaining_bytes = self.total - self.done + avg = sum(self._speed_avg) / len(self._speed_avg) + if avg == 0: + # Download stalled + return None + else: + return remaining_bytes / avg + + @pyqtSlot('qint64', 'qint64') + def on_download_progress(self, bytes_done, bytes_total): + """Update local variables when the download progress changed. + + Args: + bytes_done: How many bytes are downloaded. + bytes_total: How many bytes there are to download in total. + """ + if bytes_total == -1: + bytes_total = None + self.done = bytes_done + self.total = bytes_total + + +class AbstractDownloadItem(QObject): + + """Shared QtNetwork/QtWebEngine part of a download item. + + FIXME + """ + + data_changed = pyqtSignal() + finished = pyqtSignal() + error = pyqtSignal(str) + cancelled = pyqtSignal() + remove_requested = pyqtSignal() + + def __init__(self, parent=None): + super().__init__(parent) + self.done = False + self.stats = DownloadItemStats(self) + self.index = 0 + self.error_msg = None + self.basename = '???' + self.successful = False + + self.autoclose = UnsupportedAttribute() + self.fileobj = UnsupportedAttribute() + self.raw_headers = UnsupportedAttribute() + + self._filename = None + self._dead = False + + def __repr__(self): + return utils.get_repr(self, basename=self.basename) + + def __str__(self): + """Get the download as a string. + + Example: foo.pdf [699.2kB/s|0.34|16%|4.253/25.124] + """ + speed = utils.format_size(self.stats.speed, suffix='B/s') + down = utils.format_size(self.stats.done, suffix='B') + perc = self.stats.percentage() + remaining = self.stats.remaining_time() + if self.error_msg is None: + errmsg = "" + else: + errmsg = " - {}".format(self.error_msg) + if all(e is None for e in [perc, remaining, self.stats.total]): + return ('{index}: {name} [{speed:>10}|{down}]{errmsg}'.format( + index=self.index, name=self.basename, speed=speed, + down=down, errmsg=errmsg)) + perc = round(perc) + if remaining is None: + remaining = '?' + else: + remaining = utils.format_seconds(remaining) + total = utils.format_size(self.stats.total, suffix='B') + if self.done: + return ('{index}: {name} [{perc:>2}%|{total}]{errmsg}'.format( + index=self.index, name=self.basename, perc=perc, + total=total, errmsg=errmsg)) + else: + return ('{index}: {name} [{speed:>10}|{remaining:>5}|{perc:>2}%|' + '{down}/{total}]{errmsg}'.format( + index=self.index, name=self.basename, speed=speed, + remaining=remaining, perc=perc, down=down, + total=total, errmsg=errmsg)) + + def _die(self, msg): + """Abort the download and emit an error.""" + assert not self.successful + # Prevent actions if calling _die() twice. + # + # For QtWebKit, this might happen if the error handler correctly + # connects, and the error occurs in _init_reply between + # reply.error.connect and the reply.error() check. In this case, the + # connected error handlers will be called twice, once via the direct + # error.emit() and once here in _die(). The stacks look like this then: + # + # -> on_reply_error -> _die -> + # self.error.emit() + # + # and + # + # [_init_reply -> ->] -> + # self.error.emit() + # + # which may lead to duplicate error messages (and failing tests) + if self._dead: + return + self._dead = True + self.error_msg = msg + self.stats.finish() + self.error.emit(msg) + self.done = True + self.data_changed.emit() + + def get_status_color(self, position): + """Choose an appropriate color for presenting the download's status. + + Args: + position: The color type requested, can be 'fg' or 'bg'. + """ + # pylint: disable=bad-config-call + # WORKAROUND for https://bitbucket.org/logilab/astroid/issue/104/ + assert position in ["fg", "bg"] + start = config.get('colors', 'downloads.{}.start'.format(position)) + stop = config.get('colors', 'downloads.{}.stop'.format(position)) + system = config.get('colors', 'downloads.{}.system'.format(position)) + error = config.get('colors', 'downloads.{}.error'.format(position)) + if self.error_msg is not None: + assert not self.successful + return error + elif self.stats.percentage() is None: + return start + else: + return utils.interpolate_color(start, stop, + self.stats.percentage(), system) + + def _do_cancel(self): + """Actual cancel implementation.""" + raise NotImplementedError + + @pyqtSlot() + def cancel(self, *, remove_data=True): + """Cancel the download. + + Args: + remove_data: Whether to remove the downloaded data. + """ + self._do_cancel() + log.downloads.debug("cancelled") + self.cancelled.emit() + if remove_data: + self.delete() + self.done = True + self.finished.emit() + self.data_changed.emit() + + @pyqtSlot() + def remove(self): + """Remove the download from the model.""" + self.remove_requested.emit() + + def delete(self): + """Delete the downloaded file.""" + try: + if self._filename is not None and os.path.exists(self._filename): + os.remove(self._filename) + log.downloads.debug("Deleted {}".format(self._filename)) + else: + log.downloads.debug("Not deleting {}".format(self._filename)) + except OSError: + log.downloads.exception("Failed to remove partial file") + + @pyqtSlot() + def retry(self): + """Retry a failed download.""" + raise NotImplementedError + + def _get_open_filename(self): + """Get the filename to open a download. + + Returns None if no suitable filename was found. + """ + raise NotImplementedError + + @pyqtSlot() + def open_file(self, cmdline=None): + """Open the downloaded file. + + Args: + cmdline: The command to use as string. A `{}` is expanded to the + filename. None means to use the system's default + application. If no `{}` is found, the filename is appended + to the cmdline. + """ + assert self.successful + filename = self._get_open_filename() + if filename is None: # pragma: no cover + log.downloads.error("No filename to open the download!") + return + + if cmdline is None: + log.downloads.debug("Opening {} with the system application" + .format(filename)) + url = QUrl.fromLocalFile(filename) + QDesktopServices.openUrl(url) + return + + cmd, *args = shlex.split(cmdline) + args = [arg.replace('{}', filename) for arg in args] + if '{}' not in cmdline: + args.append(filename) + log.downloads.debug("Opening {} with {}" + .format(filename, [cmd] + args)) + proc = guiprocess.GUIProcess(what='download') + proc.start_detached(cmd, args) + + def _ensure_can_set_filename(self, filename): + """Make sure we can still set a filename.""" + raise NotImplementedError + + def _after_set_filename(self): + """Finish initialization based on self._filename.""" + raise NotImplementedError + + def set_filename(self, filename): + """Set the filename to save the download to. + + Args: + filename: The full filename to save the download to. + None: special value to stop the download. + """ + global last_used_directory + filename = os.path.expanduser(filename) + self._ensure_can_set_filename(filename) + + self._filename = create_full_filename(self.basename, filename) + if self._filename is None: + # We only got a filename (without directory) or a relative path + # from the user, so we append that to the default directory and + # try again. + self._filename = create_full_filename( + self.basename, os.path.join(download_dir(), filename)) + + # At this point, we have a misconfigured XDG_DOWNLOAD_DIR, as + # download_dir() + filename is still no absolute path. + # The config value is checked for "absoluteness", but + # ~/.config/user-dirs.dirs may be misconfigured and a non-absolute path + # may be set for XDG_DOWNLOAD_DIR + if self._filename is None: + message.error( + "XDG_DOWNLOAD_DIR points to a relative path - please check" + " your ~/.config/user-dirs.dirs. The download is saved in" + " your home directory.", + ) + # fall back to $HOME as download_dir + self._filename = create_full_filename(self.basename, + os.path.expanduser('~')) + + self.basename = os.path.basename(self._filename) + last_used_directory = os.path.dirname(self._filename) + + log.downloads.debug("Setting filename to {}".format(filename)) + if os.path.isfile(self._filename): + # The file already exists, so ask the user if it should be + # overwritten. + txt = "{} already exists. Overwrite?".format( + html.escape(self._filename)) + self._ask_confirm_question("Overwrite existing file?", txt) + # FIFO, device node, etc. Make sure we want to do this + elif (os.path.exists(self._filename) and + not os.path.isdir(self._filename)): + txt = ("{} already exists and is a special file. Write to " + "it anyways?".format(html.escape(self._filename))) + self._ask_confirm_question("Overwrite special file?", txt) + else: + self._after_set_filename() diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index 1fe6d643b..82d8c499e 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -22,7 +22,6 @@ import io import os import sys -import shlex import os.path import shutil import functools @@ -33,218 +32,21 @@ import html import sip from PyQt5.QtCore import (pyqtSlot, pyqtSignal, QObject, QTimer, Qt, QAbstractListModel, QModelIndex, QUrl) -from PyQt5.QtGui import QDesktopServices from PyQt5.QtNetwork import QNetworkRequest, QNetworkReply from qutebrowser.config import config from qutebrowser.commands import cmdexc, cmdutils from qutebrowser.utils import (message, usertypes, log, utils, urlutils, objreg, standarddir, qtutils) -from qutebrowser.misc import guiprocess +from qutebrowser.browser import downloads from qutebrowser.browser.webkit import http from qutebrowser.browser.webkit.network import networkmanager -ModelRole = usertypes.enum('ModelRole', ['item'], start=Qt.UserRole, - is_int=True) - - _RetryInfo = collections.namedtuple('_RetryInfo', ['request', 'manager']) -_DownloadPath = collections.namedtuple('_DownloadPath', ['filename', - 'question']) -# Remember the last used directory -last_used_directory = None - - -# All REFRESH_INTERVAL milliseconds, speeds will be recalculated and downloads -# redrawn. -_REFRESH_INTERVAL = 500 - - -def download_dir(): - """Get the download directory to use.""" - directory = config.get('storage', 'download-directory') - remember_dir = config.get('storage', 'remember-download-directory') - - if remember_dir and last_used_directory is not None: - return last_used_directory - elif directory is None: - return standarddir.download() - else: - return directory - - -def _path_suggestion(filename): - """Get the suggested file path. - - Args: - filename: The filename to use if included in the suggestion. - """ - suggestion = config.get('completion', 'download-path-suggestion') - if suggestion == 'path': - # add trailing '/' if not present - return os.path.join(download_dir(), '') - elif suggestion == 'filename': - return filename - elif suggestion == 'both': - return os.path.join(download_dir(), filename) - else: # pragma: no cover - raise ValueError("Invalid suggestion value {}!".format(suggestion)) - - -def create_full_filename(basename, filename): - """Create a full filename based on the given basename and filename. - - Args: - basename: The basename to use if filename is a directory. - filename: The path to a folder or file where you want to save. - - Return: - The full absolute path, or None if filename creation was not possible. - """ - # Remove chars which can't be encoded in the filename encoding. - # See https://github.com/The-Compiler/qutebrowser/issues/427 - encoding = sys.getfilesystemencoding() - filename = utils.force_encoding(filename, encoding) - basename = utils.force_encoding(basename, encoding) - if os.path.isabs(filename) and os.path.isdir(filename): - # We got an absolute directory from the user, so we save it under - # the default filename in that directory. - return os.path.join(filename, basename) - elif os.path.isabs(filename): - # We got an absolute filename from the user, so we save it under - # that filename. - return filename - return None - - -def ask_for_filename(suggested_filename, *, url, parent=None, - prompt_download_directory=None): - """Prepare a question for a download-path. - - If a filename can be determined directly, it is returned instead. - - Returns a (filename, question)-namedtuple, in which one component is - None. filename is a string, question is a usertypes.Question. The - question has a special .ask() method that takes no arguments for - convenience, as this function does not yet ask the question, it - only prepares it. - - Args: - suggested_filename: The "default"-name that is pre-entered as path. - url: The URL the download originated from. - parent: The parent of the question (a QObject). - prompt_download_directory: If this is something else than None, it - will overwrite the - storage->prompt-download-directory setting. - """ - if prompt_download_directory is None: - prompt_download_directory = config.get('storage', - 'prompt-download-directory') - - if not prompt_download_directory: - return _DownloadPath(filename=download_dir(), question=None) - - encoding = sys.getfilesystemencoding() - suggested_filename = utils.force_encoding(suggested_filename, encoding) - - q = usertypes.Question(parent) - q.title = "Save file to:" - q.text = "Please enter a location for {}".format( - html.escape(url.toDisplayString())) - q.mode = usertypes.PromptMode.text - q.completed.connect(q.deleteLater) - q.default = _path_suggestion(suggested_filename) - - q.ask = lambda: message.global_bridge.ask(q, blocking=False) - return _DownloadPath(filename=None, question=q) - - -class DownloadItemStats(QObject): - - """Statistics (bytes done, total bytes, time, etc.) about a download. - - Class attributes: - SPEED_AVG_WINDOW: How many seconds of speed data to average to - estimate the remaining time. - - Attributes: - done: How many bytes there are already downloaded. - total: The total count of bytes. None if the total is unknown. - speed: The current download speed, in bytes per second. - _speed_avg: A rolling average of speeds. - _last_done: The count of bytes which where downloaded when calculating - the speed the last time. - """ - - SPEED_AVG_WINDOW = 30 - - def __init__(self, parent=None): - super().__init__(parent) - self.total = None - self.done = 0 - self.speed = 0 - self._last_done = 0 - samples = int(self.SPEED_AVG_WINDOW * (1000 / _REFRESH_INTERVAL)) - self._speed_avg = collections.deque(maxlen=samples) - - def update_speed(self): - """Recalculate the current download speed. - - The caller needs to guarantee this is called all _REFRESH_INTERVAL ms. - """ - if self.done is None: - # this can happen for very fast downloads, e.g. when actually - # opening a file - return - delta = self.done - self._last_done - self.speed = delta * 1000 / _REFRESH_INTERVAL - self._speed_avg.append(self.speed) - self._last_done = self.done - - def finish(self): - """Set the download stats as finished.""" - self.done = self.total - - def percentage(self): - """The current download percentage, or None if unknown.""" - if self.done == self.total: - return 100 - elif self.total == 0 or self.total is None: - return None - else: - return 100 * self.done / self.total - - def remaining_time(self): - """The remaining download time in seconds, or None.""" - if self.total is None or not self._speed_avg: - # No average yet or we don't know the total size. - return None - remaining_bytes = self.total - self.done - avg = sum(self._speed_avg) / len(self._speed_avg) - if avg == 0: - # Download stalled - return None - else: - return remaining_bytes / avg - - @pyqtSlot('qint64', 'qint64') - def on_download_progress(self, bytes_done, bytes_total): - """Update local variables when the download progress changed. - - Args: - bytes_done: How many bytes are downloaded. - bytes_total: How many bytes there are to download in total. - """ - if bytes_total == -1: - bytes_total = None - self.done = bytes_done - self.total = bytes_total - - -class DownloadItem(QObject): +class DownloadItem(downloads.AbstractDownloadItem): """A single download currently running. @@ -299,12 +101,7 @@ class DownloadItem(QObject): """ _MAX_REDIRECTS = 10 - data_changed = pyqtSignal() - finished = pyqtSignal() - error = pyqtSignal(str) - cancelled = pyqtSignal() adopt_download = pyqtSignal(object) # DownloadItem - remove_requested = pyqtSignal() def __init__(self, reply, win_id, parent=None): """Constructor. @@ -313,63 +110,22 @@ class DownloadItem(QObject): reply: The QNetworkReply to download. """ super().__init__(parent) - self._retry_info = None - self.done = False - self.stats = DownloadItemStats(self) - self.index = 0 self.autoclose = True + self.fileobj = None + self.raw_headers = {} + + self._filename = None + self._dead = False + + self._retry_info = None self._reply = None self._buffer = io.BytesIO() self._read_timer = usertypes.Timer(self, name='download-read-timer') self._read_timer.setInterval(500) self._read_timer.timeout.connect(self._on_read_timer_timeout) self._redirects = 0 - self.error_msg = None - self.basename = '???' - self.successful = False - self.fileobj = None - self._filename = None self._init_reply(reply) self._win_id = win_id - self.raw_headers = {} - self._dead = False - - def __repr__(self): - return utils.get_repr(self, basename=self.basename) - - def __str__(self): - """Get the download as a string. - - Example: foo.pdf [699.2kB/s|0.34|16%|4.253/25.124] - """ - speed = utils.format_size(self.stats.speed, suffix='B/s') - down = utils.format_size(self.stats.done, suffix='B') - perc = self.stats.percentage() - remaining = self.stats.remaining_time() - if self.error_msg is None: - errmsg = "" - else: - errmsg = " - {}".format(self.error_msg) - if all(e is None for e in [perc, remaining, self.stats.total]): - return ('{index}: {name} [{speed:>10}|{down}]{errmsg}'.format( - index=self.index, name=self.basename, speed=speed, - down=down, errmsg=errmsg)) - perc = round(perc) - if remaining is None: - remaining = '?' - else: - remaining = utils.format_seconds(remaining) - total = utils.format_size(self.stats.total, suffix='B') - if self.done: - return ('{index}: {name} [{perc:>2}%|{total}]{errmsg}'.format( - index=self.index, name=self.basename, perc=perc, - total=total, errmsg=errmsg)) - else: - return ('{index}: {name} [{speed:>10}|{remaining:>5}|{perc:>2}%|' - '{down}/{total}]{errmsg}'.format( - index=self.index, name=self.basename, speed=speed, - remaining=remaining, perc=perc, down=down, - total=total, errmsg=errmsg)) def _create_fileobj(self): """Create a file object using the internal filename.""" @@ -382,38 +138,21 @@ class DownloadItem(QObject): def _ask_confirm_question(self, title, msg): """Create a Question object to be asked.""" + # FIXME:qtwebengine move this? no_action = functools.partial(self.cancel, remove_data=False) message.confirm_async(title=title, text=msg, - yes_action=self._create_fileobj, + yes_action=self._after_set_filename, no_action=no_action, cancel_action=no_action, abort_on=[self.cancelled, self.error]) def _die(self, msg): """Abort the download and emit an error.""" - assert not self.successful - # Prevent actions if calling _die() twice. This might happen if the - # error handler correctly connects, and the error occurs in _init_reply - # between reply.error.connect and the reply.error() check. In this - # case, the connected error handlers will be called twice, once via the - # direct error.emit() and once here in _die(). The stacks look like - # this then: - # -> on_reply_error -> _die -> - # self.error.emit() - # and - # [_init_reply -> ->] -> - # self.error.emit() - # which may lead to duplicate error messages (and failing tests) - if self._dead: - return - self._dead = True + super()._die(msg) self._read_timer.stop() self._reply.downloadProgress.disconnect() self._reply.finished.disconnect() self._reply.error.disconnect() self._reply.readyRead.disconnect() - self.error_msg = msg - self.stats.finish() - self.error.emit(msg) with log.hide_qt_warning('QNetworkReplyImplPrivate::error: Internal ' 'problem, this method must only be called ' 'once.'): @@ -421,8 +160,6 @@ class DownloadItem(QObject): self._reply.abort() self._reply.deleteLater() self._reply = None - self.done = True - self.data_changed.emit() if self.fileobj is not None: try: self.fileobj.close() @@ -454,38 +191,7 @@ class DownloadItem(QObject): if reply.error() != QNetworkReply.NoError: QTimer.singleShot(0, lambda: self._die(reply.errorString())) - def get_status_color(self, position): - """Choose an appropriate color for presenting the download's status. - - Args: - position: The color type requested, can be 'fg' or 'bg'. - """ - # pylint: disable=bad-config-call - # WORKAROUND for https://bitbucket.org/logilab/astroid/issue/104/ - assert position in ["fg", "bg"] - start = config.get('colors', 'downloads.{}.start'.format(position)) - stop = config.get('colors', 'downloads.{}.stop'.format(position)) - system = config.get('colors', 'downloads.{}.system'.format(position)) - error = config.get('colors', 'downloads.{}.error'.format(position)) - if self.error_msg is not None: - assert not self.successful - return error - elif self.stats.percentage() is None: - return start - else: - return utils.interpolate_color(start, stop, - self.stats.percentage(), system) - - @pyqtSlot() - def cancel(self, *, remove_data=True): - """Cancel the download. - - Args: - remove_data: Whether to remove the downloaded data. - """ - log.downloads.debug("cancelled") - self._read_timer.stop() - self.cancelled.emit() + def _do_cancel(self): if self._reply is not None: self._reply.finished.disconnect(self._on_reply_finished) self._reply.abort() @@ -493,27 +199,6 @@ class DownloadItem(QObject): self._reply = None if self.fileobj is not None: self.fileobj.close() - if remove_data: - self.delete() - self.done = True - self.finished.emit() - self.data_changed.emit() - - @pyqtSlot() - def remove(self): - """Remove the download from the model.""" - self.remove_requested.emit() - - def delete(self): - """Delete the downloaded file.""" - try: - if self._filename is not None and os.path.exists(self._filename): - os.remove(self._filename) - log.downloads.debug("Deleted {}".format(self._filename)) - else: - log.downloads.debug("Not deleting {}".format(self._filename)) - except OSError: - log.downloads.exception("Failed to remove partial file") @pyqtSlot() def retry(self): @@ -528,94 +213,20 @@ class DownloadItem(QObject): self.adopt_download.emit(new_download) self.cancel() - @pyqtSlot() - def open_file(self, cmdline=None): - """Open the downloaded file. - - Args: - cmdline: The command to use as string. A `{}` is expanded to the - filename. None means to use the system's default - application. If no `{}` is found, the filename is appended - to the cmdline. - """ - assert self.successful + def _get_open_filename(self): filename = self._filename if filename is None: filename = getattr(self.fileobj, 'name', None) - if filename is None: # pragma: no cover - log.downloads.error("No filename to open the download!") - return + return filename - if cmdline is None: - log.downloads.debug("Opening {} with the system application" - .format(filename)) - url = QUrl.fromLocalFile(filename) - QDesktopServices.openUrl(url) - return - - cmd, *args = shlex.split(cmdline) - args = [arg.replace('{}', filename) for arg in args] - if '{}' not in cmdline: - args.append(filename) - log.downloads.debug("Opening {} with {}" - .format(filename, [cmd] + args)) - proc = guiprocess.GUIProcess(what='download') - proc.start_detached(cmd, args) - - def set_filename(self, filename): - """Set the filename to save the download to. - - Args: - filename: The full filename to save the download to. - None: special value to stop the download. - """ - global last_used_directory + def _ensure_can_set_filename(self, filename): if self.fileobj is not None: # pragma: no cover raise ValueError("fileobj was already set! filename: {}, " "existing: {}, fileobj {}".format( filename, self._filename, self.fileobj)) - filename = os.path.expanduser(filename) - self._filename = create_full_filename(self.basename, filename) - if self._filename is None: - # We only got a filename (without directory) or a relative path - # from the user, so we append that to the default directory and - # try again. - self._filename = create_full_filename( - self.basename, os.path.join(download_dir(), filename)) - # At this point, we have a misconfigured XDG_DOWNLOAD_DIR, as - # download_dir() + filename is still no absolute path. - # The config value is checked for "absoluteness", but - # ~/.config/user-dirs.dirs may be misconfigured and a non-absolute path - # may be set for XDG_DOWNLOAD_DIR - if self._filename is None: - message.error( - "XDG_DOWNLOAD_DIR points to a relative path - please check" - " your ~/.config/user-dirs.dirs. The download is saved in" - " your home directory.", - ) - # fall back to $HOME as download_dir - self._filename = create_full_filename(self.basename, - os.path.expanduser('~')) - - self.basename = os.path.basename(self._filename) - last_used_directory = os.path.dirname(self._filename) - - log.downloads.debug("Setting filename to {}".format(filename)) - if os.path.isfile(self._filename): - # The file already exists, so ask the user if it should be - # overwritten. - txt = "{} already exists. Overwrite?".format( - html.escape(self._filename)) - self._ask_confirm_question("Overwrite existing file?", txt) - # FIFO, device node, etc. Make sure we want to do this - elif (os.path.exists(self._filename) and - not os.path.isdir(self._filename)): - txt = ("{} already exists and is a special file. Write to " - "it anyways?".format(html.escape(self._filename))) - self._ask_confirm_question("Overwrite special file?", txt) - else: - self._create_fileobj() + def _after_set_filename(self): + self._create_fileobj() def set_fileobj(self, fileobj): """"Set the file object to write the download to. From 5b04f1052f930e40b312cfc887b71346e5eb7aea Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 16:28:47 +0100 Subject: [PATCH 09/44] Add DownloadItem.set_target This allows us to make _set_fileobj private, and also makes some code simpler. --- qutebrowser/browser/webkit/downloads.py | 94 ++++++++++++------------- 1 file changed, 44 insertions(+), 50 deletions(-) diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index 82d8c499e..958b6e122 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -134,7 +134,7 @@ class DownloadItem(downloads.AbstractDownloadItem): except OSError as e: self._die(e.strerror) else: - self.set_fileobj(fileobj) + self._set_fileobj(fileobj) def _ask_confirm_question(self, title, msg): """Create a Question object to be asked.""" @@ -228,7 +228,7 @@ class DownloadItem(downloads.AbstractDownloadItem): def _after_set_filename(self): self._create_fileobj() - def set_fileobj(self, fileobj): + def _set_fileobj(self, fileobj): """"Set the file object to write the download to. Args: @@ -380,6 +380,45 @@ class DownloadItem(downloads.AbstractDownloadItem): self._retry_info.manager is nam) return running_nam or retry_nam + def _open_if_successful(self, cmdline): + """Open the downloaded file, but only if it was successful. + + Args: + cmdline: Passed to DownloadItem.open_file(). + """ + if not self.successful: + log.downloads.debug("{} finished but not successful, not opening!" + .format(self)) + return + self.open_file(cmdline) + + def set_target(self, target): + """Set the target for a given download. + + Args: + target: The usertypes.DownloadTarget for this download. + """ + if isinstance(target, usertypes.FileObjDownloadTarget): + self._set_fileobj(target.fileobj) + self.autoclose = False + elif isinstance(target, usertypes.FileDownloadTarget): + self.set_filename(target.filename) + elif isinstance(target, usertypes.OpenFileDownloadTarget): + tmp_manager = objreg.get('temporary-downloads') + try: + fobj = tmp_manager.get_tmpfile(self.basename) + except OSError as exc: + msg = "Download error: {}".format(exc) + message.error(msg) + self.cancel() + return + self.finished.connect( + functools.partial(self._open_if_successful, target.cmdline)) + self.autoclose = True + self._set_fileobj(fobj) + else: # pragma: no cover + raise ValueError("Unknown download target: {}".format(target)) + class DownloadManager(QObject): @@ -564,7 +603,7 @@ class DownloadManager(QObject): self._update_timer.start() if target is not None: - self._set_download_target(download, suggested_filename, target) + download.set_target(target) return download # Neither filename nor fileobj were given, prepare a question @@ -576,14 +615,12 @@ class DownloadManager(QObject): # User doesn't want to be asked, so just use the download_dir if filename is not None: target = usertypes.FileDownloadTarget(filename) - self._set_download_target(download, suggested_filename, target) + download.set_target(target) return download # Ask the user for a filename self._postprocess_question(q) - q.answered.connect( - functools.partial(self._set_download_target, download, - suggested_filename)) + q.answered.connect(download.set_target) q.cancelled.connect(download.cancel) download.cancelled.connect(q.abort) download.error.connect(q.abort) @@ -591,49 +628,6 @@ class DownloadManager(QObject): return download - def _set_download_target(self, download, suggested_filename, target): - """Set the target for a given download. - - Args: - download: The download to set the filename for. - suggested_filename: The suggested filename. - target: The usertypes.DownloadTarget for this download. - """ - if isinstance(target, usertypes.FileObjDownloadTarget): - download.set_fileobj(target.fileobj) - download.autoclose = False - elif isinstance(target, usertypes.FileDownloadTarget): - download.set_filename(target.filename) - elif isinstance(target, usertypes.OpenFileDownloadTarget): - tmp_manager = objreg.get('temporary-downloads') - try: - fobj = tmp_manager.get_tmpfile(suggested_filename) - except OSError as exc: - msg = "Download error: {}".format(exc) - message.error(msg) - download.cancel() - return - download.finished.connect( - functools.partial(self._open_download, download, - target.cmdline)) - download.autoclose = True - download.set_fileobj(fobj) - else: # pragma: no cover - raise ValueError("Unknown download target: {}".format(target)) - - def _open_download(self, download, cmdline): - """Open the given download but only if it was successful. - - Args: - download: The DownloadItem to use. - cmdline: Passed to DownloadItem.open_file(). - """ - if not download.successful: - log.downloads.debug("{} finished but not successful, not opening!" - .format(download)) - return - download.open_file(cmdline) - @pyqtSlot(DownloadItem) def _on_data_changed(self, download): """Emit data_changed signal when download data changed.""" From 12d798d54da3c4f9637a7c12b967dc4395a4b01e Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 16:35:22 +0100 Subject: [PATCH 10/44] Continue download splitting --- qutebrowser/browser/downloads.py | 40 +++++++++++++++++++++++++ qutebrowser/browser/webkit/downloads.py | 23 ++------------ 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index b1cf085e1..25a5655e9 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -499,3 +499,43 @@ class AbstractDownloadItem(QObject): self._ask_confirm_question("Overwrite special file?", txt) else: self._after_set_filename() + + def _open_if_successful(self, cmdline): + """Open the downloaded file, but only if it was successful. + + Args: + cmdline: Passed to DownloadItem.open_file(). + """ + if not self.successful: + log.downloads.debug("{} finished but not successful, not opening!" + .format(self)) + return + self.open_file(cmdline) + + def set_target(self, target): + """Set the target for a given download. + + Args: + target: The usertypes.DownloadTarget for this download. + """ + raise NotImplementedError + + +class AbstractDownloadManager(QObject): + + """Backend-independent download manager code. + + Attributes: + downloads: A list of active DownloadItems. + questions: A list of Question objects to not GC them. + _networkmanager: A NetworkManager for generic downloads. + _win_id: The window ID the DownloadManager runs in. + + Signals: + begin_remove_rows: Emitted before downloads are removed. + end_remove_rows: Emitted after downloads are removed. + begin_insert_rows: Emitted before downloads are inserted. + end_insert_rows: Emitted after downloads are inserted. + data_changed: Emitted when the data of the model changed. + The arguments are int indices to the downloads. + """ diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index 958b6e122..a71ea6741 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -372,7 +372,7 @@ class DownloadItem(downloads.AbstractDownloadItem): old_reply.deleteLater() return True - def uses_nam(self, nam): + def _uses_nam(self, nam): """Check if this download uses the given QNetworkAccessManager.""" running_nam = self._reply is not None and self._reply.manager() is nam # user could request retry after tab is closed. @@ -380,24 +380,7 @@ class DownloadItem(downloads.AbstractDownloadItem): self._retry_info.manager is nam) return running_nam or retry_nam - def _open_if_successful(self, cmdline): - """Open the downloaded file, but only if it was successful. - - Args: - cmdline: Passed to DownloadItem.open_file(). - """ - if not self.successful: - log.downloads.debug("{} finished but not successful, not opening!" - .format(self)) - return - self.open_file(cmdline) - def set_target(self, target): - """Set the target for a given download. - - Args: - target: The usertypes.DownloadTarget for this download. - """ if isinstance(target, usertypes.FileObjDownloadTarget): self._set_fileobj(target.fileobj) self.autoclose = False @@ -417,7 +400,7 @@ class DownloadItem(downloads.AbstractDownloadItem): self.autoclose = True self._set_fileobj(fobj) else: # pragma: no cover - raise ValueError("Unknown download target: {}".format(target)) + raise ValueError("Unsupported download target: {}".format(target)) class DownloadManager(QObject): @@ -654,7 +637,7 @@ class DownloadManager(QObject): """ assert nam.adopted_downloads == 0 for download in self.downloads: - if download.uses_nam(nam): + if download._uses_nam(nam): # pylint: disable=protected-access nam.adopt_download(download) return nam.adopted_downloads From 92b1bf2227aa313f8949d2c4c4ced6b08d44acc5 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 16:37:56 +0100 Subject: [PATCH 11/44] Get rid of win_id for DownloadItem --- qutebrowser/browser/downloads.py | 1 - qutebrowser/browser/webkit/downloads.py | 18 +++++++----------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 25a5655e9..1f1dd147f 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -529,7 +529,6 @@ class AbstractDownloadManager(QObject): downloads: A list of active DownloadItems. questions: A list of Question objects to not GC them. _networkmanager: A NetworkManager for generic downloads. - _win_id: The window ID the DownloadManager runs in. Signals: begin_remove_rows: Emitted before downloads are removed. diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index a71ea6741..45ebfd437 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -83,7 +83,7 @@ class DownloadItem(downloads.AbstractDownloadItem): target file. _read_timer: A Timer which reads the QNetworkReply into self._buffer periodically. - _win_id: The window ID the DownloadItem runs in. + _manager: The DownloadManager which started this download _dead: Whether the Download has _die()'d. _reply: The QNetworkReply associated with this download. @@ -103,13 +103,13 @@ class DownloadItem(downloads.AbstractDownloadItem): _MAX_REDIRECTS = 10 adopt_download = pyqtSignal(object) # DownloadItem - def __init__(self, reply, win_id, parent=None): + def __init__(self, reply, manager): """Constructor. Args: reply: The QNetworkReply to download. """ - super().__init__(parent) + super().__init__(parent=manager) self.autoclose = True self.fileobj = None self.raw_headers = {} @@ -117,6 +117,7 @@ class DownloadItem(downloads.AbstractDownloadItem): self._filename = None self._dead = False + self._manager = manager self._retry_info = None self._reply = None self._buffer = io.BytesIO() @@ -125,7 +126,6 @@ class DownloadItem(downloads.AbstractDownloadItem): self._read_timer.timeout.connect(self._on_read_timer_timeout) self._redirects = 0 self._init_reply(reply) - self._win_id = win_id def _create_fileobj(self): """Create a file object using the internal filename.""" @@ -205,11 +205,9 @@ class DownloadItem(downloads.AbstractDownloadItem): """Retry a failed download.""" assert self.done assert not self.successful - download_manager = objreg.get('download-manager', scope='window', - window=self._win_id) new_reply = self._retry_info.manager.get(self._retry_info.request) - new_download = download_manager.fetch( - new_reply, suggested_filename=self.basename) + new_download = self._manager.fetch(new_reply, + suggested_filename=self.basename) self.adopt_download.emit(new_download) self.cancel() @@ -411,7 +409,6 @@ class DownloadManager(QObject): downloads: A list of active DownloadItems. questions: A list of Question objects to not GC them. _networkmanager: A NetworkManager for generic downloads. - _win_id: The window ID the DownloadManager runs in. Signals: begin_remove_rows: Emitted before downloads are removed. @@ -432,7 +429,6 @@ class DownloadManager(QObject): def __init__(self, win_id, parent=None): super().__init__(parent) - self._win_id = win_id self.downloads = [] self.questions = [] self._networkmanager = networkmanager.NetworkManager( @@ -560,7 +556,7 @@ class DownloadManager(QObject): _, suggested_filename = http.parse_content_disposition(reply) log.downloads.debug("fetch: {} -> {}".format(reply.url(), suggested_filename)) - download = DownloadItem(reply, self._win_id, self) + download = DownloadItem(reply, manager=self) download.cancelled.connect(download.remove) download.remove_requested.connect(functools.partial( self._remove_item, download)) From 3b51548d3a0192c6cdcebd50629d41c1c4213d44 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 17:06:56 +0100 Subject: [PATCH 12/44] More download splitting --- qutebrowser/browser/commands.py | 4 +- qutebrowser/browser/downloads.py | 480 +++++++++++++++++++++-- qutebrowser/browser/webkit/downloads.py | 490 +++--------------------- 3 files changed, 495 insertions(+), 479 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 2c0e01b5f..84285c9e1 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1355,8 +1355,8 @@ class CommandDispatcher: if dest is None: suggested_fn = self._current_title() + ".mht" suggested_fn = utils.sanitize_filename(suggested_fn) - filename, q = downloads.ask_for_filename(suggested_fn, parent=tab, - url=tab.url()) + filename, q = downloads.ask_for_filename_async( + suggested_fn, parent=tab, url=tab.url()) if filename is not None: mhtml.start_download_checked(filename, tab=tab) else: diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 1f1dd147f..37e23ecbb 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -24,8 +24,11 @@ import shlex import html import os.path import collections +import functools -from PyQt5.QtCore import pyqtSlot, pyqtSignal, Qt, QObject, QUrl +import sip +from PyQt5.QtCore import (pyqtSlot, pyqtSignal, Qt, QObject, QUrl, QModelIndex, + QTimer, QAbstractListModel) from PyQt5.QtGui import QDesktopServices from qutebrowser.config import config @@ -33,10 +36,6 @@ from qutebrowser.utils import usertypes, standarddir, utils, message, log from qutebrowser.misc import guiprocess -_DownloadPath = collections.namedtuple('_DownloadPath', ['filename', - 'question']) - - ModelRole = usertypes.enum('ModelRole', ['item'], start=Qt.UserRole, is_int=True) @@ -118,33 +117,14 @@ def create_full_filename(basename, filename): return None -def ask_for_filename(suggested_filename, *, url, parent=None, - prompt_download_directory=None): - """Prepare a question for a download-path. - - If a filename can be determined directly, it is returned instead. - - Returns a (filename, question)-namedtuple, in which one component is - None. filename is a string, question is a usertypes.Question. The - question has a special .ask() method that takes no arguments for - convenience, as this function does not yet ask the question, it - only prepares it. +def get_filename_question(*, suggested_filename, url, parent=None): + """Get a Question object for a download-path. Args: suggested_filename: The "default"-name that is pre-entered as path. url: The URL the download originated from. parent: The parent of the question (a QObject). - prompt_download_directory: If this is something else than None, it - will overwrite the - storage->prompt-download-directory setting. """ - if prompt_download_directory is None: - prompt_download_directory = config.get('storage', - 'prompt-download-directory') - - if not prompt_download_directory: - return _DownloadPath(filename=download_dir(), question=None) - encoding = sys.getfilesystemencoding() suggested_filename = utils.force_encoding(suggested_filename, encoding) @@ -155,9 +135,7 @@ def ask_for_filename(suggested_filename, *, url, parent=None, q.mode = usertypes.PromptMode.text q.completed.connect(q.deleteLater) q.default = _path_suggestion(suggested_filename) - - q.ask = lambda: message.global_bridge.ask(q, blocking=False) - return _DownloadPath(filename=None, question=q) + return q class DownloadItemStats(QObject): @@ -246,7 +224,27 @@ class AbstractDownloadItem(QObject): """Shared QtNetwork/QtWebEngine part of a download item. - FIXME + Attributes: + done: Whether the download is finished. + stats: A DownloadItemStats object. + index: The index of the download in the view. + successful: Whether the download has completed successfully. + error_msg: The current error message, or None + autoclose: Whether to close the associated file if the download is + done. + fileobj: The file object to download the file to. + raw_headers: The headers sent by the server. + _filename: The filename of the download. + _dead: Whether the Download has _die()'d. + + Signals: + data_changed: The downloads metadata changed. + finished: The download was finished. + cancelled: The download was cancelled. + error: An error with the download occurred. + arg: The error message as string. + remove_requested: Emitted when the removal of this download was + requested. """ data_changed = pyqtSignal() @@ -538,3 +536,425 @@ class AbstractDownloadManager(QObject): data_changed: Emitted when the data of the model changed. The arguments are int indices to the downloads. """ + + # parent, first, last + begin_remove_rows = pyqtSignal(QModelIndex, int, int) + end_remove_rows = pyqtSignal() + # parent, first, last + begin_insert_rows = pyqtSignal(QModelIndex, int, int) + end_insert_rows = pyqtSignal() + data_changed = pyqtSignal(int, int) # begin, end + + def __init__(self, parent=None): + super().__init__(parent) + self.downloads = [] + self.questions = [] + self._update_timer = usertypes.Timer(self, 'download-update') + self._update_timer.timeout.connect(self._update_gui) + self._update_timer.setInterval(_REFRESH_INTERVAL) + + def __repr__(self): + return utils.get_repr(self, downloads=len(self.downloads)) + + def _postprocess_question(self, q): + """Postprocess a Question object that is asked.""" + q.destroyed.connect(functools.partial(self.questions.remove, q)) + # We set the mode here so that other code that uses ask_for_filename + # doesn't need to handle the special download mode. + q.mode = usertypes.PromptMode.download + self.questions.append(q) + + @pyqtSlot() + def _update_gui(self): + """Periodical GUI update of all items.""" + assert self.downloads + for dl in self.downloads: + dl.stats.update_speed() + self.data_changed.emit(0, -1) + + def _init_item(self, download, auto_remove, suggested_filename): + """Initialize a newly created DownloadItem.""" + download.cancelled.connect(download.remove) + download.remove_requested.connect(functools.partial( + self._remove_item, download)) + + delay = config.get('ui', 'remove-finished-downloads') + if delay > -1: + download.finished.connect( + lambda: QTimer.singleShot(delay, download.remove)) + elif auto_remove: + download.finished.connect(download.remove) + + download.data_changed.connect( + functools.partial(self._on_data_changed, download)) + download.error.connect(self._on_error) + download.basename = suggested_filename + idx = len(self.downloads) + download.index = idx + 1 # "Human readable" index + self.begin_insert_rows.emit(QModelIndex(), idx, idx) + self.downloads.append(download) + self.end_insert_rows.emit() + + if not self._update_timer.isActive(): + self._update_timer.start() + + @pyqtSlot(AbstractDownloadItem) + def _on_data_changed(self, download): + """Emit data_changed signal when download data changed.""" + try: + idx = self.downloads.index(download) + except ValueError: + # download has been deleted in the meantime + return + self.data_changed.emit(idx, idx) + + @pyqtSlot(str) + def _on_error(self, msg): + """Display error message on download errors.""" + message.error("Download error: {}".format(msg)) + + @pyqtSlot(AbstractDownloadItem) + def _remove_item(self, download): + """Remove a given download.""" + if sip.isdeleted(self): + # https://github.com/The-Compiler/qutebrowser/issues/1242 + return + try: + idx = self.downloads.index(download) + except ValueError: + # already removed + return + self.begin_remove_rows.emit(QModelIndex(), idx, idx) + del self.downloads[idx] + self.end_remove_rows.emit() + download.deleteLater() + self._update_indexes() + if not self.downloads: + self._update_timer.stop() + log.downloads.debug("Removed download {}".format(download)) + + def _update_indexes(self): + """Update indexes of all DownloadItems.""" + first_idx = None + for i, d in enumerate(self.downloads, 1): + if first_idx is None and d.index != i: + first_idx = i - 1 + d.index = i + if first_idx is not None: + self.data_changed.emit(first_idx, -1) + + +class DownloadModel(QAbstractListModel): + + """A list model showing downloads.""" + + def __init__(self, downloader, parent=None): + super().__init__(parent) + self._downloader = downloader + # FIXME we'll need to translate indices here... + downloader.data_changed.connect(self._on_data_changed) + downloader.begin_insert_rows.connect(self.beginInsertRows) + downloader.end_insert_rows.connect(self.endInsertRows) + downloader.begin_remove_rows.connect(self.beginRemoveRows) + downloader.end_remove_rows.connect(self.endRemoveRows) + + def _all_downloads(self): + """Combine downloads from both downloaders.""" + return self._downloader.downloads[:] + + def __len__(self): + return len(self._all_downloads()) + + def __iter__(self): + return iter(self._all_downloads()) + + def __getitem__(self, idx): + return self._all_downloads()[idx] + + @pyqtSlot(int, int) + def _on_data_changed(self, start, end): + """Called when a downloader's data changed. + + Args: + start: The first changed index as int. + end: The last changed index as int, or -1 for all indices. + """ + # FIXME we'll need to translate indices here... + start_index = self.index(start, 0) + qtutils.ensure_valid(start_index) + if end == -1: + end_index = self.last_index() + else: + end_index = self.index(end, 0) + qtutils.ensure_valid(end_index) + self.dataChanged.emit(start_index, end_index) + + def _raise_no_download(self, count): + """Raise an exception that the download doesn't exist. + + Args: + count: The index of the download + """ + if not count: + raise cmdexc.CommandError("There's no download!") + raise cmdexc.CommandError("There's no download {}!".format(count)) + + @cmdutils.register(instance='download-model', scope='window') + @cmdutils.argument('count', count=True) + def download_cancel(self, all_=False, count=0): + """Cancel the last/[count]th download. + + Args: + all_: Cancel all running downloads + count: The index of the download to cancel. + """ + downloads = self._all_downloads() + if all_: + for download in downloads: + if not download.done: + download.cancel() + else: + try: + download = downloads[count - 1] + except IndexError: + self._raise_no_download(count) + if download.done: + if not count: + count = len(self) + raise cmdexc.CommandError("Download {} is already done!" + .format(count)) + download.cancel() + + @cmdutils.register(instance='download-model', scope='window') + @cmdutils.argument('count', count=True) + def download_delete(self, count=0): + """Delete the last/[count]th download from disk. + + Args: + count: The index of the download to delete. + """ + try: + download = self[count - 1] + except IndexError: + self._raise_no_download(count) + if not download.successful: + if not count: + count = len(self) + raise cmdexc.CommandError("Download {} is not done!".format(count)) + download.delete() + download.remove() + log.downloads.debug("deleted download {}".format(download)) + + @cmdutils.register(instance='download-model', scope='window', maxsplit=0) + @cmdutils.argument('count', count=True) + def download_open(self, cmdline: str=None, count=0): + """Open the last/[count]th download. + + If no specific command is given, this will use the system's default + application to open the file. + + Args: + cmdline: The command which should be used to open the file. A `{}` + is expanded to the temporary file name. If no `{}` is + present, the filename is automatically appended to the + cmdline. + count: The index of the download to open. + """ + try: + download = self[count - 1] + except IndexError: + self._raise_no_download(count) + if not download.successful: + if not count: + count = len(self) + raise cmdexc.CommandError("Download {} is not done!".format(count)) + download.open_file(cmdline) + + @cmdutils.register(instance='download-model', scope='window') + @cmdutils.argument('count', count=True) + def download_retry(self, count=0): + """Retry the first failed/[count]th download. + + Args: + count: The index of the download to retry. + """ + if count: + try: + download = self[count - 1] + except IndexError: + self._raise_no_download(count) + if download.successful or not download.done: + raise cmdexc.CommandError("Download {} did not fail!".format( + count)) + else: + to_retry = [d for d in self if d.done and not d.successful] + if not to_retry: + raise cmdexc.CommandError("No failed downloads!") + else: + download = to_retry[0] + download.retry() + + def can_clear(self): + """Check if there are finished downloads to clear.""" + return any(download.done for download in self) + + @cmdutils.register(instance='download-model', scope='window') + def download_clear(self): + """Remove all finished downloads from the list.""" + for download in self: + if download.done: + download.remove() + + @cmdutils.register(instance='download-model', scope='window') + @cmdutils.argument('count', count=True) + def download_remove(self, all_=False, count=0): + """Remove the last/[count]th download from the list. + + Args: + all_: Remove all finished downloads. + count: The index of the download to remove. + """ + if all_: + self.download_clear() + else: + try: + download = self[count - 1] + except IndexError: + self._raise_no_download(count) + if not download.done: + if not count: + count = len(self) + raise cmdexc.CommandError("Download {} is not done!" + .format(count)) + download.remove() + + def running_downloads(self): + """Return the amount of still running downloads. + + Return: + The number of unfinished downloads. + """ + return sum(1 for download in self if not download.done) + + def last_index(self): + """Get the last index in the model. + + Return: + A (possibly invalid) QModelIndex. + """ + idx = self.index(self.rowCount() - 1) + return idx + + def headerData(self, section, orientation, role=Qt.DisplayRole): + """Simple constant header.""" + if (section == 0 and orientation == Qt.Horizontal and + role == Qt.DisplayRole): + return "Downloads" + else: + return "" + + def data(self, index, role): + """Download data from DownloadManager.""" + if not index.isValid(): + return None + + if index.parent().isValid() or index.column() != 0: + return None + + item = self[index.row()] + if role == Qt.DisplayRole: + data = str(item) + elif role == Qt.ForegroundRole: + data = item.get_status_color('fg') + elif role == Qt.BackgroundRole: + data = item.get_status_color('bg') + elif role == ModelRole.item: + data = item + elif role == Qt.ToolTipRole: + if item.error_msg is None: + data = None + else: + return item.error_msg + else: + data = None + return data + + def flags(self, index): + """Override flags so items aren't selectable. + + The default would be Qt.ItemIsEnabled | Qt.ItemIsSelectable. + """ + if not index.isValid(): + return Qt.ItemFlags() + return Qt.ItemIsEnabled | Qt.ItemNeverHasChildren + + def rowCount(self, parent=QModelIndex()): + """Get count of active downloads.""" + if parent.isValid(): + # We don't have children + return 0 + return len(self) + + +class TempDownloadManager(QObject): + + """Manager to handle temporary download files. + + The downloads are downloaded to a temporary location and then openened with + the system standard application. The temporary files are deleted when + qutebrowser is shutdown. + + Attributes: + files: A list of NamedTemporaryFiles of downloaded items. + """ + + def __init__(self, parent=None): + super().__init__(parent) + self.files = [] + self._tmpdir = None + + def cleanup(self): + """Clean up any temporary files.""" + if self._tmpdir is not None: + try: + self._tmpdir.cleanup() + except OSError: + log.misc.exception("Failed to clean up temporary download " + "directory") + self._tmpdir = None + + def _get_tmpdir(self): + """Return the temporary directory that is used for downloads. + + The directory is created lazily on first access. + + Return: + The tempfile.TemporaryDirectory that is used. + """ + if self._tmpdir is None: + self._tmpdir = tempfile.TemporaryDirectory( + prefix='qutebrowser-downloads-') + return self._tmpdir + + def get_tmpfile(self, suggested_name): + """Return a temporary file in the temporary downloads directory. + + The files are kept as long as qutebrowser is running and automatically + cleaned up at program exit. + + Args: + suggested_name: str of the "suggested"/original filename. Used as a + suffix, so any file extenions are preserved. + + Return: + A tempfile.NamedTemporaryFile that should be used to save the file. + """ + tmpdir = self._get_tmpdir() + encoding = sys.getfilesystemencoding() + suggested_name = utils.force_encoding(suggested_name, encoding) + # Make sure that the filename is not too long + suggested_name = utils.elide_filename(suggested_name, 50) + fobj = tempfile.NamedTemporaryFile(dir=tmpdir.name, delete=False, + suffix=suggested_name) + self.files.append(fobj) + return fobj diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index 45ebfd437..91a038ffd 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -31,7 +31,7 @@ import html import sip from PyQt5.QtCore import (pyqtSlot, pyqtSignal, QObject, QTimer, - Qt, QAbstractListModel, QModelIndex, QUrl) + Qt, QModelIndex, QUrl) from PyQt5.QtNetwork import QNetworkRequest, QNetworkReply from qutebrowser.config import config @@ -46,6 +46,43 @@ from qutebrowser.browser.webkit.network import networkmanager _RetryInfo = collections.namedtuple('_RetryInfo', ['request', 'manager']) +_DownloadPath = collections.namedtuple('_DownloadPath', ['filename', + 'question']) + + +def ask_for_filename_async(suggested_filename, *, url, parent=None, + prompt_download_directory=None): + """Prepare a question for a download-path. + + If a filename can be determined directly, it is returned instead. + + Returns a (filename, question)-namedtuple, in which one component is + None. filename is a string, question is a usertypes.Question. The + question has a special .ask() method that takes no arguments for + convenience, as this function does not yet ask the question, it + only prepares it. + + Args: + suggested_filename: The "default"-name that is pre-entered as path. + url: The URL the download originated from. + parent: The parent of the question (a QObject). + prompt_download_directory: If this is something else than None, it + will overwrite the + storage->prompt-download-directory setting. + """ + if prompt_download_directory is None: + prompt_download_directory = config.get('storage', + 'prompt-download-directory') + + if not prompt_download_directory: + return _DownloadPath(filename=downloads.download_dir(), question=None) + + q = downloads.get_filename_question( + suggested_filename=suggested_filename, url=url, parent=parent) + q.ask = lambda: message.global_bridge.ask(q, blocking=False) + return _DownloadPath(filename=None, question=q) + + class DownloadItem(downloads.AbstractDownloadItem): """A single download currently running. @@ -67,37 +104,19 @@ class DownloadItem(downloads.AbstractDownloadItem): _MAX_REDIRECTS: The maximum redirection count. Attributes: - done: Whether the download is finished. - stats: A DownloadItemStats object. - index: The index of the download in the view. - successful: Whether the download has completed successfully. - error_msg: The current error message, or None - autoclose: Whether to close the associated file if the download is - done. - fileobj: The file object to download the file to. _retry_info: A _RetryInfo instance. - raw_headers: The headers sent by the server. - _filename: The filename of the download. _redirects: How many time we were redirected already. _buffer: A BytesIO object to buffer incoming data until we know the target file. _read_timer: A Timer which reads the QNetworkReply into self._buffer periodically. _manager: The DownloadManager which started this download - _dead: Whether the Download has _die()'d. _reply: The QNetworkReply associated with this download. Signals: - data_changed: The downloads metadata changed. - finished: The download was finished. - cancelled: The download was cancelled. - error: An error with the download occurred. - arg: The error message as string. adopt_download: Emitted when a download is retried and should be adopted by the QNAM if needed.. arg 0: The new DownloadItem - remove_requested: Emitted when the removal of this download was - requested. """ _MAX_REDIRECTS = 10 @@ -401,60 +420,18 @@ class DownloadItem(downloads.AbstractDownloadItem): raise ValueError("Unsupported download target: {}".format(target)) -class DownloadManager(QObject): +class DownloadManager(downloads.AbstractDownloadManager): """Manager for currently running downloads. Attributes: - downloads: A list of active DownloadItems. - questions: A list of Question objects to not GC them. _networkmanager: A NetworkManager for generic downloads. - - Signals: - begin_remove_rows: Emitted before downloads are removed. - end_remove_rows: Emitted after downloads are removed. - begin_insert_rows: Emitted before downloads are inserted. - end_insert_rows: Emitted after downloads are inserted. - data_changed: Emitted when the data of the model changed. - The arguments are int indices to the downloads. """ - # parent, first, last - begin_remove_rows = pyqtSignal(QModelIndex, int, int) - end_remove_rows = pyqtSignal() - # parent, first, last - begin_insert_rows = pyqtSignal(QModelIndex, int, int) - end_insert_rows = pyqtSignal() - data_changed = pyqtSignal(int, int) # begin, end - def __init__(self, win_id, parent=None): super().__init__(parent) - self.downloads = [] - self.questions = [] self._networkmanager = networkmanager.NetworkManager( win_id, None, self) - self._update_timer = usertypes.Timer(self, 'download-update') - self._update_timer.timeout.connect(self._update_gui) - self._update_timer.setInterval(_REFRESH_INTERVAL) - - def __repr__(self): - return utils.get_repr(self, downloads=len(self.downloads)) - - def _postprocess_question(self, q): - """Postprocess a Question object that is asked.""" - q.destroyed.connect(functools.partial(self.questions.remove, q)) - # We set the mode here so that other code that uses ask_for_filename - # doesn't need to handle the special download mode. - q.mode = usertypes.PromptMode.download - self.questions.append(q) - - @pyqtSlot() - def _update_gui(self): - """Periodical GUI update of all items.""" - assert self.downloads - for dl in self.downloads: - dl.stats.update_speed() - self.data_changed.emit(0, -1) @pyqtSlot('QUrl') def get(self, url, **kwargs): @@ -557,36 +534,14 @@ class DownloadManager(QObject): log.downloads.debug("fetch: {} -> {}".format(reply.url(), suggested_filename)) download = DownloadItem(reply, manager=self) - download.cancelled.connect(download.remove) - download.remove_requested.connect(functools.partial( - self._remove_item, download)) - - delay = config.get('ui', 'remove-finished-downloads') - if delay > -1: - download.finished.connect( - lambda: QTimer.singleShot(delay, download.remove)) - elif auto_remove: - download.finished.connect(download.remove) - - download.data_changed.connect( - functools.partial(self._on_data_changed, download)) - download.error.connect(self._on_error) - download.basename = suggested_filename - idx = len(self.downloads) - download.index = idx + 1 # "Human readable" index - self.begin_insert_rows.emit(QModelIndex(), idx, idx) - self.downloads.append(download) - self.end_insert_rows.emit() - - if not self._update_timer.isActive(): - self._update_timer.start() + self._init_item(download, auto_remove, suggested_filename) if target is not None: download.set_target(target) return download # Neither filename nor fileobj were given, prepare a question - filename, q = ask_for_filename( + filename, q = ask_for_filename_async( suggested_filename, parent=self, prompt_download_directory=prompt_download_directory, url=reply.url()) @@ -597,6 +552,8 @@ class DownloadManager(QObject): download.set_target(target) return download + ## FIXME + # Ask the user for a filename self._postprocess_question(q) q.answered.connect(download.set_target) @@ -607,21 +564,6 @@ class DownloadManager(QObject): return download - @pyqtSlot(DownloadItem) - def _on_data_changed(self, download): - """Emit data_changed signal when download data changed.""" - try: - idx = self.downloads.index(download) - except ValueError: - # download has been deleted in the meantime - return - self.data_changed.emit(idx, idx) - - @pyqtSlot(str) - def _on_error(self, msg): - """Display error message on download errors.""" - message.error("Download error: {}".format(msg)) - def has_downloads_with_nam(self, nam): """Check if the DownloadManager has any downloads with the given QNAM. @@ -636,349 +578,3 @@ class DownloadManager(QObject): if download._uses_nam(nam): # pylint: disable=protected-access nam.adopt_download(download) return nam.adopted_downloads - - @pyqtSlot(DownloadItem) - def _remove_item(self, download): - """Remove a given download.""" - if sip.isdeleted(self): - # https://github.com/The-Compiler/qutebrowser/issues/1242 - return - try: - idx = self.downloads.index(download) - except ValueError: - # already removed - return - self.begin_remove_rows.emit(QModelIndex(), idx, idx) - del self.downloads[idx] - self.end_remove_rows.emit() - download.deleteLater() - self._update_indexes() - if not self.downloads: - self._update_timer.stop() - log.downloads.debug("Removed download {}".format(download)) - - def _update_indexes(self): - """Update indexes of all DownloadItems.""" - first_idx = None - for i, d in enumerate(self.downloads, 1): - if first_idx is None and d.index != i: - first_idx = i - 1 - d.index = i - if first_idx is not None: - self.data_changed.emit(first_idx, -1) - - -class DownloadModel(QAbstractListModel): - - """A list model showing downloads.""" - - def __init__(self, downloader, parent=None): - super().__init__(parent) - self._downloader = downloader - # FIXME we'll need to translate indices here... - downloader.data_changed.connect(self._on_data_changed) - downloader.begin_insert_rows.connect(self.beginInsertRows) - downloader.end_insert_rows.connect(self.endInsertRows) - downloader.begin_remove_rows.connect(self.beginRemoveRows) - downloader.end_remove_rows.connect(self.endRemoveRows) - - def _all_downloads(self): - """Combine downloads from both downloaders.""" - return self._downloader.downloads[:] - - def __len__(self): - return len(self._all_downloads()) - - def __iter__(self): - return iter(self._all_downloads()) - - def __getitem__(self, idx): - return self._all_downloads()[idx] - - @pyqtSlot(int, int) - def _on_data_changed(self, start, end): - """Called when a downloader's data changed. - - Args: - start: The first changed index as int. - end: The last changed index as int, or -1 for all indices. - """ - # FIXME we'll need to translate indices here... - start_index = self.index(start, 0) - qtutils.ensure_valid(start_index) - if end == -1: - end_index = self.last_index() - else: - end_index = self.index(end, 0) - qtutils.ensure_valid(end_index) - self.dataChanged.emit(start_index, end_index) - - def _raise_no_download(self, count): - """Raise an exception that the download doesn't exist. - - Args: - count: The index of the download - """ - if not count: - raise cmdexc.CommandError("There's no download!") - raise cmdexc.CommandError("There's no download {}!".format(count)) - - @cmdutils.register(instance='download-model', scope='window') - @cmdutils.argument('count', count=True) - def download_cancel(self, all_=False, count=0): - """Cancel the last/[count]th download. - - Args: - all_: Cancel all running downloads - count: The index of the download to cancel. - """ - downloads = self._all_downloads() - if all_: - for download in downloads: - if not download.done: - download.cancel() - else: - try: - download = downloads[count - 1] - except IndexError: - self._raise_no_download(count) - if download.done: - if not count: - count = len(self) - raise cmdexc.CommandError("Download {} is already done!" - .format(count)) - download.cancel() - - @cmdutils.register(instance='download-model', scope='window') - @cmdutils.argument('count', count=True) - def download_delete(self, count=0): - """Delete the last/[count]th download from disk. - - Args: - count: The index of the download to delete. - """ - try: - download = self[count - 1] - except IndexError: - self._raise_no_download(count) - if not download.successful: - if not count: - count = len(self) - raise cmdexc.CommandError("Download {} is not done!".format(count)) - download.delete() - download.remove() - log.downloads.debug("deleted download {}".format(download)) - - @cmdutils.register(instance='download-model', scope='window', maxsplit=0) - @cmdutils.argument('count', count=True) - def download_open(self, cmdline: str=None, count=0): - """Open the last/[count]th download. - - If no specific command is given, this will use the system's default - application to open the file. - - Args: - cmdline: The command which should be used to open the file. A `{}` - is expanded to the temporary file name. If no `{}` is - present, the filename is automatically appended to the - cmdline. - count: The index of the download to open. - """ - try: - download = self[count - 1] - except IndexError: - self._raise_no_download(count) - if not download.successful: - if not count: - count = len(self) - raise cmdexc.CommandError("Download {} is not done!".format(count)) - download.open_file(cmdline) - - @cmdutils.register(instance='download-model', scope='window') - @cmdutils.argument('count', count=True) - def download_retry(self, count=0): - """Retry the first failed/[count]th download. - - Args: - count: The index of the download to retry. - """ - if count: - try: - download = self[count - 1] - except IndexError: - self._raise_no_download(count) - if download.successful or not download.done: - raise cmdexc.CommandError("Download {} did not fail!".format( - count)) - else: - to_retry = [d for d in self if d.done and not d.successful] - if not to_retry: - raise cmdexc.CommandError("No failed downloads!") - else: - download = to_retry[0] - download.retry() - - def can_clear(self): - """Check if there are finished downloads to clear.""" - return any(download.done for download in self) - - @cmdutils.register(instance='download-model', scope='window') - def download_clear(self): - """Remove all finished downloads from the list.""" - for download in self: - if download.done: - download.remove() - - @cmdutils.register(instance='download-model', scope='window') - @cmdutils.argument('count', count=True) - def download_remove(self, all_=False, count=0): - """Remove the last/[count]th download from the list. - - Args: - all_: Remove all finished downloads. - count: The index of the download to remove. - """ - if all_: - self.download_clear() - else: - try: - download = self[count - 1] - except IndexError: - self._raise_no_download(count) - if not download.done: - if not count: - count = len(self) - raise cmdexc.CommandError("Download {} is not done!" - .format(count)) - download.remove() - - def running_downloads(self): - """Return the amount of still running downloads. - - Return: - The number of unfinished downloads. - """ - return sum(1 for download in self if not download.done) - - def last_index(self): - """Get the last index in the model. - - Return: - A (possibly invalid) QModelIndex. - """ - idx = self.index(self.rowCount() - 1) - return idx - - def headerData(self, section, orientation, role=Qt.DisplayRole): - """Simple constant header.""" - if (section == 0 and orientation == Qt.Horizontal and - role == Qt.DisplayRole): - return "Downloads" - else: - return "" - - def data(self, index, role): - """Download data from DownloadManager.""" - if not index.isValid(): - return None - - if index.parent().isValid() or index.column() != 0: - return None - - item = self[index.row()] - if role == Qt.DisplayRole: - data = str(item) - elif role == Qt.ForegroundRole: - data = item.get_status_color('fg') - elif role == Qt.BackgroundRole: - data = item.get_status_color('bg') - elif role == ModelRole.item: - data = item - elif role == Qt.ToolTipRole: - if item.error_msg is None: - data = None - else: - return item.error_msg - else: - data = None - return data - - def flags(self, index): - """Override flags so items aren't selectable. - - The default would be Qt.ItemIsEnabled | Qt.ItemIsSelectable. - """ - if not index.isValid(): - return Qt.ItemFlags() - return Qt.ItemIsEnabled | Qt.ItemNeverHasChildren - - def rowCount(self, parent=QModelIndex()): - """Get count of active downloads.""" - if parent.isValid(): - # We don't have children - return 0 - return len(self) - - -class TempDownloadManager(QObject): - - """Manager to handle temporary download files. - - The downloads are downloaded to a temporary location and then openened with - the system standard application. The temporary files are deleted when - qutebrowser is shutdown. - - Attributes: - files: A list of NamedTemporaryFiles of downloaded items. - """ - - def __init__(self, parent=None): - super().__init__(parent) - self.files = [] - self._tmpdir = None - - def cleanup(self): - """Clean up any temporary files.""" - if self._tmpdir is not None: - try: - self._tmpdir.cleanup() - except OSError: - log.misc.exception("Failed to clean up temporary download " - "directory") - self._tmpdir = None - - def _get_tmpdir(self): - """Return the temporary directory that is used for downloads. - - The directory is created lazily on first access. - - Return: - The tempfile.TemporaryDirectory that is used. - """ - if self._tmpdir is None: - self._tmpdir = tempfile.TemporaryDirectory( - prefix='qutebrowser-downloads-') - return self._tmpdir - - def get_tmpfile(self, suggested_name): - """Return a temporary file in the temporary downloads directory. - - The files are kept as long as qutebrowser is running and automatically - cleaned up at program exit. - - Args: - suggested_name: str of the "suggested"/original filename. Used as a - suffix, so any file extenions are preserved. - - Return: - A tempfile.NamedTemporaryFile that should be used to save the file. - """ - tmpdir = self._get_tmpdir() - encoding = sys.getfilesystemencoding() - suggested_name = utils.force_encoding(suggested_name, encoding) - # Make sure that the filename is not too long - suggested_name = utils.elide_filename(suggested_name, 50) - fobj = tempfile.NamedTemporaryFile(dir=tmpdir.name, delete=False, - suffix=suggested_name) - self.files.append(fobj) - return fobj From 0ac2b713043a772ec47f8b883c1e570299f5272f Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 17:24:54 +0100 Subject: [PATCH 13/44] Simplify how filename questions are handled --- qutebrowser/browser/commands.py | 10 ++-- qutebrowser/browser/downloads.py | 28 +++++++---- qutebrowser/browser/webkit/downloads.py | 63 +++++-------------------- 3 files changed, 36 insertions(+), 65 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 84285c9e1..1f2ab4ba4 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1355,14 +1355,16 @@ class CommandDispatcher: if dest is None: suggested_fn = self._current_title() + ".mht" suggested_fn = utils.sanitize_filename(suggested_fn) - filename, q = downloads.ask_for_filename_async( - suggested_fn, parent=tab, url=tab.url()) + + filename = downloads.immediate_download_path() if filename is not None: mhtml.start_download_checked(filename, tab=tab) else: - q.answered.connect(functools.partial( + question = downloads.get_filename_question( + suggested_filename=suggested_fn, url=tab.url(), parent=tab) + question.answered.connect(functools.partial( mhtml.start_download_checked, tab=tab)) - q.ask() + message.global_bridge.ask(question, blocking=False) else: mhtml.start_download_checked(dest, tab=tab) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 37e23ecbb..7af3cfa07 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -73,6 +73,24 @@ def download_dir(): return directory +def immediate_download_path(prompt_download_directory=None): + """Try to get an immediate download path without asking the user. + + If that's possible, we return a path immediately. If not, None is returned. + + Args: + prompt_download_directory: If this is something else than None, it + will overwrite the + storage->prompt-download-directory setting. + """ + if prompt_download_directory is None: + prompt_download_directory = config.get('storage', + 'prompt-download-directory') + + if not prompt_download_directory: + return download_dir() + + def _path_suggestion(filename): """Get the suggested file path. @@ -525,7 +543,6 @@ class AbstractDownloadManager(QObject): Attributes: downloads: A list of active DownloadItems. - questions: A list of Question objects to not GC them. _networkmanager: A NetworkManager for generic downloads. Signals: @@ -548,7 +565,6 @@ class AbstractDownloadManager(QObject): def __init__(self, parent=None): super().__init__(parent) self.downloads = [] - self.questions = [] self._update_timer = usertypes.Timer(self, 'download-update') self._update_timer.timeout.connect(self._update_gui) self._update_timer.setInterval(_REFRESH_INTERVAL) @@ -556,14 +572,6 @@ class AbstractDownloadManager(QObject): def __repr__(self): return utils.get_repr(self, downloads=len(self.downloads)) - def _postprocess_question(self, q): - """Postprocess a Question object that is asked.""" - q.destroyed.connect(functools.partial(self.questions.remove, q)) - # We set the mode here so that other code that uses ask_for_filename - # doesn't need to handle the special download mode. - q.mode = usertypes.PromptMode.download - self.questions.append(q) - @pyqtSlot() def _update_gui(self): """Periodical GUI update of all items.""" diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index 91a038ffd..0855619b6 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -46,43 +46,6 @@ from qutebrowser.browser.webkit.network import networkmanager _RetryInfo = collections.namedtuple('_RetryInfo', ['request', 'manager']) -_DownloadPath = collections.namedtuple('_DownloadPath', ['filename', - 'question']) - - -def ask_for_filename_async(suggested_filename, *, url, parent=None, - prompt_download_directory=None): - """Prepare a question for a download-path. - - If a filename can be determined directly, it is returned instead. - - Returns a (filename, question)-namedtuple, in which one component is - None. filename is a string, question is a usertypes.Question. The - question has a special .ask() method that takes no arguments for - convenience, as this function does not yet ask the question, it - only prepares it. - - Args: - suggested_filename: The "default"-name that is pre-entered as path. - url: The URL the download originated from. - parent: The parent of the question (a QObject). - prompt_download_directory: If this is something else than None, it - will overwrite the - storage->prompt-download-directory setting. - """ - if prompt_download_directory is None: - prompt_download_directory = config.get('storage', - 'prompt-download-directory') - - if not prompt_download_directory: - return _DownloadPath(filename=downloads.download_dir(), question=None) - - q = downloads.get_filename_question( - suggested_filename=suggested_filename, url=url, parent=parent) - q.ask = lambda: message.global_bridge.ask(q, blocking=False) - return _DownloadPath(filename=None, question=q) - - class DownloadItem(downloads.AbstractDownloadItem): """A single download currently running. @@ -540,27 +503,25 @@ class DownloadManager(downloads.AbstractDownloadManager): download.set_target(target) return download - # Neither filename nor fileobj were given, prepare a question - filename, q = ask_for_filename_async( - suggested_filename, parent=self, - prompt_download_directory=prompt_download_directory, - url=reply.url()) + # Neither filename nor fileobj were given - # User doesn't want to be asked, so just use the download_dir + filename = downloads.immediate_download_path(prompt_download_directory) if filename is not None: + # User doesn't want to be asked, so just use the download_dir target = usertypes.FileDownloadTarget(filename) download.set_target(target) return download - ## FIXME - # Ask the user for a filename - self._postprocess_question(q) - q.answered.connect(download.set_target) - q.cancelled.connect(download.cancel) - download.cancelled.connect(q.abort) - download.error.connect(q.abort) - q.ask() + question = downloads.get_filename_question( + suggested_filename=suggested_filename, url=reply.url(), + parent=self) + question.mode = usertypes.PromptMode.download + question.answered.connect(download.set_target) + question.cancelled.connect(download.cancel) + download.cancelled.connect(question.abort) + download.error.connect(question.abort) + message.global_bridge.ask(question, blocking=False) return download From 7ca6996f395aa65d593b586478594ae4a1dc376d Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 17:31:39 +0100 Subject: [PATCH 14/44] Adjust imports --- qutebrowser/browser/downloads.py | 5 ++++- qutebrowser/browser/webkit/downloads.py | 13 ++----------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 7af3cfa07..c8cc988ac 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -25,14 +25,17 @@ import html import os.path import collections import functools +import tempfile import sip from PyQt5.QtCore import (pyqtSlot, pyqtSignal, Qt, QObject, QUrl, QModelIndex, QTimer, QAbstractListModel) from PyQt5.QtGui import QDesktopServices +from qutebrowser.commands import cmdexc, cmdutils from qutebrowser.config import config -from qutebrowser.utils import usertypes, standarddir, utils, message, log +from qutebrowser.utils import (usertypes, standarddir, utils, message, log, + qtutils) from qutebrowser.misc import guiprocess diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index 0855619b6..fa1c2bcaf 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -20,24 +20,15 @@ """Download manager.""" import io -import os -import sys import os.path import shutil import functools -import tempfile import collections -import html -import sip -from PyQt5.QtCore import (pyqtSlot, pyqtSignal, QObject, QTimer, - Qt, QModelIndex, QUrl) +from PyQt5.QtCore import pyqtSlot, pyqtSignal, QTimer from PyQt5.QtNetwork import QNetworkRequest, QNetworkReply -from qutebrowser.config import config -from qutebrowser.commands import cmdexc, cmdutils -from qutebrowser.utils import (message, usertypes, log, utils, urlutils, - objreg, standarddir, qtutils) +from qutebrowser.utils import message, usertypes, log, urlutils, objreg from qutebrowser.browser import downloads from qutebrowser.browser.webkit import http from qutebrowser.browser.webkit.network import networkmanager From e985730cbf2133d1c41d6ab289ecf8ea405dbfa6 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 17:35:53 +0100 Subject: [PATCH 15/44] Simplify TempDownloadManager initialization --- qutebrowser/app.py | 2 -- qutebrowser/browser/downloads.py | 8 +++++--- qutebrowser/browser/webkit/downloads.py | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/qutebrowser/app.py b/qutebrowser/app.py index 73485a634..6b49a1231 100644 --- a/qutebrowser/app.py +++ b/qutebrowser/app.py @@ -435,8 +435,6 @@ def _init_modules(args, crash_handler): os.environ['QT_WAYLAND_DISABLE_WINDOWDECORATION'] = '1' else: os.environ.pop('QT_WAYLAND_DISABLE_WINDOWDECORATION', None) - temp_downloads = downloads.TempDownloadManager(qApp) - objreg.register('temporary-downloads', temp_downloads) # Init backend-specific stuff browsertab.init(args) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index c8cc988ac..d05202e82 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -907,7 +907,7 @@ class DownloadModel(QAbstractListModel): return len(self) -class TempDownloadManager(QObject): +class TempDownloadManager: """Manager to handle temporary download files. @@ -919,8 +919,7 @@ class TempDownloadManager(QObject): files: A list of NamedTemporaryFiles of downloaded items. """ - def __init__(self, parent=None): - super().__init__(parent) + def __init__(self): self.files = [] self._tmpdir = None @@ -969,3 +968,6 @@ class TempDownloadManager(QObject): suffix=suggested_name) self.files.append(fobj) return fobj + + +temp_download_manager = TempDownloadManager() diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/webkit/downloads.py index fa1c2bcaf..9fd7165df 100644 --- a/qutebrowser/browser/webkit/downloads.py +++ b/qutebrowser/browser/webkit/downloads.py @@ -358,9 +358,9 @@ class DownloadItem(downloads.AbstractDownloadItem): elif isinstance(target, usertypes.FileDownloadTarget): self.set_filename(target.filename) elif isinstance(target, usertypes.OpenFileDownloadTarget): - tmp_manager = objreg.get('temporary-downloads') try: - fobj = tmp_manager.get_tmpfile(self.basename) + fobj = downloads.temp_download_manager.get_tmpfile( + self.basename) except OSError as exc: msg = "Download error: {}".format(exc) message.error(msg) From 990985e60f7ea1812d4711eb8e2948d82992ff37 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 17:38:33 +0100 Subject: [PATCH 16/44] Rename browser.webkit.downloads to browser.qtnetworkdownloads --- .../browser/{webkit/downloads.py => qtnetworkdownloads.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename qutebrowser/browser/{webkit/downloads.py => qtnetworkdownloads.py} (100%) diff --git a/qutebrowser/browser/webkit/downloads.py b/qutebrowser/browser/qtnetworkdownloads.py similarity index 100% rename from qutebrowser/browser/webkit/downloads.py rename to qutebrowser/browser/qtnetworkdownloads.py From 6eef79e180bbd83664f34c049ce950c4082d33f8 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 17:50:29 +0100 Subject: [PATCH 17/44] Adjust imports/objreg --- qutebrowser/app.py | 2 +- qutebrowser/browser/adblock.py | 4 ++-- qutebrowser/browser/commands.py | 8 ++++---- qutebrowser/browser/downloadview.py | 2 +- qutebrowser/browser/hints.py | 5 +++-- qutebrowser/browser/webkit/mhtml.py | 7 ++++--- qutebrowser/browser/webkit/webpage.py | 12 ++++++------ qutebrowser/commands/userscripts.py | 2 +- qutebrowser/mainwindow/mainwindow.py | 11 ++++++----- tests/unit/browser/test_adblock.py | 5 +++-- tests/unit/browser/webkit/test_downloads.py | 4 ++-- 11 files changed, 33 insertions(+), 29 deletions(-) diff --git a/qutebrowser/app.py b/qutebrowser/app.py index 6b49a1231..39d208ab2 100644 --- a/qutebrowser/app.py +++ b/qutebrowser/app.py @@ -46,7 +46,7 @@ from qutebrowser.completion.models import instances as completionmodels from qutebrowser.commands import cmdutils, runners, cmdexc from qutebrowser.config import style, config, websettings, configexc from qutebrowser.browser import urlmarks, adblock, history, browsertab -from qutebrowser.browser.webkit import cookies, cache, downloads +from qutebrowser.browser.webkit import cookies, cache from qutebrowser.browser.webkit.network import networkmanager from qutebrowser.mainwindow import mainwindow, prompt from qutebrowser.misc import (readline, ipc, savemanager, sessions, diff --git a/qutebrowser/browser/adblock.py b/qutebrowser/browser/adblock.py index 0069fe7e3..9a65231bb 100644 --- a/qutebrowser/browser/adblock.py +++ b/qutebrowser/browser/adblock.py @@ -190,8 +190,8 @@ class HostBlocker: self._blocked_hosts = set() self._done_count = 0 urls = config.get('content', 'host-block-lists') - download_manager = objreg.get('download-manager', scope='window', - window='last-focused') + download_manager = objreg.get('qtnetwork-download-manager', + scope='window', window='last-focused') if urls is None: return for url in urls: diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 1f2ab4ba4..606ed2ec1 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -43,8 +43,7 @@ import pygments.formatters from qutebrowser.commands import userscripts, cmdexc, cmdutils, runners from qutebrowser.config import config, configexc from qutebrowser.browser import (urlmarks, browsertab, inspector, navigate, - webelem) -from qutebrowser.browser.webkit import downloads + webelem, downloads) try: from qutebrowser.browser.webkit import mhtml except ImportError: @@ -1318,8 +1317,9 @@ class CommandDispatcher: " download.") dest = dest_old - download_manager = objreg.get('download-manager', scope='window', - window=self._win_id) + # FIXME:qtwebengine do this with the QtWebEngine download manager? + download_manager = objreg.get('qtnetwork-download-manager', + scope='window', window=self._win_id) if url: if mhtml_: raise cmdexc.CommandError("Can only download the current page" diff --git a/qutebrowser/browser/downloadview.py b/qutebrowser/browser/downloadview.py index c54b03e61..783a6a94d 100644 --- a/qutebrowser/browser/downloadview.py +++ b/qutebrowser/browser/downloadview.py @@ -25,7 +25,7 @@ import sip from PyQt5.QtCore import pyqtSlot, QSize, Qt, QTimer from PyQt5.QtWidgets import QListView, QSizePolicy, QMenu -from qutebrowser.browser.webkit import downloads +from qutebrowser.browser import downloads from qutebrowser.config import style from qutebrowser.utils import qtutils, utils, objreg diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py index 4d9407469..89f327fbf 100644 --- a/qutebrowser/browser/hints.py +++ b/qutebrowser/browser/hints.py @@ -288,8 +288,9 @@ class HintActions: qnam = elem._elem.webFrame().page().networkAccessManager() # pylint: enable=protected-access - download_manager = objreg.get('download-manager', scope='window', - window=self._win_id) + # FIXME:qtwebengine do this with QtWebEngine downloads? + download_manager = objreg.get('qtnetwork-download-manager', + scope='window', window=self._win_id) download_manager.get(url, qnam=qnam, prompt_download_directory=prompt) def call_userscript(self, elem, context): diff --git a/qutebrowser/browser/webkit/mhtml.py b/qutebrowser/browser/webkit/mhtml.py index 41d4e7355..1024f5238 100644 --- a/qutebrowser/browser/webkit/mhtml.py +++ b/qutebrowser/browser/webkit/mhtml.py @@ -34,7 +34,8 @@ import email.message from PyQt5.QtCore import QUrl -from qutebrowser.browser.webkit import webkitelem, downloads +from qutebrowser.browser import downloads +from qutebrowser.browser.webkit import webkitelem from qutebrowser.utils import log, objreg, message, usertypes, utils, urlutils _File = collections.namedtuple('_File', @@ -341,8 +342,8 @@ class _Downloader: self.writer.add_file(urlutils.encoded_url(url), b'') return - download_manager = objreg.get('download-manager', scope='window', - window=self._win_id) + download_manager = objreg.get('qtnetwork-download-manager', + scope='window', window=self._win_id) target = usertypes.FileObjDownloadTarget(_NoCloseBytesIO()) item = download_manager.get(url, target=target, auto_remove=True) diff --git a/qutebrowser/browser/webkit/webpage.py b/qutebrowser/browser/webkit/webpage.py index 00b253766..4634c0bcc 100644 --- a/qutebrowser/browser/webkit/webpage.py +++ b/qutebrowser/browser/webkit/webpage.py @@ -219,8 +219,8 @@ class BrowserPage(QWebPage): """Prepare the web page for being deleted.""" self._is_shutting_down = True self.shutting_down.emit() - download_manager = objreg.get('download-manager', scope='window', - window=self._win_id) + download_manager = objreg.get('qtnetwork-download-manager', + scope='window', window=self._win_id) nam = self.networkAccessManager() if download_manager.has_downloads_with_nam(nam): nam.setParent(download_manager) @@ -252,8 +252,8 @@ class BrowserPage(QWebPage): after this slot returns. """ req = QNetworkRequest(request) - download_manager = objreg.get('download-manager', scope='window', - window=self._win_id) + download_manager = objreg.get('qtnetwork-download-manager', + scope='window', window=self._win_id) download_manager.get_request(req, qnam=self.networkAccessManager()) @pyqtSlot('QNetworkReply*') @@ -267,8 +267,8 @@ class BrowserPage(QWebPage): here: http://mimesniff.spec.whatwg.org/ """ inline, suggested_filename = http.parse_content_disposition(reply) - download_manager = objreg.get('download-manager', scope='window', - window=self._win_id) + download_manager = objreg.get('qtnetwork-download-manager', + scope='window', window=self._win_id) if not inline: # Content-Disposition: attachment -> force download download_manager.fetch(reply, diff --git a/qutebrowser/commands/userscripts.py b/qutebrowser/commands/userscripts.py index de557f940..d939d6928 100644 --- a/qutebrowser/commands/userscripts.py +++ b/qutebrowser/commands/userscripts.py @@ -29,7 +29,7 @@ from qutebrowser.utils import message, log, objreg, standarddir from qutebrowser.commands import runners from qutebrowser.config import config from qutebrowser.misc import guiprocess -from qutebrowser.browser.webkit import downloads +from qutebrowser.browser import downloads class _QtFIFOReader(QObject): diff --git a/qutebrowser/mainwindow/mainwindow.py b/qutebrowser/mainwindow/mainwindow.py index 0e5a5c069..296a3a61b 100644 --- a/qutebrowser/mainwindow/mainwindow.py +++ b/qutebrowser/mainwindow/mainwindow.py @@ -35,8 +35,8 @@ from qutebrowser.mainwindow import tabbedbrowser, messageview, prompt from qutebrowser.mainwindow.statusbar import bar from qutebrowser.completion import completionwidget, completer from qutebrowser.keyinput import modeman -from qutebrowser.browser import commands, downloadview, hints -from qutebrowser.browser.webkit import downloads +from qutebrowser.browser import (commands, downloadview, hints, + qtnetworkdownloads, downloads) from qutebrowser.misc import crashsignal, keyhintwidget @@ -258,9 +258,10 @@ class MainWindow(QWidget): def _init_downloadmanager(self): log.init.debug("Initializing downloads...") - download_manager = downloads.DownloadManager(self.win_id, self) - objreg.register('download-manager', download_manager, scope='window', - window=self.win_id) + download_manager = qtnetworkdownloads.DownloadManager(self.win_id, + self) + objreg.register('qtnetwork-download-manager', download_manager, + scope='window', window=self.win_id) download_model = downloads.DownloadModel(download_manager) objreg.register('download-model', download_model, scope='window', window=self.win_id) diff --git a/tests/unit/browser/test_adblock.py b/tests/unit/browser/test_adblock.py index a1a44d5ec..b0285a8f5 100644 --- a/tests/unit/browser/test_adblock.py +++ b/tests/unit/browser/test_adblock.py @@ -101,10 +101,11 @@ class FakeDownloadManager: def download_stub(win_registry): """Register a FakeDownloadManager.""" stub = FakeDownloadManager() - objreg.register('download-manager', stub, + objreg.register('qtnetwork-download-manager', stub, scope='window', window='last-focused') yield - objreg.delete('download-manager', scope='window', window='last-focused') + objreg.delete('qtnetwork-download-manager', scope='window', + window='last-focused') def create_zipfile(directory, files, zipname='test'): diff --git a/tests/unit/browser/webkit/test_downloads.py b/tests/unit/browser/webkit/test_downloads.py index 44f1f73d3..498375700 100644 --- a/tests/unit/browser/webkit/test_downloads.py +++ b/tests/unit/browser/webkit/test_downloads.py @@ -18,12 +18,12 @@ # along with qutebrowser. If not, see . -from qutebrowser.browser.webkit import downloads +from qutebrowser.browser import downloads, qtnetworkdownloads def test_download_model(qapp, qtmodeltester, config_stub, cookiejar_and_cache): """Simple check for download model internals.""" config_stub.data = {'general': {'private-browsing': False}} - manager = downloads.DownloadManager(win_id=0) + manager = qtnetworkdownloads.DownloadManager(win_id=0) model = downloads.DownloadModel(manager) qtmodeltester.check(model) From a3a167e683cc0f388767429d8f5763238b8c8e39 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 18:02:54 +0100 Subject: [PATCH 18/44] Split _die in two methods We need to do some stuff before AbstractDownloadItem._die runs. --- qutebrowser/browser/downloads.py | 5 +++++ qutebrowser/browser/qtnetworkdownloads.py | 3 +-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index d05202e82..bcaab81a7 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -327,6 +327,10 @@ class AbstractDownloadItem(QObject): remaining=remaining, perc=perc, down=down, total=total, errmsg=errmsg)) + def _do_die(self): + """Do cleanup steps after a download has died.""" + raise NotImplementedError + def _die(self, msg): """Abort the download and emit an error.""" assert not self.successful @@ -350,6 +354,7 @@ class AbstractDownloadItem(QObject): if self._dead: return self._dead = True + self._do_die() self.error_msg = msg self.stats.finish() self.error.emit(msg) diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index 9fd7165df..6696a06c6 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -118,9 +118,8 @@ class DownloadItem(downloads.AbstractDownloadItem): no_action=no_action, cancel_action=no_action, abort_on=[self.cancelled, self.error]) - def _die(self, msg): + def _do_die(self): """Abort the download and emit an error.""" - super()._die(msg) self._read_timer.stop() self._reply.downloadProgress.disconnect() self._reply.finished.disconnect() From c876c3d2446e0d3b0e33978b3643758c48b0ea73 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 19:42:09 +0100 Subject: [PATCH 19/44] Fix lint --- .pylintrc | 3 ++- qutebrowser/app.py | 1 - qutebrowser/browser/qtnetworkdownloads.py | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.pylintrc b/.pylintrc index dda9e75ab..3f06facaf 100644 --- a/.pylintrc +++ b/.pylintrc @@ -75,4 +75,5 @@ ignored-modules=pytest # UnsetObject because pylint infers any objreg.get(...) as UnsetObject. ignored-classes=qutebrowser.utils.objreg.UnsetObject, qutebrowser.browser.webkit.webelem.WebElementWrapper, - scripts.dev.check_coverage.MsgType + scripts.dev.check_coverage.MsgType, + qutebrowser.browser.downloads.UnsupportedAttribute diff --git a/qutebrowser/app.py b/qutebrowser/app.py index 39d208ab2..11d7267f9 100644 --- a/qutebrowser/app.py +++ b/qutebrowser/app.py @@ -371,7 +371,6 @@ def _init_modules(args, crash_handler): args: The argparse namespace. crash_handler: The CrashHandler instance. """ - # pylint: disable=too-many-statements log.init.debug("Initializing prompts...") prompt.init() diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index 6696a06c6..38c6a353d 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -28,7 +28,7 @@ import collections from PyQt5.QtCore import pyqtSlot, pyqtSignal, QTimer from PyQt5.QtNetwork import QNetworkRequest, QNetworkReply -from qutebrowser.utils import message, usertypes, log, urlutils, objreg +from qutebrowser.utils import message, usertypes, log, urlutils from qutebrowser.browser import downloads from qutebrowser.browser.webkit import http from qutebrowser.browser.webkit.network import networkmanager @@ -68,8 +68,8 @@ class DownloadItem(downloads.AbstractDownloadItem): _reply: The QNetworkReply associated with this download. Signals: - adopt_download: Emitted when a download is retried and should be adopted - by the QNAM if needed.. + adopt_download: Emitted when a download is retried and should be + adopted by the QNAM if needed. arg 0: The new DownloadItem """ From bf994cd8daf6c9e23cb01c74f8abbf886e95a881 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 20:43:12 +0100 Subject: [PATCH 20/44] Initial QtWebEngine download support --- qutebrowser/browser/downloads.py | 42 +++++- qutebrowser/browser/qtnetworkdownloads.py | 29 +--- .../browser/webengine/webenginedownloads.py | 131 ++++++++++++++++++ qutebrowser/browser/webengine/webenginetab.py | 8 +- 4 files changed, 181 insertions(+), 29 deletions(-) create mode 100644 qutebrowser/browser/webengine/webenginedownloads.py diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index bcaab81a7..db4fd59dc 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -63,6 +63,11 @@ class UnsupportedAttribute: pass +class UnsupportedOperationError(Exception): + + """Raised when an operation is not supported with the given backend.""" + + def download_dir(): """Get the download directory to use.""" directory = config.get('storage', 'download-directory') @@ -396,7 +401,6 @@ class AbstractDownloadItem(QObject): """ self._do_cancel() log.downloads.debug("cancelled") - self.cancelled.emit() if remove_data: self.delete() self.done = True @@ -471,7 +475,15 @@ class AbstractDownloadItem(QObject): """Finish initialization based on self._filename.""" raise NotImplementedError - def set_filename(self, filename): + def _set_fileobj(self, fileobj): + """Set a file object to save the download to. + + Note that some backends (QtWebEngine) will simply access the .name + attribute and not actually use the file object directly. + """ + raise NotImplementedError + + def _set_filename(self, filename): """Set the filename to save the download to. Args: @@ -542,7 +554,23 @@ class AbstractDownloadItem(QObject): Args: target: The usertypes.DownloadTarget for this download. """ - raise NotImplementedError + if isinstance(target, usertypes.FileObjDownloadTarget): + raise UnsupportedAttribute("FileObjDownloadTarget is unsupported") + elif isinstance(target, usertypes.FileDownloadTarget): + self._set_filename(target.filename) + elif isinstance(target, usertypes.OpenFileDownloadTarget): + try: + fobj = temp_download_manager.get_tmpfile(self.basename) + except OSError as exc: + msg = "Download error: {}".format(exc) + message.error(msg) + self.cancel() + return + self.finished.connect( + functools.partial(self._open_if_successful, target.cmdline)) + self._set_fileobj(fobj) + else: # pragma: no cover + raise ValueError("Unsupported download target: {}".format(target)) class AbstractDownloadManager(QObject): @@ -659,6 +687,14 @@ class AbstractDownloadManager(QObject): if first_idx is not None: self.data_changed.emit(first_idx, -1) + def _init_filename_question(self, question, download): + """Set up an existing filename question with a download.""" + question.mode = usertypes.PromptMode.download + question.answered.connect(download.set_target) + question.cancelled.connect(download.cancel) + download.cancelled.connect(question.abort) + download.error.connect(question.abort) + class DownloadModel(QAbstractListModel): diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index 38c6a353d..e841f2aca 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -87,9 +87,6 @@ class DownloadItem(downloads.AbstractDownloadItem): self.fileobj = None self.raw_headers = {} - self._filename = None - self._dead = False - self._manager = manager self._retry_info = None self._reply = None @@ -171,6 +168,7 @@ class DownloadItem(downloads.AbstractDownloadItem): self._reply = None if self.fileobj is not None: self.fileobj.close() + self.cancelled.emit() @pyqtSlot() def retry(self): @@ -354,23 +352,8 @@ class DownloadItem(downloads.AbstractDownloadItem): if isinstance(target, usertypes.FileObjDownloadTarget): self._set_fileobj(target.fileobj) self.autoclose = False - elif isinstance(target, usertypes.FileDownloadTarget): - self.set_filename(target.filename) - elif isinstance(target, usertypes.OpenFileDownloadTarget): - try: - fobj = downloads.temp_download_manager.get_tmpfile( - self.basename) - except OSError as exc: - msg = "Download error: {}".format(exc) - message.error(msg) - self.cancel() - return - self.finished.connect( - functools.partial(self._open_if_successful, target.cmdline)) - self.autoclose = True - self._set_fileobj(fobj) - else: # pragma: no cover - raise ValueError("Unsupported download target: {}".format(target)) + else: + super().set_target(target) class DownloadManager(downloads.AbstractDownloadManager): @@ -506,11 +489,7 @@ class DownloadManager(downloads.AbstractDownloadManager): question = downloads.get_filename_question( suggested_filename=suggested_filename, url=reply.url(), parent=self) - question.mode = usertypes.PromptMode.download - question.answered.connect(download.set_target) - question.cancelled.connect(download.cancel) - download.cancelled.connect(question.abort) - download.error.connect(question.abort) + self._init_filename_question(question, download) message.global_bridge.ask(question, blocking=False) return download diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py new file mode 100644 index 000000000..1d1ef3e8e --- /dev/null +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -0,0 +1,131 @@ +# vim: ft=python fileencoding=utf-8 sts=4 sw=4 et: + +# Copyright 2014-2016 Florian Bruhin (The Compiler) +# +# This file is part of qutebrowser. +# +# qutebrowser is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# qutebrowser is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with qutebrowser. If not, see . + +"""QtWebEngine specific code for downloads.""" + + +from PyQt5.QtCore import pyqtSlot, Qt +from PyQt5.QtWebEngineWidgets import QWebEngineDownloadItem +from PyQt5.QtWidgets import QApplication + +from qutebrowser.browser import downloads +from qutebrowser.utils import debug, usertypes, message, log + + +class DownloadItem(downloads.AbstractDownloadItem): + + """A wrapper over a QWebEngineDownloadItem. + + Attributes: + _qt_item: The wrapped item. + """ + + def __init__(self, qt_item, parent=None): + super().__init__(parent) + self._qt_item = qt_item + qt_item.downloadProgress.connect(self.stats.on_download_progress) + qt_item.stateChanged.connect(self._on_state_changed) + + @pyqtSlot(QWebEngineDownloadItem.DownloadState) + def _on_state_changed(self, state): + state_name = debug.qenum_key(QWebEngineDownloadItem, state) + log.downloads.debug("State for {!r} changed to {}".format( + self, state_name)) + if state == QWebEngineDownloadItem.DownloadRequested: + pass + elif state == QWebEngineDownloadItem.DownloadInProgress: + pass + elif state == QWebEngineDownloadItem.DownloadCompleted: + self.successful = True + self.done = True + self.finished.emit() + elif state == QWebEngineDownloadItem.DownloadCancelled: + self.successful = False + self.done = True + self.cancelled.emit() + elif state == QWebEngineDownloadItem.DownloadInterrupted: + self.successful = False + self.done = True + # https://bugreports.qt.io/browse/QTBUG-56839 + self.error.emit("Download failed") + else: + raise ValueError("_on_state_changed was called with unknown state " + "{}".format(state_name)) + + def _do_die(self): + self._qt_item.downloadProgress.disconnect() + self._qt_item.cancel() + + def _do_cancel(self): + self._qt_item.cancel() + + def retry(self): + # https://bugreports.qt.io/browse/QTBUG-56840 + raise downloads.UnsupportedOperationError + + def get_open_filename(self): + return self._filename + + def _set_fileobj(self, fileobj): + self._set_filename(fileobj.name) + + def _ensure_can_set_filename(self, filename): + state = self._qt_item.state() + if state != QWebEngineDownloadItem.DownloadRequested: + state_name = debug.qenum_key(QWebEngineDownloadItem, state) + raise ValueError("Trying to set filename {} on {!r} which is state " + "{} (not in requested state)!".format( + filename, self, state_name)) + + def _after_set_filename(self): + self._qt_item.setPath(self._filename) + self._qt_item.accept() + + +class DownloadManager(downloads.AbstractDownloadManager): + + """Manager for currently running downloads.""" + + def install(self, profile): + """Set up the download manager on a QWebEngineProfile.""" + profile.downloadRequested.connect(self.handle_download, + Qt.DirectConnection) + + @pyqtSlot(QWebEngineDownloadItem) + def handle_download(self, qt_item): + download = DownloadItem(qt_item) + self._init_item(download, auto_remove=False, + suggested_filename=qt_item.path()) + + filename = downloads.immediate_download_path() + if filename is not None: + # User doesn't want to be asked, so just use the download_dir + target = usertypes.FileDownloadTarget(filename) + download.set_target(target) + return + + # Ask the user for a filename - needs to be blocking! + question = downloads.get_filename_question( + suggested_filename=qt_item.path(), url=qt_item.url(), + parent=self) + self._init_filename_question(question, download) + + message.global_bridge.ask(question, blocking=True) + # The filename is set via the question.answered signal, connected in + # _init_filename_question. diff --git a/qutebrowser/browser/webengine/webenginetab.py b/qutebrowser/browser/webengine/webenginetab.py index 8d1a1ac1c..5f0ea264a 100644 --- a/qutebrowser/browser/webengine/webenginetab.py +++ b/qutebrowser/browser/webengine/webenginetab.py @@ -34,7 +34,8 @@ from PyQt5.QtWebEngineWidgets import (QWebEnginePage, QWebEngineScript, from qutebrowser.browser import browsertab, mouse from qutebrowser.browser.webengine import (webview, webengineelem, tabhistory, - interceptor, webenginequtescheme) + interceptor, webenginequtescheme, + webenginedownloads) from qutebrowser.utils import (usertypes, qtutils, log, javascript, utils, objreg) @@ -61,6 +62,11 @@ def init(): host_blocker, parent=app) req_interceptor.install(profile) + log.init.debug("Initializing QtWebEngine downloads...") + download_manager = webenginedownloads.DownloadManager(parent=app) + download_manager.install(profile) + objreg.register('webengine-download-manager', download_manager) + # Mapping worlds from usertypes.JsWorld to QWebEngineScript world IDs. _JS_WORLD_MAP = { From ea9796403fe6f00995b5174ffef14b7093f10845 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 20:53:03 +0100 Subject: [PATCH 21/44] Fix _ask_confirm_question --- qutebrowser/browser/downloads.py | 4 ++++ qutebrowser/browser/qtnetworkdownloads.py | 16 +++++++--------- .../browser/webengine/webenginedownloads.py | 14 ++++++++++++++ 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index db4fd59dc..2df5d9731 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -475,6 +475,10 @@ class AbstractDownloadItem(QObject): """Finish initialization based on self._filename.""" raise NotImplementedError + def _ask_confirm_question(self, title, msg): + """Ask a confirmation question for the download.""" + raise NotImplementedError + def _set_fileobj(self, fileobj): """Set a file object to save the download to. diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index e841f2aca..d66f34162 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -106,15 +106,6 @@ class DownloadItem(downloads.AbstractDownloadItem): else: self._set_fileobj(fileobj) - def _ask_confirm_question(self, title, msg): - """Create a Question object to be asked.""" - # FIXME:qtwebengine move this? - no_action = functools.partial(self.cancel, remove_data=False) - message.confirm_async(title=title, text=msg, - yes_action=self._after_set_filename, - no_action=no_action, cancel_action=no_action, - abort_on=[self.cancelled, self.error]) - def _do_die(self): """Abort the download and emit an error.""" self._read_timer.stop() @@ -196,6 +187,13 @@ class DownloadItem(downloads.AbstractDownloadItem): def _after_set_filename(self): self._create_fileobj() + def _ask_confirm_question(self, title, msg): + no_action = functools.partial(self.cancel, remove_data=False) + message.confirm_async(title=title, text=msg, + yes_action=self._after_set_filename, + no_action=no_action, cancel_action=no_action, + abort_on=[self.cancelled, self.error]) + def _set_fileobj(self, fileobj): """"Set the file object to write the download to. diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index 1d1ef3e8e..77f970e88 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -19,6 +19,7 @@ """QtWebEngine specific code for downloads.""" +import functools from PyQt5.QtCore import pyqtSlot, Qt from PyQt5.QtWebEngineWidgets import QWebEngineDownloadItem @@ -93,6 +94,19 @@ class DownloadItem(downloads.AbstractDownloadItem): "{} (not in requested state)!".format( filename, self, state_name)) + def _ask_confirm_question(self, title, msg): + no_action = functools.partial(self.cancel, remove_data=False) + question = usertypes.Question() + question.title = title + question.text = msg + question.mode = usertypes.PromptMode.yesno + question.answered_yes.connect(self._after_set_filename) + question.answered_no.connect(no_action) + question.cancelled.connect(no_action) + self.cancelled.connect(question.abort) + self.error.connect(question.abort) + message.global_bridge.ask(question, blocking=True) + def _after_set_filename(self): self._qt_item.setPath(self._filename) self._qt_item.accept() From a5afdf6fb6dd5884c35b8fa12923d3373f095cce Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 22:26:14 +0100 Subject: [PATCH 22/44] Handle QtWebEngine downloads in DownloadModel --- qutebrowser/browser/downloads.py | 117 ++++++++++++++++++--------- qutebrowser/mainwindow/mainwindow.py | 17 +++- 2 files changed, 91 insertions(+), 43 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 2df5d9731..0a7396292 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -586,21 +586,19 @@ class AbstractDownloadManager(QObject): _networkmanager: A NetworkManager for generic downloads. Signals: - begin_remove_rows: Emitted before downloads are removed. - end_remove_rows: Emitted after downloads are removed. - begin_insert_rows: Emitted before downloads are inserted. - end_insert_rows: Emitted after downloads are inserted. + begin_remove_row: Emitted before downloads are removed. + end_remove_row: Emitted after downloads are removed. + begin_insert_row: Emitted before downloads are inserted. + end_insert_row: Emitted after downloads are inserted. data_changed: Emitted when the data of the model changed. - The arguments are int indices to the downloads. + The argument is the index of the changed download """ - # parent, first, last - begin_remove_rows = pyqtSignal(QModelIndex, int, int) - end_remove_rows = pyqtSignal() - # parent, first, last - begin_insert_rows = pyqtSignal(QModelIndex, int, int) - end_insert_rows = pyqtSignal() - data_changed = pyqtSignal(int, int) # begin, end + begin_remove_row = pyqtSignal(int) + end_remove_row = pyqtSignal() + begin_insert_row = pyqtSignal(int) + end_insert_row = pyqtSignal() + data_changed = pyqtSignal(int) def __init__(self, parent=None): super().__init__(parent) @@ -618,7 +616,7 @@ class AbstractDownloadManager(QObject): assert self.downloads for dl in self.downloads: dl.stats.update_speed() - self.data_changed.emit(0, -1) + self.data_changed.emit(-1) def _init_item(self, download, auto_remove, suggested_filename): """Initialize a newly created DownloadItem.""" @@ -639,9 +637,9 @@ class AbstractDownloadManager(QObject): download.basename = suggested_filename idx = len(self.downloads) download.index = idx + 1 # "Human readable" index - self.begin_insert_rows.emit(QModelIndex(), idx, idx) + self.begin_insert_row.emit(idx) self.downloads.append(download) - self.end_insert_rows.emit() + self.end_insert_row.emit() if not self._update_timer.isActive(): self._update_timer.start() @@ -654,7 +652,7 @@ class AbstractDownloadManager(QObject): except ValueError: # download has been deleted in the meantime return - self.data_changed.emit(idx, idx) + self.data_changed.emit(idx) @pyqtSlot(str) def _on_error(self, msg): @@ -672,9 +670,9 @@ class AbstractDownloadManager(QObject): except ValueError: # already removed return - self.begin_remove_rows.emit(QModelIndex(), idx, idx) + self.begin_remove_row.emit(idx) del self.downloads[idx] - self.end_remove_rows.emit() + self.end_remove_row.emit() download.deleteLater() self._update_indexes() if not self.downloads: @@ -683,13 +681,9 @@ class AbstractDownloadManager(QObject): def _update_indexes(self): """Update indexes of all DownloadItems.""" - first_idx = None for i, d in enumerate(self.downloads, 1): - if first_idx is None and d.index != i: - first_idx = i - 1 d.index = i - if first_idx is not None: - self.data_changed.emit(first_idx, -1) + self.data_changed.emit(-1) def _init_filename_question(self, question, download): """Set up an existing filename question with a download.""" @@ -704,19 +698,37 @@ class DownloadModel(QAbstractListModel): """A list model showing downloads.""" - def __init__(self, downloader, parent=None): + def __init__(self, qtnetwork_manager, webengine_manager=None, parent=None): super().__init__(parent) - self._downloader = downloader - # FIXME we'll need to translate indices here... - downloader.data_changed.connect(self._on_data_changed) - downloader.begin_insert_rows.connect(self.beginInsertRows) - downloader.end_insert_rows.connect(self.endInsertRows) - downloader.begin_remove_rows.connect(self.beginRemoveRows) - downloader.end_remove_rows.connect(self.endRemoveRows) + self._qtnetwork_manager = qtnetwork_manager + self._webengine_manager = webengine_manager + + qtnetwork_manager.data_changed.connect( + functools.partial(self._on_data_changed, webengine=False)) + qtnetwork_manager.begin_insert_row.connect( + functools.partial(self._on_begin_insert_row, webengine=False)) + qtnetwork_manager.begin_remove_row.connect( + functools.partial(self._on_begin_remove_row, webengine=False)) + qtnetwork_manager.end_insert_row.connect(self.endInsertRows) + qtnetwork_manager.end_remove_row.connect(self.endRemoveRows) + + if webengine_manager is not None: + webengine_manager.data_changed.connect( + functools.partial(self._on_data_changed, webengine=True)) + webengine_manager.begin_insert_row.connect( + functools.partial(self._on_begin_insert_row, webengine=True)) + webengine_manager.begin_remove_row.connect( + functools.partial(self._on_begin_remove_row, webengine=True)) + webengine_manager.end_insert_row.connect(self.endInsertRows) + webengine_manager.end_remove_row.connect(self.endRemoveRows) def _all_downloads(self): """Combine downloads from both downloaders.""" - return self._downloader.downloads[:] + if self._webengine_manager is None: + return self._qtnetwork_manager.downloads[:] + else: + return (self._qtnetwork_manager.downloads + + self._webengine_manager.downloads) def __len__(self): return len(self._all_downloads()) @@ -727,21 +739,48 @@ class DownloadModel(QAbstractListModel): def __getitem__(self, idx): return self._all_downloads()[idx] - @pyqtSlot(int, int) - def _on_data_changed(self, start, end): + def _on_begin_insert_row(self, idx, webengine=False): + log.downloads.debug("_on_begin_insert_row with idx {}, " + "webengine {}".format(idx, webengine)) + if idx == -1: + self.beginInsertRows(QModelIndex(), 0, -1) + return + + assert idx >= 0, idx + if webengine: + idx += len(self._qtnetwork_manager.downloads) + self.beginInsertRows(QModelIndex(), idx, idx) + + def _on_begin_remove_row(self, idx, webengine=False): + log.downloads.debug("_on_begin_remove_row with idx {}, " + "webengine {}".format(idx, webengine)) + if idx == -1: + self.beginRemoveRows(QModelIndex(), 0, -1) + return + + assert idx >= 0, idx + if webengine: + idx += len(self._qtnetwork_manager.downloads) + self.beginRemoveRows(QModelIndex(), idx, idx) + + def _on_data_changed(self, idx, *, webengine): """Called when a downloader's data changed. Args: start: The first changed index as int. end: The last changed index as int, or -1 for all indices. + webengine: If given, the QtNetwork download length is added to the + index. """ - # FIXME we'll need to translate indices here... - start_index = self.index(start, 0) - qtutils.ensure_valid(start_index) - if end == -1: + if idx == -1: + start_index = self.index(0, 0) end_index = self.last_index() else: - end_index = self.index(end, 0) + if webengine: + idx += len(self._qtnetwork_manager.downloads) + start_index = self.index(idx, 0) + end_index = self.index(idx, 0) + qtutils.ensure_valid(start_index) qtutils.ensure_valid(end_index) self.dataChanged.emit(start_index, end_index) diff --git a/qutebrowser/mainwindow/mainwindow.py b/qutebrowser/mainwindow/mainwindow.py index 296a3a61b..9e8f5a191 100644 --- a/qutebrowser/mainwindow/mainwindow.py +++ b/qutebrowser/mainwindow/mainwindow.py @@ -258,11 +258,20 @@ class MainWindow(QWidget): def _init_downloadmanager(self): log.init.debug("Initializing downloads...") - download_manager = qtnetworkdownloads.DownloadManager(self.win_id, - self) - objreg.register('qtnetwork-download-manager', download_manager, + qtnetwork_download_manager = qtnetworkdownloads.DownloadManager( + self.win_id, self) + objreg.register('qtnetwork-download-manager', + qtnetwork_download_manager, scope='window', window=self.win_id) - download_model = downloads.DownloadModel(download_manager) + + try: + webengine_download_manager = objreg.get( + 'webengine-download-manager') + except KeyError: + webengine_download_manager = None + + download_model = downloads.DownloadModel(qtnetwork_download_manager, + webengine_download_manager) objreg.register('download-model', download_model, scope='window', window=self.win_id) From fc6c6d4998051e6d68f830f39a83778703900af1 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 23:09:54 +0100 Subject: [PATCH 23/44] Fix lint --- qutebrowser/browser/downloads.py | 3 ++- qutebrowser/browser/webengine/webenginedownloads.py | 10 ++++++---- tests/end2end/features/test_downloads_bdd.py | 1 - 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 0a7396292..afb9bb62b 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -559,7 +559,8 @@ class AbstractDownloadItem(QObject): target: The usertypes.DownloadTarget for this download. """ if isinstance(target, usertypes.FileObjDownloadTarget): - raise UnsupportedAttribute("FileObjDownloadTarget is unsupported") + raise UnsupportedOperationError("FileObjDownloadTarget is " + "unsupported") elif isinstance(target, usertypes.FileDownloadTarget): self._set_filename(target.filename) elif isinstance(target, usertypes.OpenFileDownloadTarget): diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index 77f970e88..ebe4686b4 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -22,8 +22,9 @@ import functools from PyQt5.QtCore import pyqtSlot, Qt +# pylint: disable=no-name-in-module,import-error,useless-suppression from PyQt5.QtWebEngineWidgets import QWebEngineDownloadItem -from PyQt5.QtWidgets import QApplication +# pylint: enable=no-name-in-module,import-error,useless-suppression from qutebrowser.browser import downloads from qutebrowser.utils import debug, usertypes, message, log @@ -80,7 +81,7 @@ class DownloadItem(downloads.AbstractDownloadItem): # https://bugreports.qt.io/browse/QTBUG-56840 raise downloads.UnsupportedOperationError - def get_open_filename(self): + def _get_open_filename(self): return self._filename def _set_fileobj(self, fileobj): @@ -90,8 +91,8 @@ class DownloadItem(downloads.AbstractDownloadItem): state = self._qt_item.state() if state != QWebEngineDownloadItem.DownloadRequested: state_name = debug.qenum_key(QWebEngineDownloadItem, state) - raise ValueError("Trying to set filename {} on {!r} which is state " - "{} (not in requested state)!".format( + raise ValueError("Trying to set filename {} on {!r} which is " + "state {} (not in requested state)!".format( filename, self, state_name)) def _ask_confirm_question(self, title, msg): @@ -123,6 +124,7 @@ class DownloadManager(downloads.AbstractDownloadManager): @pyqtSlot(QWebEngineDownloadItem) def handle_download(self, qt_item): + """Start a download coming from a QWebEngineProfile.""" download = DownloadItem(qt_item) self._init_item(download, auto_remove=False, suggested_filename=qt_item.path()) diff --git a/tests/end2end/features/test_downloads_bdd.py b/tests/end2end/features/test_downloads_bdd.py index ac82aa04f..4b9476f04 100644 --- a/tests/end2end/features/test_downloads_bdd.py +++ b/tests/end2end/features/test_downloads_bdd.py @@ -21,7 +21,6 @@ import os import sys import shlex -import pytest import pytest_bdd as bdd bdd.scenarios('downloads.feature') From 5bc3914f24818c789f208ebfd5d8eb3af133d370 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 1 Nov 2016 23:19:39 +0100 Subject: [PATCH 24/44] Pass basename only as suggested filename --- qutebrowser/browser/webengine/webenginedownloads.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index ebe4686b4..687f4dd58 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -19,6 +19,7 @@ """QtWebEngine specific code for downloads.""" +import os.path import functools from PyQt5.QtCore import pyqtSlot, Qt @@ -125,9 +126,11 @@ class DownloadManager(downloads.AbstractDownloadManager): @pyqtSlot(QWebEngineDownloadItem) def handle_download(self, qt_item): """Start a download coming from a QWebEngineProfile.""" + suggested_filename = os.path.basename(qt_item.path()) + download = DownloadItem(qt_item) self._init_item(download, auto_remove=False, - suggested_filename=qt_item.path()) + suggested_filename=suggested_filename) filename = downloads.immediate_download_path() if filename is not None: @@ -138,7 +141,7 @@ class DownloadManager(downloads.AbstractDownloadManager): # Ask the user for a filename - needs to be blocking! question = downloads.get_filename_question( - suggested_filename=qt_item.path(), url=qt_item.url(), + suggested_filename=suggested_filename, url=qt_item.url(), parent=self) self._init_filename_question(question, download) From ce1b675a1ef02e78b9ff0beb0e0ca6b42b516f34 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Mon, 7 Nov 2016 10:59:25 +0100 Subject: [PATCH 25/44] Implement :download/hints via QtNetwork --- qutebrowser/browser/commands.py | 15 ++++++++++++--- qutebrowser/browser/hints.py | 11 +++++------ tests/end2end/features/test_downloads_bdd.py | 4 ---- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 606ed2ec1..78059d7ba 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1294,8 +1294,7 @@ class CommandDispatcher: except inspector.WebInspectorError as e: raise cmdexc.CommandError(e) - @cmdutils.register(instance='command-dispatcher', scope='window', - backend=usertypes.Backend.QtWebKit) + @cmdutils.register(instance='command-dispatcher', scope='window') @cmdutils.argument('dest_old', hide=True) def download(self, url=None, dest_old=None, *, mhtml_=False, dest=None): """Download a given URL, or current page if no URL given. @@ -1335,10 +1334,16 @@ class CommandDispatcher: self._download_mhtml(dest) else: tab = self._current_widget() + # FIXME:qtwebengine have a proper API for this # pylint: disable=protected-access - qnam = tab._widget.page().networkAccessManager() + try: + qnam = tab._widget.page().networkAccessManager() + except AttributeError: + # QtWebEngine + qnam = None # pylint: enable=protected-access + if dest is None: target = None else: @@ -1352,6 +1357,10 @@ class CommandDispatcher: dest: The file path to write the download to. """ tab = self._current_widget() + if tab.backend == usertypes.Backend.QtWebEngine: + raise cmdexc.CommandError("Download --mhtml is not implemented " + "with QtWebEngine yet") + if dest is None: suggested_fn = self._current_title() + ".mht" suggested_fn = utils.sanitize_filename(suggested_fn) diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py index 89f327fbf..e92a73642 100644 --- a/qutebrowser/browser/hints.py +++ b/qutebrowser/browser/hints.py @@ -285,7 +285,11 @@ class HintActions: # FIXME:qtwebengine get a proper API for this # pylint: disable=protected-access - qnam = elem._elem.webFrame().page().networkAccessManager() + try: + qnam = elem._elem.webFrame().page().networkAccessManager() + except AttributeError: + # QtWebEngine + qnam = None # pylint: enable=protected-access # FIXME:qtwebengine do this with QtWebEngine downloads? @@ -663,11 +667,6 @@ class HintManager(QObject): tab = tabbed_browser.currentWidget() if tab is None: raise cmdexc.CommandError("No WebView available yet!") - if (tab.backend == usertypes.Backend.QtWebEngine and - target == Target.download): - message.error("The download target is not available yet with " - "QtWebEngine.") - return mode_manager = objreg.get('mode-manager', scope='window', window=self._win_id) diff --git a/tests/end2end/features/test_downloads_bdd.py b/tests/end2end/features/test_downloads_bdd.py index 4b9476f04..65ccf1f3e 100644 --- a/tests/end2end/features/test_downloads_bdd.py +++ b/tests/end2end/features/test_downloads_bdd.py @@ -25,10 +25,6 @@ import pytest_bdd as bdd bdd.scenarios('downloads.feature') -pytestmark = pytest.mark.qtwebengine_todo("Downloads not implemented yet", - run=False) - - PROMPT_MSG = ("Asking question text=* " "title='Save file to:'>, *") From bc1e4385e04321f50e41f73f4ec5f389b8204e90 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Mon, 7 Nov 2016 11:42:02 +0100 Subject: [PATCH 26/44] Fix first bunch of download tests --- .../browser/webengine/webenginedownloads.py | 2 + tests/end2end/features/downloads.feature | 49 +++++++++++-------- 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index 687f4dd58..7e878e096 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -50,11 +50,13 @@ class DownloadItem(downloads.AbstractDownloadItem): state_name = debug.qenum_key(QWebEngineDownloadItem, state) log.downloads.debug("State for {!r} changed to {}".format( self, state_name)) + if state == QWebEngineDownloadItem.DownloadRequested: pass elif state == QWebEngineDownloadItem.DownloadInProgress: pass elif state == QWebEngineDownloadItem.DownloadCompleted: + log.downloads.debug("Download {} finished".format(self.basename)) self.successful = True self.done = True self.finished.emit() diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 9a868cd57..19a2081b3 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -75,6 +75,7 @@ Feature: Downloading things from a website. And I run :leave-mode Then no crash should happen + @qtwebengine_todo: ssl-strict is not implemented yet Scenario: Downloading with SSL errors (issue 1413) When I run :debug-clear-ssl-errors And I set network -> ssl-strict to ask @@ -85,7 +86,7 @@ Feature: Downloading things from a website. Scenario: Closing window with remove-finished-downloads timeout (issue 1242) When I set ui -> remove-finished-downloads to 500 - And I open data/downloads/download.bin in a new window + And I open data/downloads/download.bin in a new window without waiting And I wait until the download is finished And I run :close And I wait 0.5s @@ -95,7 +96,7 @@ Feature: Downloading things from a website. Given I have a fresh instance When I set storage -> prompt-download-directory to false And I set ui -> confirm-quit to downloads - And I open data/downloads/download.bin + And I open data/downloads/download.bin without waiting And I wait until the download is finished And I run :close Then qutebrowser should quit @@ -171,12 +172,14 @@ Feature: Downloading things from a website. ## mhtml downloads + @qtwebengine_todo: :download --mhtml is not implemented yet Scenario: Downloading as mhtml is available When I open html And I run :download --mhtml And I wait for "File successfully written." in the log Then no crash should happen + @qtwebengine_todo: :download --mhtml is not implemented yet Scenario: Downloading as mhtml with non-ASCII headers When I open response-headers?Content-Type=text%2Fpl%C3%A4in And I run :download --mhtml --dest mhtml-response-headers.mht @@ -199,13 +202,13 @@ Feature: Downloading things from a website. Then the error "There's no download 42!" should be shown Scenario: Cancelling a download which is already done - When I open data/downloads/download.bin + When I open data/downloads/download.bin without waiting And I wait until the download is finished And I run :download-cancel Then the error "Download 1 is already done!" should be shown Scenario: Cancelling a download which is already done (with count) - When I open data/downloads/download.bin + When I open data/downloads/download.bin without waiting And I wait until the download is finished And I run :download-cancel with count 1 Then the error "Download 1 is already done!" should be shown @@ -218,6 +221,7 @@ Feature: Downloading things from a website. And "cancelled" should be logged # https://github.com/The-Compiler/qutebrowser/issues/1535 + @qtwebengine_todo: :download --mhtml is not implemented yet Scenario: Cancelling an MHTML download (issue 1535) When I open data/downloads/issue1535.html And I run :download --mhtml @@ -228,7 +232,7 @@ Feature: Downloading things from a website. ## :download-remove / :download-clear Scenario: Removing a download - When I open data/downloads/download.bin + When I open data/downloads/download.bin without waiting And I wait until the download is finished And I run :download-remove Then "Removed download *" should be logged @@ -248,17 +252,17 @@ Feature: Downloading things from a website. Then the error "Download 1 is not done!" should be shown Scenario: Removing all downloads via :download-remove - When I open data/downloads/download.bin + When I open data/downloads/download.bin without waiting And I wait until the download is finished - And I open data/downloads/download2.bin + And I open data/downloads/download2.bin without waiting And I wait until the download is finished And I run :download-remove --all Then "Removed download *" should be logged Scenario: Removing all downloads via :download-clear - When I open data/downloads/download.bin + When I open data/downloads/download.bin without waiting And I wait until the download is finished - And I open data/downloads/download2.bin + And I open data/downloads/download2.bin without waiting And I wait until the download is finished And I run :download-clear Then "Removed download *" should be logged @@ -266,7 +270,7 @@ Feature: Downloading things from a website. ## :download-delete Scenario: Deleting a download - When I open data/downloads/download.bin + When I open data/downloads/download.bin without waiting And I wait until the download is finished And I run :download-delete And I wait for "deleted download *" in the log @@ -289,13 +293,13 @@ Feature: Downloading things from a website. ## :download-open Scenario: Opening a download - When I open data/downloads/download.bin + When I open data/downloads/download.bin without waiting And I wait until the download is finished And I open the download Then "Opening *download.bin* with [*python*]" should be logged Scenario: Opening a download with a placeholder - When I open data/downloads/download.bin + When I open data/downloads/download.bin without waiting And I wait until the download is finished And I open the download with a placeholder Then "Opening *download.bin* with [*python*]" should be logged @@ -318,7 +322,8 @@ Feature: Downloading things from a website. Scenario: Opening a download directly When I set storage -> prompt-download-directory to true - And I open data/downloads/download.bin + And I open data/downloads/download.bin without waiting + And I wait for the download prompt for "*" And I directly open the download And I wait until the download is finished Then "Opening *download.bin* with [*python*]" should be logged @@ -328,6 +333,7 @@ Feature: Downloading things from a website. Scenario: Cancelling a download that should be opened When I set storage -> prompt-download-directory to true And I run :download http://localhost:(port)/drip?numbytes=128&duration=5 + And I wait for the download prompt for "*" And I directly open the download And I run :download-cancel Then "* finished but not successful, not opening!" should be logged @@ -338,7 +344,7 @@ Feature: Downloading things from a website. When I set storage -> prompt-download-directory to true And I open data/downloads/issue1725.html And I run :click-element id long-link - And I wait for "Asking question text=* title='Save file to:'>, *" in the log + And I wait for the download prompt for "*" And I directly open the download And I wait until the download is finished Then "Opening * with [*python*]" should be logged @@ -348,19 +354,19 @@ Feature: Downloading things from a website. Scenario: completion -> download-path-suggestion = path When I set storage -> prompt-download-directory to true And I set completion -> download-path-suggestion to path - And I open data/downloads/download.bin + And I open data/downloads/download.bin without waiting Then the download prompt should be shown with "(tmpdir)/" Scenario: completion -> download-path-suggestion = filename When I set storage -> prompt-download-directory to true And I set completion -> download-path-suggestion to filename - And I open data/downloads/download.bin + And I open data/downloads/download.bin without waiting Then the download prompt should be shown with "download.bin" Scenario: completion -> download-path-suggestion = both When I set storage -> prompt-download-directory to true And I set completion -> download-path-suggestion to both - And I open data/downloads/download.bin + And I open data/downloads/download.bin without waiting Then the download prompt should be shown with "(tmpdir)/download.bin" ## storage -> remember-download-directory @@ -369,20 +375,20 @@ Feature: Downloading things from a website. When I set storage -> prompt-download-directory to true And I set completion -> download-path-suggestion to both And I set storage -> remember-download-directory to true - And I open data/downloads/download.bin + And I open data/downloads/download.bin without waiting And I wait for the download prompt for "*/download.bin" And I run :prompt-accept (tmpdir)(dirsep)subdir - And I open data/downloads/download2.bin + And I open data/downloads/download2.bin without waiting Then the download prompt should be shown with "(tmpdir)/subdir/download2.bin" Scenario: Not remembering the last download directory When I set storage -> prompt-download-directory to true And I set completion -> download-path-suggestion to both And I set storage -> remember-download-directory to false - And I open data/downloads/download.bin + And I open data/downloads/download.bin without waiting And I wait for the download prompt for "(tmpdir)/download.bin" And I run :prompt-accept (tmpdir)(dirsep)subdir - And I open data/downloads/download2.bin + And I open data/downloads/download2.bin without waiting Then the download prompt should be shown with "(tmpdir)/download2.bin" # Overwriting files @@ -475,6 +481,7 @@ Feature: Downloading things from a website. And I run :download foo! Then the error "Invalid URL" should be shown + @qtwebengine_todo: pdfjs is not implemented yet Scenario: Downloading via pdfjs Given pdfjs is available When I set storage -> prompt-download-directory to false From 53e360ec4b156c1e43ecceb6ef0787f64281c345 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Mon, 7 Nov 2016 13:33:21 +0100 Subject: [PATCH 27/44] Always use a global QNAM for downloads This makes a lot of code eaiser, and we don't have per-tab settings yet anyways. Also, with QtWebEngine, we can't honour any per-tab settings for downloads... --- qutebrowser/browser/commands.py | 11 +---- qutebrowser/browser/hints.py | 11 +---- qutebrowser/browser/qtnetworkdownloads.py | 40 ++----------------- .../browser/webkit/network/networkmanager.py | 28 ------------- qutebrowser/browser/webkit/webpage.py | 10 +---- 5 files changed, 7 insertions(+), 93 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 78059d7ba..5e8a9a9d2 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1335,20 +1335,11 @@ class CommandDispatcher: else: tab = self._current_widget() - # FIXME:qtwebengine have a proper API for this - # pylint: disable=protected-access - try: - qnam = tab._widget.page().networkAccessManager() - except AttributeError: - # QtWebEngine - qnam = None - # pylint: enable=protected-access - if dest is None: target = None else: target = usertypes.FileDownloadTarget(dest) - download_manager.get(self._current_url(), qnam=qnam, target=target) + download_manager.get(self._current_url(), target=target) def _download_mhtml(self, dest=None): """Download the current page as an MHTML file, including all assets. diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py index e92a73642..136b769b6 100644 --- a/qutebrowser/browser/hints.py +++ b/qutebrowser/browser/hints.py @@ -283,19 +283,10 @@ class HintActions: else: prompt = None - # FIXME:qtwebengine get a proper API for this - # pylint: disable=protected-access - try: - qnam = elem._elem.webFrame().page().networkAccessManager() - except AttributeError: - # QtWebEngine - qnam = None - # pylint: enable=protected-access - # FIXME:qtwebengine do this with QtWebEngine downloads? download_manager = objreg.get('qtnetwork-download-manager', scope='window', window=self._win_id) - download_manager.get(url, qnam=qnam, prompt_download_directory=prompt) + download_manager.get(url, prompt_download_directory=prompt) def call_userscript(self, elem, context): """Call a userscript from a hint. diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index d66f34162..a6e57e428 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -66,15 +66,9 @@ class DownloadItem(downloads.AbstractDownloadItem): periodically. _manager: The DownloadManager which started this download _reply: The QNetworkReply associated with this download. - - Signals: - adopt_download: Emitted when a download is retried and should be - adopted by the QNAM if needed. - arg 0: The new DownloadItem """ _MAX_REDIRECTS = 10 - adopt_download = pyqtSignal(object) # DownloadItem def __init__(self, reply, manager): """Constructor. @@ -167,9 +161,7 @@ class DownloadItem(downloads.AbstractDownloadItem): assert self.done assert not self.successful new_reply = self._retry_info.manager.get(self._retry_info.request) - new_download = self._manager.fetch(new_reply, - suggested_filename=self.basename) - self.adopt_download.emit(new_download) + self._manager.fetch(new_reply, suggested_filename=self.basename) self.cancel() def _get_open_filename(self): @@ -338,14 +330,6 @@ class DownloadItem(downloads.AbstractDownloadItem): old_reply.deleteLater() return True - def _uses_nam(self, nam): - """Check if this download uses the given QNetworkAccessManager.""" - running_nam = self._reply is not None and self._reply.manager() is nam - # user could request retry after tab is closed. - retry_nam = (self.done and (not self.successful) and - self._retry_info.manager is nam) - return running_nam or retry_nam - def set_target(self, target): if isinstance(target, usertypes.FileObjDownloadTarget): self._set_fileobj(target.fileobj) @@ -427,20 +411,17 @@ class DownloadManager(downloads.AbstractDownloadManager): suggested_filename=suggested_fn, **kwargs) - def _fetch_request(self, request, *, qnam=None, **kwargs): + def _fetch_request(self, request, **kwargs): """Download a QNetworkRequest to disk. Args: request: The QNetworkRequest to download. - qnam: The QNetworkAccessManager to use. **kwargs: passed to fetch(). Return: The created DownloadItem. """ - if qnam is None: - qnam = self._networkmanager - reply = qnam.get(request) + reply = self._networkmanager.get(request) return self.fetch(reply, **kwargs) @pyqtSlot('QNetworkReply') @@ -491,18 +472,3 @@ class DownloadManager(downloads.AbstractDownloadManager): message.global_bridge.ask(question, blocking=False) return download - - def has_downloads_with_nam(self, nam): - """Check if the DownloadManager has any downloads with the given QNAM. - - Args: - nam: The QNetworkAccessManager to check. - - Return: - A boolean. - """ - assert nam.adopted_downloads == 0 - for download in self.downloads: - if download._uses_nam(nam): # pylint: disable=protected-access - nam.adopt_download(download) - return nam.adopted_downloads diff --git a/qutebrowser/browser/webkit/network/networkmanager.py b/qutebrowser/browser/webkit/network/networkmanager.py index 6eb6c4ba7..81ce0eb79 100644 --- a/qutebrowser/browser/webkit/network/networkmanager.py +++ b/qutebrowser/browser/webkit/network/networkmanager.py @@ -135,11 +135,6 @@ class NetworkManager(QNetworkAccessManager): """Our own QNetworkAccessManager. Attributes: - adopted_downloads: If downloads are running with this QNAM but the - associated tab gets closed already, the NAM gets - reparented to the DownloadManager. This counts the - still running downloads, so the QNAM can clean - itself up when this reaches zero again. _requests: Pending requests. _scheme_handlers: A dictionary (scheme -> handler) of supported custom schemes. @@ -161,7 +156,6 @@ class NetworkManager(QNetworkAccessManager): # http://www.riverbankcomputing.com/pipermail/pyqt/2014-November/035045.html super().__init__(parent) log.init.debug("NetworkManager init done") - self.adopted_downloads = 0 self._win_id = win_id self._tab_id = tab_id self._requests = [] @@ -394,28 +388,6 @@ class NetworkManager(QNetworkAccessManager): # switched from private mode to normal mode self._set_cookiejar() - @pyqtSlot() - def on_adopted_download_destroyed(self): - """Check if we can clean up if an adopted download was destroyed. - - See the description for adopted_downloads for details. - """ - self.adopted_downloads -= 1 - log.downloads.debug("Adopted download destroyed, {} left.".format( - self.adopted_downloads)) - assert self.adopted_downloads >= 0 - if self.adopted_downloads == 0: - self.deleteLater() - - @pyqtSlot(object) # DownloadItem - def adopt_download(self, download): - """Adopt a new DownloadItem.""" - self.adopted_downloads += 1 - log.downloads.debug("Adopted download, {} adopted.".format( - self.adopted_downloads)) - download.destroyed.connect(self.on_adopted_download_destroyed) - download.adopt_download.connect(self.adopt_download) - def set_referer(self, req, current_url): """Set the referer header.""" referer_header_conf = config.get('network', 'referer-header') diff --git a/qutebrowser/browser/webkit/webpage.py b/qutebrowser/browser/webkit/webpage.py index 4634c0bcc..47adc0aa3 100644 --- a/qutebrowser/browser/webkit/webpage.py +++ b/qutebrowser/browser/webkit/webpage.py @@ -219,13 +219,7 @@ class BrowserPage(QWebPage): """Prepare the web page for being deleted.""" self._is_shutting_down = True self.shutting_down.emit() - download_manager = objreg.get('qtnetwork-download-manager', - scope='window', window=self._win_id) - nam = self.networkAccessManager() - if download_manager.has_downloads_with_nam(nam): - nam.setParent(download_manager) - else: - nam.shutdown() + self.networkAccessManager().shutdown() def display_content(self, reply, mimetype): """Display a QNetworkReply with an explicitly set mimetype.""" @@ -254,7 +248,7 @@ class BrowserPage(QWebPage): req = QNetworkRequest(request) download_manager = objreg.get('qtnetwork-download-manager', scope='window', window=self._win_id) - download_manager.get_request(req, qnam=self.networkAccessManager()) + download_manager.get_request(req) @pyqtSlot('QNetworkReply*') def on_unsupported_content(self, reply): From 54db4255b144ac13e4fe8782b81eb2a2a28af95c Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 07:21:21 +0100 Subject: [PATCH 28/44] Fix handling of temporary files When we use self._set_filename in self._set_fileobj, the file already exists, so we need to force "overwriting" it. Also, move temporary file handling to a dedicated _set_tempfile method, so we can officially claim not supporting _set_fileobj with QtWebEngine instead of supporting it with a hack. --- qutebrowser/browser/downloads.py | 16 +++++++++++----- qutebrowser/browser/qtnetworkdownloads.py | 3 +++ .../browser/webengine/webenginedownloads.py | 5 ++++- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index afb9bb62b..c83af523d 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -482,17 +482,21 @@ class AbstractDownloadItem(QObject): def _set_fileobj(self, fileobj): """Set a file object to save the download to. - Note that some backends (QtWebEngine) will simply access the .name - attribute and not actually use the file object directly. + Not supported by QtWebEngine. """ raise NotImplementedError - def _set_filename(self, filename): + def _set_tempfile(self, fileobj): + """Set a temporary file when opening the download.""" + raise NotImplementedError + + def _set_filename(self, filename, *, force_overwrite=False): """Set the filename to save the download to. Args: filename: The full filename to save the download to. None: special value to stop the download. + force_overwrite: Force overwriting existing files. """ global last_used_directory filename = os.path.expanduser(filename) @@ -525,7 +529,9 @@ class AbstractDownloadItem(QObject): last_used_directory = os.path.dirname(self._filename) log.downloads.debug("Setting filename to {}".format(filename)) - if os.path.isfile(self._filename): + if force_overwrite: + self._after_set_filename() + elif os.path.isfile(self._filename): # The file already exists, so ask the user if it should be # overwritten. txt = "{} already exists. Overwrite?".format( @@ -573,7 +579,7 @@ class AbstractDownloadItem(QObject): return self.finished.connect( functools.partial(self._open_if_successful, target.cmdline)) - self._set_fileobj(fobj) + self._set_tempfile(fobj) else: # pragma: no cover raise ValueError("Unsupported download target: {}".format(target)) diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index a6e57e428..5bcc67c0f 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -214,6 +214,9 @@ class DownloadItem(downloads.AbstractDownloadItem): except OSError as e: self._die(e.strerror) + def _set_tempfile(self, fileobj): + self._set_fileobj(fileobj) + def _finish_download(self): """Write buffered data to disk and finish the QNetworkReply.""" log.downloads.debug("Finishing download...") diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index 7e878e096..d43916f9d 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -88,7 +88,10 @@ class DownloadItem(downloads.AbstractDownloadItem): return self._filename def _set_fileobj(self, fileobj): - self._set_filename(fileobj.name) + raise downloads.UnsupportedOperationError + + def _set_tempfile(self, fileobj): + self._set_filename(fileobj.name, force_overwrite=True) def _ensure_can_set_filename(self, filename): state = self._qt_item.state() From ed3347365f6bbd8c8736a6e54365e20b07962c06 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 07:38:17 +0100 Subject: [PATCH 29/44] Fix lint --- qutebrowser/browser/commands.py | 2 -- qutebrowser/browser/qtnetworkdownloads.py | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 5e8a9a9d2..042e75a51 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1333,8 +1333,6 @@ class CommandDispatcher: elif mhtml_: self._download_mhtml(dest) else: - tab = self._current_widget() - if dest is None: target = None else: diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index 5bcc67c0f..10fd4dafc 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -25,7 +25,7 @@ import shutil import functools import collections -from PyQt5.QtCore import pyqtSlot, pyqtSignal, QTimer +from PyQt5.QtCore import pyqtSlot, QTimer from PyQt5.QtNetwork import QNetworkRequest, QNetworkReply from qutebrowser.utils import message, usertypes, log, urlutils From 19c7d747ddb0d01a34dac6a8d843a837353b216a Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 09:05:46 +0100 Subject: [PATCH 30/44] Fix downloads with unknown size with WebEngine --- qutebrowser/browser/downloads.py | 4 +++- tests/end2end/features/downloads.feature | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index c83af523d..8684c6c13 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -240,7 +240,7 @@ class DownloadItemStats(QObject): bytes_done: How many bytes are downloaded. bytes_total: How many bytes there are to download in total. """ - if bytes_total == -1: + if bytes_total in [0, -1]: # QtWebEngine, QtWebKit bytes_total = None self.done = bytes_done self.total = bytes_total @@ -311,10 +311,12 @@ class AbstractDownloadItem(QObject): errmsg = "" else: errmsg = " - {}".format(self.error_msg) + if all(e is None for e in [perc, remaining, self.stats.total]): return ('{index}: {name} [{speed:>10}|{down}]{errmsg}'.format( index=self.index, name=self.basename, speed=speed, down=down, errmsg=errmsg)) + perc = round(perc) if remaining is None: remaining = '?' diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 19a2081b3..aaf87dcf3 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -503,3 +503,9 @@ Feature: Downloading things from a website. And I wait until the download is finished Then the downloaded file download.bin should exist And the downloaded file download2.bin should not exist + + Scenario: Downloading a file with unknown size + When I set storage -> prompt-download-directory to false + And I open stream-bytes/1024 without waiting + And I wait until the download is finished + Then the downloaded file 1024 should exist From b00c889dd176ad091398e410c0801cd34d613db0 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 20:33:25 +0100 Subject: [PATCH 31/44] Call _set_fileobj in AbstractDownloadItem.set_target --- qutebrowser/browser/downloads.py | 9 ++++++--- qutebrowser/browser/qtnetworkdownloads.py | 10 ++-------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 8684c6c13..a4018aa04 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -481,10 +481,14 @@ class AbstractDownloadItem(QObject): """Ask a confirmation question for the download.""" raise NotImplementedError - def _set_fileobj(self, fileobj): + def _set_fileobj(self, fileobj, *, autoclose=True): """Set a file object to save the download to. Not supported by QtWebEngine. + + Args: + fileobj: The file object to download to. + autoclose: Close the file object automatically when it's done. """ raise NotImplementedError @@ -567,8 +571,7 @@ class AbstractDownloadItem(QObject): target: The usertypes.DownloadTarget for this download. """ if isinstance(target, usertypes.FileObjDownloadTarget): - raise UnsupportedOperationError("FileObjDownloadTarget is " - "unsupported") + self._set_fileobj(target.fileobj, autoclose=False) elif isinstance(target, usertypes.FileDownloadTarget): self._set_filename(target.filename) elif isinstance(target, usertypes.OpenFileDownloadTarget): diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index 10fd4dafc..fd62c8ffc 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -186,7 +186,7 @@ class DownloadItem(downloads.AbstractDownloadItem): no_action=no_action, cancel_action=no_action, abort_on=[self.cancelled, self.error]) - def _set_fileobj(self, fileobj): + def _set_fileobj(self, fileobj, *, autoclose=True): """"Set the file object to write the download to. Args: @@ -196,6 +196,7 @@ class DownloadItem(downloads.AbstractDownloadItem): raise ValueError("fileobj was already set! Old: {}, new: " "{}".format(self.fileobj, fileobj)) self.fileobj = fileobj + self.autoclose = autoclose try: self._read_timer.stop() log.downloads.debug("buffer: {} bytes".format(self._buffer.tell())) @@ -333,13 +334,6 @@ class DownloadItem(downloads.AbstractDownloadItem): old_reply.deleteLater() return True - def set_target(self, target): - if isinstance(target, usertypes.FileObjDownloadTarget): - self._set_fileobj(target.fileobj) - self.autoclose = False - else: - super().set_target(target) - class DownloadManager(downloads.AbstractDownloadManager): From de1e3a7a54b1841faf0296599c28fbcfa62e666b Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 20:35:07 +0100 Subject: [PATCH 32/44] Make DownloadItem._autoclose private --- qutebrowser/browser/downloads.py | 3 --- qutebrowser/browser/qtnetworkdownloads.py | 8 +++++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index a4018aa04..be4d3285b 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -256,8 +256,6 @@ class AbstractDownloadItem(QObject): index: The index of the download in the view. successful: Whether the download has completed successfully. error_msg: The current error message, or None - autoclose: Whether to close the associated file if the download is - done. fileobj: The file object to download the file to. raw_headers: The headers sent by the server. _filename: The filename of the download. @@ -288,7 +286,6 @@ class AbstractDownloadItem(QObject): self.basename = '???' self.successful = False - self.autoclose = UnsupportedAttribute() self.fileobj = UnsupportedAttribute() self.raw_headers = UnsupportedAttribute() diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index fd62c8ffc..b9aaeea8c 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -66,6 +66,8 @@ class DownloadItem(downloads.AbstractDownloadItem): periodically. _manager: The DownloadManager which started this download _reply: The QNetworkReply associated with this download. + _autoclose: Whether to close the associated file when the download is + done. """ _MAX_REDIRECTS = 10 @@ -77,10 +79,10 @@ class DownloadItem(downloads.AbstractDownloadItem): reply: The QNetworkReply to download. """ super().__init__(parent=manager) - self.autoclose = True self.fileobj = None self.raw_headers = {} + self._autoclose = True self._manager = manager self._retry_info = None self._reply = None @@ -196,7 +198,7 @@ class DownloadItem(downloads.AbstractDownloadItem): raise ValueError("fileobj was already set! Old: {}, new: " "{}".format(self.fileobj, fileobj)) self.fileobj = fileobj - self.autoclose = autoclose + self._autoclose = autoclose try: self._read_timer.stop() log.downloads.debug("buffer: {} bytes".format(self._buffer.tell())) @@ -223,7 +225,7 @@ class DownloadItem(downloads.AbstractDownloadItem): log.downloads.debug("Finishing download...") if self._reply.isOpen(): self.fileobj.write(self._reply.readAll()) - if self.autoclose: + if self._autoclose: self.fileobj.close() self.successful = self._reply.error() == QNetworkReply.NoError self._reply.close() From 3ba7f28069776214e18537b410a4322223fb2675 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 20:36:49 +0100 Subject: [PATCH 33/44] Re-add download cleanup on shutdown --- qutebrowser/app.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/qutebrowser/app.py b/qutebrowser/app.py index 11d7267f9..c6b369503 100644 --- a/qutebrowser/app.py +++ b/qutebrowser/app.py @@ -45,7 +45,8 @@ import qutebrowser.resources from qutebrowser.completion.models import instances as completionmodels from qutebrowser.commands import cmdutils, runners, cmdexc from qutebrowser.config import style, config, websettings, configexc -from qutebrowser.browser import urlmarks, adblock, history, browsertab +from qutebrowser.browser import (urlmarks, adblock, history, browsertab, + downloads) from qutebrowser.browser.webkit import cookies, cache from qutebrowser.browser.webkit.network import networkmanager from qutebrowser.mainwindow import mainwindow, prompt @@ -702,6 +703,7 @@ class Quitter: atexit.register(shutil.rmtree, self._args.basedir, ignore_errors=True) # Delete temp download dir + downloads.temp_download_manager.cleanup() # If we don't kill our custom handler here we might get segfaults log.destroy.debug("Deactivating message handler...") qInstallMessageHandler(None) From baeb8653c83d73ec7a237019e86b78c803a4a464 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 20:41:40 +0100 Subject: [PATCH 34/44] Finish stats correctly with QtWebEngine downloads --- qutebrowser/browser/webengine/webenginedownloads.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index d43916f9d..5b4e7f498 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -60,15 +60,18 @@ class DownloadItem(downloads.AbstractDownloadItem): self.successful = True self.done = True self.finished.emit() + self.stats.finish() elif state == QWebEngineDownloadItem.DownloadCancelled: self.successful = False self.done = True self.cancelled.emit() + self.stats.finish() elif state == QWebEngineDownloadItem.DownloadInterrupted: self.successful = False self.done = True # https://bugreports.qt.io/browse/QTBUG-56839 self.error.emit("Download failed") + self.stats.finish() else: raise ValueError("_on_state_changed was called with unknown state " "{}".format(state_name)) From df9bee33f43bfbcba2fefa33c593732d805e3557 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 21:56:54 +0100 Subject: [PATCH 35/44] Fix 100% coverage in misc.ipc --- qutebrowser/misc/ipc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/misc/ipc.py b/qutebrowser/misc/ipc.py index 4afae78e9..6641a9ee8 100644 --- a/qutebrowser/misc/ipc.py +++ b/qutebrowser/misc/ipc.py @@ -362,7 +362,7 @@ class IPCServer(QObject): @pyqtSlot() def on_timeout(self): """Cancel the current connection if it was idle for too long.""" - if self._socket is None: + if self._socket is None: # pragma: no cover log.ipc.error("on_timeout got called with None socket!") return log.ipc.error("IPC connection timed out " From 970e4d3e03e27f3c4534ccdcbfc961b9aec65c1f Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 23:23:53 +0100 Subject: [PATCH 36/44] Fix mhtml overwrite prompts See #2101 --- qutebrowser/browser/webkit/mhtml.py | 9 +++++---- tests/end2end/features/downloads.feature | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/qutebrowser/browser/webkit/mhtml.py b/qutebrowser/browser/webkit/mhtml.py index 1024f5238..e89e85ef6 100644 --- a/qutebrowser/browser/webkit/mhtml.py +++ b/qutebrowser/browser/webkit/mhtml.py @@ -19,6 +19,7 @@ """Utils for writing an MHTML file.""" +import html import functools import io import os @@ -537,10 +538,10 @@ def start_download_checked(dest, tab): q = usertypes.Question() q.mode = usertypes.PromptMode.yesno - q.text = "{} exists. Overwrite?".format(path) + q.title = "Overwrite existing file?" + q.text = "{} already exists. Overwrite?".format( + html.escape(path)) q.completed.connect(q.deleteLater) q.answered_yes.connect(functools.partial( _start_download, path, tab=tab)) - message_bridge = objreg.get('message-bridge', scope='window', - window=tab.win_id) - message_bridge.ask(q, blocking=False) + message.global_bridge.ask(q, blocking=False) diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index aaf87dcf3..f187a7336 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -186,6 +186,22 @@ Feature: Downloading things from a website. And I wait for "File successfully written." in the log Then no crash should happen + @qtwebengine_todo: :download --mhtml is not implemented yet + Scenario: Overwriting existing mhtml file + When I set storage -> prompt-download-directory to true + And I open html + And I run :download --mhtml + And I wait for "Asking question text='Please enter a location for http://localhost:*/html' title='Save file to:'>, *" in the log + And I run :prompt-accept + And I wait for "File successfully written." in the log + And I run :download --mhtml + And I wait for "Asking question text='Please enter a location for http://localhost:*/html' title='Save file to:'>, *" in the log + And I run :prompt-accept + And I wait for "Asking question text='* already exists. Overwrite?' title='Overwrite existing file?'>, *" in the log + And I run :prompt-accept yes + And I wait for "File successfully written." in the log + Then no crash should happen + ## :download-cancel Scenario: Cancelling a download From 8771759f68ece05c0ed1fb553459a90b015ecb3e Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 9 Nov 2016 07:54:02 +0100 Subject: [PATCH 37/44] Improve error handling in objreg.dump_objects --- qutebrowser/utils/objreg.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/qutebrowser/utils/objreg.py b/qutebrowser/utils/objreg.py index 06a29c85e..d05424fc3 100644 --- a/qutebrowser/utils/objreg.py +++ b/qutebrowser/utils/objreg.py @@ -23,7 +23,6 @@ import collections import functools -import sip from PyQt5.QtCore import QObject, QTimer from qutebrowser.utils import log @@ -144,8 +143,12 @@ class ObjectRegistry(collections.UserDict): """Dump all objects as a list of strings.""" lines = [] for name, obj in self.data.items(): - if not (isinstance(obj, QObject) and sip.isdeleted(obj)): - lines.append("{}: {}".format(name, repr(obj))) + try: + obj_repr = repr(obj) + except (RuntimeError, TypeError): + # Underlying object deleted probably + obj_repr = '' + lines.append("{}: {}".format(name, obj_repr)) return lines From 80562fbdcaf31ca30084294865059ccfaee14938 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 9 Nov 2016 08:06:57 +0100 Subject: [PATCH 38/44] Add DownloadTarget.suggested_filename --- qutebrowser/browser/qtnetworkdownloads.py | 9 +++------ qutebrowser/utils/usertypes.py | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index b9aaeea8c..5161b3309 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -438,12 +438,9 @@ class DownloadManager(downloads.AbstractDownloadManager): The created DownloadItem. """ if not suggested_filename: - if isinstance(target, usertypes.FileDownloadTarget): - suggested_filename = os.path.basename(target.filename) - elif (isinstance(target, usertypes.FileObjDownloadTarget) and - getattr(target.fileobj, 'name', None)): - suggested_filename = target.fileobj.name - else: + try: + suggested_filename = target.suggested_filename() + except usertypes.NoFilenameError: _, suggested_filename = http.parse_content_disposition(reply) log.downloads.debug("fetch: {} -> {}".format(reply.url(), suggested_filename)) diff --git a/qutebrowser/utils/usertypes.py b/qutebrowser/utils/usertypes.py index 4e67a2eac..052db5720 100644 --- a/qutebrowser/utils/usertypes.py +++ b/qutebrowser/utils/usertypes.py @@ -23,6 +23,7 @@ Module attributes: _UNSET: Used as default argument in the constructor so default can be None. """ +import os.path import operator import collections.abc import enum as pyenum @@ -268,6 +269,11 @@ JsWorld = enum('JsWorld', ['main', 'application', 'user', 'jseval']) MessageLevel = enum('MessageLevel', ['error', 'warning', 'info']) +class NoFilenameError(Exception): + + """Raised when we can't find out a filename in DownloadTarget.""" + + # Where a download should be saved class DownloadTarget: @@ -276,6 +282,10 @@ class DownloadTarget: def __init__(self): raise NotImplementedError + def suggested_filename(self): + """Get the suggested filename for this download target.""" + raise NotImplementedError + class FileDownloadTarget(DownloadTarget): @@ -289,6 +299,9 @@ class FileDownloadTarget(DownloadTarget): # pylint: disable=super-init-not-called self.filename = filename + def suggested_filename(self): + return os.path.basename(self.filename) + class FileObjDownloadTarget(DownloadTarget): @@ -302,6 +315,12 @@ class FileObjDownloadTarget(DownloadTarget): # pylint: disable=super-init-not-called self.fileobj = fileobj + def suggested_filename(self): + try: + return self.fileobj.name + except AttributeError: + raise NoFilenameError + class OpenFileDownloadTarget(DownloadTarget): @@ -317,6 +336,9 @@ class OpenFileDownloadTarget(DownloadTarget): # pylint: disable=super-init-not-called self.cmdline = cmdline + def suggested_filename(self): + raise NoFilenameError + class Question(QObject): From cf32aac111c76c9233ff5b606efb1c47fa6f9d28 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 9 Nov 2016 08:15:31 +0100 Subject: [PATCH 39/44] Move usertypes.DownloadTarget to downloads module --- qutebrowser/browser/adblock.py | 3 +- qutebrowser/browser/commands.py | 4 +- qutebrowser/browser/downloads.py | 79 ++++++++++++++++++- qutebrowser/browser/qtnetworkdownloads.py | 6 +- .../browser/webengine/webenginedownloads.py | 2 +- qutebrowser/browser/webkit/mhtml.py | 2 +- qutebrowser/mainwindow/prompt.py | 5 +- qutebrowser/utils/usertypes.py | 71 ----------------- tests/unit/browser/webkit/test_downloads.py | 33 ++++++++ .../utils/usertypes/test_downloadtarget.py | 59 -------------- 10 files changed, 120 insertions(+), 144 deletions(-) delete mode 100644 tests/unit/utils/usertypes/test_downloadtarget.py diff --git a/qutebrowser/browser/adblock.py b/qutebrowser/browser/adblock.py index 9a65231bb..6197847d9 100644 --- a/qutebrowser/browser/adblock.py +++ b/qutebrowser/browser/adblock.py @@ -26,6 +26,7 @@ import posixpath import zipfile import fnmatch +from qutebrowser.browser import downloads from qutebrowser.config import config from qutebrowser.utils import objreg, standarddir, log, message, usertypes from qutebrowser.commands import cmdutils, cmdexc @@ -208,7 +209,7 @@ class HostBlocker: else: fobj = io.BytesIO() fobj.name = 'adblock: ' + url.host() - target = usertypes.FileObjDownloadTarget(fobj) + target = downloads.FileObjDownloadTarget(fobj) download = download_manager.get(url, target=target, auto_remove=True) self._in_progress.append(download) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 042e75a51..b3f9d67c8 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1328,7 +1328,7 @@ class CommandDispatcher: if dest is None: target = None else: - target = usertypes.FileDownloadTarget(dest) + target = downloads.FileDownloadTarget(dest) download_manager.get(url, target=target) elif mhtml_: self._download_mhtml(dest) @@ -1336,7 +1336,7 @@ class CommandDispatcher: if dest is None: target = None else: - target = usertypes.FileDownloadTarget(dest) + target = downloads.FileDownloadTarget(dest) download_manager.get(self._current_url(), target=target) def _download_mhtml(self, dest=None): diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index be4d3285b..12e766ecb 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -164,6 +164,77 @@ def get_filename_question(*, suggested_filename, url, parent=None): return q +class NoFilenameError(Exception): + + """Raised when we can't find out a filename in DownloadTarget.""" + + +# Where a download should be saved +class _DownloadTarget: + + """Abstract base class for different download targets.""" + + def __init__(self): + raise NotImplementedError + + def suggested_filename(self): + """Get the suggested filename for this download target.""" + raise NotImplementedError + + +class FileDownloadTarget(_DownloadTarget): + + """Save the download to the given file. + + Attributes: + filename: Filename where the download should be saved. + """ + + def __init__(self, filename): + # pylint: disable=super-init-not-called + self.filename = filename + + def suggested_filename(self): + return os.path.basename(self.filename) + + +class FileObjDownloadTarget(_DownloadTarget): + + """Save the download to the given file-like object. + + Attributes: + fileobj: File-like object where the download should be written to. + """ + + def __init__(self, fileobj): + # pylint: disable=super-init-not-called + self.fileobj = fileobj + + def suggested_filename(self): + try: + return self.fileobj.name + except AttributeError: + raise NoFilenameError + + +class OpenFileDownloadTarget(_DownloadTarget): + + """Save the download in a temp dir and directly open it. + + Attributes: + cmdline: The command to use as string. A `{}` is expanded to the + filename. None means to use the system's default application. + If no `{}` is found, the filename is appended to the cmdline. + """ + + def __init__(self, cmdline=None): + # pylint: disable=super-init-not-called + self.cmdline = cmdline + + def suggested_filename(self): + raise NoFilenameError + + class DownloadItemStats(QObject): """Statistics (bytes done, total bytes, time, etc.) about a download. @@ -565,13 +636,13 @@ class AbstractDownloadItem(QObject): """Set the target for a given download. Args: - target: The usertypes.DownloadTarget for this download. + target: The DownloadTarget for this download. """ - if isinstance(target, usertypes.FileObjDownloadTarget): + if isinstance(target, FileObjDownloadTarget): self._set_fileobj(target.fileobj, autoclose=False) - elif isinstance(target, usertypes.FileDownloadTarget): + elif isinstance(target, FileDownloadTarget): self._set_filename(target.filename) - elif isinstance(target, usertypes.OpenFileDownloadTarget): + elif isinstance(target, OpenFileDownloadTarget): try: fobj = temp_download_manager.get_tmpfile(self.basename) except OSError as exc: diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index 5161b3309..c72e60303 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -372,7 +372,7 @@ class DownloadManager(downloads.AbstractDownloadManager): Args: request: The QNetworkRequest to download. - target: Where to save the download as usertypes.DownloadTarget. + target: Where to save the download as downloads.DownloadTarget. **kwargs: Passed to _fetch_request. Return: @@ -430,7 +430,7 @@ class DownloadManager(downloads.AbstractDownloadManager): Args: reply: The QNetworkReply to download. - target: Where to save the download as usertypes.DownloadTarget. + target: Where to save the download as downloads.DownloadTarget. auto_remove: Whether to remove the download even if ui -> remove-finished-downloads is set to -1. @@ -456,7 +456,7 @@ class DownloadManager(downloads.AbstractDownloadManager): filename = downloads.immediate_download_path(prompt_download_directory) if filename is not None: # User doesn't want to be asked, so just use the download_dir - target = usertypes.FileDownloadTarget(filename) + target = downloads.FileDownloadTarget(filename) download.set_target(target) return download diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index 5b4e7f498..5286a6298 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -143,7 +143,7 @@ class DownloadManager(downloads.AbstractDownloadManager): filename = downloads.immediate_download_path() if filename is not None: # User doesn't want to be asked, so just use the download_dir - target = usertypes.FileDownloadTarget(filename) + target = downloads.FileDownloadTarget(filename) download.set_target(target) return diff --git a/qutebrowser/browser/webkit/mhtml.py b/qutebrowser/browser/webkit/mhtml.py index e89e85ef6..237898809 100644 --- a/qutebrowser/browser/webkit/mhtml.py +++ b/qutebrowser/browser/webkit/mhtml.py @@ -345,7 +345,7 @@ class _Downloader: download_manager = objreg.get('qtnetwork-download-manager', scope='window', window=self._win_id) - target = usertypes.FileObjDownloadTarget(_NoCloseBytesIO()) + target = downloads.FileObjDownloadTarget(_NoCloseBytesIO()) item = download_manager.get(url, target=target, auto_remove=True) self.pending_downloads.add((url, item)) diff --git a/qutebrowser/mainwindow/prompt.py b/qutebrowser/mainwindow/prompt.py index 7435a46a7..f5ed10009 100644 --- a/qutebrowser/mainwindow/prompt.py +++ b/qutebrowser/mainwindow/prompt.py @@ -29,6 +29,7 @@ from PyQt5.QtCore import (pyqtSlot, pyqtSignal, Qt, QTimer, QDir, QModelIndex, from PyQt5.QtWidgets import (QWidget, QGridLayout, QVBoxLayout, QLineEdit, QLabel, QFileSystemModel, QTreeView, QSizePolicy) +from qutebrowser.browser import downloads from qutebrowser.config import style from qutebrowser.utils import usertypes, log, utils, qtutils, objreg, message from qutebrowser.keyinput import modeman @@ -687,11 +688,11 @@ class DownloadFilenamePrompt(FilenamePrompt): def accept(self, value=None): text = value if value is not None else self._lineedit.text() - self.question.answer = usertypes.FileDownloadTarget(text) + self.question.answer = downloads.FileDownloadTarget(text) return True def download_open(self, cmdline): - self.question.answer = usertypes.OpenFileDownloadTarget(cmdline) + self.question.answer = downloads.OpenFileDownloadTarget(cmdline) self.question.done() message.global_bridge.prompt_done.emit(self.KEY_MODE) diff --git a/qutebrowser/utils/usertypes.py b/qutebrowser/utils/usertypes.py index 052db5720..8cac08fda 100644 --- a/qutebrowser/utils/usertypes.py +++ b/qutebrowser/utils/usertypes.py @@ -269,77 +269,6 @@ JsWorld = enum('JsWorld', ['main', 'application', 'user', 'jseval']) MessageLevel = enum('MessageLevel', ['error', 'warning', 'info']) -class NoFilenameError(Exception): - - """Raised when we can't find out a filename in DownloadTarget.""" - - -# Where a download should be saved -class DownloadTarget: - - """Abstract base class for different download targets.""" - - def __init__(self): - raise NotImplementedError - - def suggested_filename(self): - """Get the suggested filename for this download target.""" - raise NotImplementedError - - -class FileDownloadTarget(DownloadTarget): - - """Save the download to the given file. - - Attributes: - filename: Filename where the download should be saved. - """ - - def __init__(self, filename): - # pylint: disable=super-init-not-called - self.filename = filename - - def suggested_filename(self): - return os.path.basename(self.filename) - - -class FileObjDownloadTarget(DownloadTarget): - - """Save the download to the given file-like object. - - Attributes: - fileobj: File-like object where the download should be written to. - """ - - def __init__(self, fileobj): - # pylint: disable=super-init-not-called - self.fileobj = fileobj - - def suggested_filename(self): - try: - return self.fileobj.name - except AttributeError: - raise NoFilenameError - - -class OpenFileDownloadTarget(DownloadTarget): - - """Save the download in a temp dir and directly open it. - - Attributes: - cmdline: The command to use as string. A `{}` is expanded to the - filename. None means to use the system's default application. - If no `{}` is found, the filename is appended to the cmdline. - """ - - def __init__(self, cmdline=None): - # pylint: disable=super-init-not-called - self.cmdline = cmdline - - def suggested_filename(self): - raise NoFilenameError - - class Question(QObject): """A question asked to the user, e.g. via the status bar. diff --git a/tests/unit/browser/webkit/test_downloads.py b/tests/unit/browser/webkit/test_downloads.py index 498375700..acee916b2 100644 --- a/tests/unit/browser/webkit/test_downloads.py +++ b/tests/unit/browser/webkit/test_downloads.py @@ -17,6 +17,7 @@ # You should have received a copy of the GNU General Public License # along with qutebrowser. If not, see . +import pytest from qutebrowser.browser import downloads, qtnetworkdownloads @@ -27,3 +28,35 @@ def test_download_model(qapp, qtmodeltester, config_stub, cookiejar_and_cache): manager = qtnetworkdownloads.DownloadManager(win_id=0) model = downloads.DownloadModel(manager) qtmodeltester.check(model) + + +class TestDownloadTarget: + + def test_base(self): + with pytest.raises(NotImplementedError): + downloads._DownloadTarget() + + def test_filename(self): + target = downloads.FileDownloadTarget("/foo/bar") + assert target.filename == "/foo/bar" + + def test_fileobj(self): + fobj = object() + target = downloads.FileObjDownloadTarget(fobj) + assert target.fileobj is fobj + + def test_openfile(self): + target = downloads.OpenFileDownloadTarget() + assert target.cmdline is None + + def test_openfile_custom_command(self): + target = downloads.OpenFileDownloadTarget('echo') + assert target.cmdline == 'echo' + + @pytest.mark.parametrize('obj', [ + downloads.FileDownloadTarget('foobar'), + downloads.FileObjDownloadTarget(None), + downloads.OpenFileDownloadTarget(), + ]) + def test_class_hierarchy(self, obj): + assert isinstance(obj, downloads._DownloadTarget) diff --git a/tests/unit/utils/usertypes/test_downloadtarget.py b/tests/unit/utils/usertypes/test_downloadtarget.py deleted file mode 100644 index 4da2e7a81..000000000 --- a/tests/unit/utils/usertypes/test_downloadtarget.py +++ /dev/null @@ -1,59 +0,0 @@ -# vim: ft=python fileencoding=utf-8 sts=4 sw=4 et: - -# Copyright 2016 Daniel Schadt -# -# This file is part of qutebrowser. -# -# qutebrowser is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# qutebrowser is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with qutebrowser. If not, see . - -"""Tests for the DownloadTarget class.""" - -from qutebrowser.utils import usertypes - -import pytest - - -def test_base(): - with pytest.raises(NotImplementedError): - usertypes.DownloadTarget() - - -def test_filename(): - target = usertypes.FileDownloadTarget("/foo/bar") - assert target.filename == "/foo/bar" - - -def test_fileobj(): - fobj = object() - target = usertypes.FileObjDownloadTarget(fobj) - assert target.fileobj is fobj - - -def test_openfile(): - target = usertypes.OpenFileDownloadTarget() - assert target.cmdline is None - - -def test_openfile_custom_command(): - target = usertypes.OpenFileDownloadTarget('echo') - assert target.cmdline == 'echo' - - -@pytest.mark.parametrize('obj', [ - usertypes.FileDownloadTarget('foobar'), - usertypes.FileObjDownloadTarget(None), - usertypes.OpenFileDownloadTarget(), -]) -def test_class_hierarchy(obj): - assert isinstance(obj, usertypes.DownloadTarget) From 4afd75a24de59ebe1d2bcfe2ee817a7a4d0105bf Mon Sep 17 00:00:00 2001 From: Daniel Karbach Date: Wed, 9 Nov 2016 09:30:37 +0100 Subject: [PATCH 40/44] typo in comment --- qutebrowser/mainwindow/tabbedbrowser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/mainwindow/tabbedbrowser.py b/qutebrowser/mainwindow/tabbedbrowser.py index 3bcadc333..e6bd21ff4 100644 --- a/qutebrowser/mainwindow/tabbedbrowser.py +++ b/qutebrowser/mainwindow/tabbedbrowser.py @@ -415,7 +415,7 @@ class TabbedBrowser(tabwidget.TabWidget): # On first sight, we'd think we have to decrement # self._tab_insert_idx_left here, as we want the next tab to be # *before* the one we just opened. However, since we opened a tab - # *before* of the currently focused tab, indices will shift by + # *before* the currently focused tab, indices will shift by # 1 automatically. elif pos == 'next': idx = self._tab_insert_idx_right From b481dd668d0459856d94b9b5792e25d1cd7dbe36 Mon Sep 17 00:00:00 2001 From: Daniel Karbach Date: Wed, 9 Nov 2016 09:38:47 +0100 Subject: [PATCH 41/44] test config migration for tab-{close,only} flags --- tests/unit/config/test_config.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/unit/config/test_config.py b/tests/unit/config/test_config.py index 37b5c9676..4f76dbb13 100644 --- a/tests/unit/config/test_config.py +++ b/tests/unit/config/test_config.py @@ -348,6 +348,16 @@ class TestKeyConfigParser: ('prompt-yes', 'prompt-accept yes'), ('prompt-no', 'prompt-accept no'), + + ('tab-close -l', 'tab-close --prev'), + ('tab-close --left', 'tab-close --prev'), + ('tab-close -r', 'tab-close --next'), + ('tab-close --right', 'tab-close --next'), + + ('tab-only -l', 'tab-only --prev'), + ('tab-only --left', 'tab-only --prev'), + ('tab-only -r', 'tab-only --next'), + ('tab-only --right', 'tab-only --next'), ] ) def test_migrations(self, old, new_expected): From 0f05ff6536dc0b937db28e6e047f0c14cccadc63 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 9 Nov 2016 12:36:07 +0100 Subject: [PATCH 42/44] Update docs --- CHANGELOG.asciidoc | 2 ++ README.asciidoc | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 8a796a2b2..256edb489 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -140,6 +140,8 @@ Changed - `qute:version` and `qutebrowser --version` now show various important paths - `:spawn`/userscripts now show a nicer error when a script wasn't found - Various functionality now works when javascript is disabled with QtWebKit +- Various commands/settings taking `left`/`right`/`previous` arguments now take + `prev`/`next`/`last-used` to remove ambiguity. Deprecated ~~~~~~~~~~ diff --git a/README.asciidoc b/README.asciidoc index b5ea0fb6d..22b21dd83 100644 --- a/README.asciidoc +++ b/README.asciidoc @@ -154,8 +154,8 @@ Contributors, sorted by the number of commits in descending order: * Bruno Oliveira * Alexander Cogneau * Felix Van der Jeugt -* Martin Tournoij * Daniel Karbach +* Martin Tournoij * Kevin Velghe * Raphael Pierzina * Joel Torstensson From 1a5e90f6528c5d40f976e37ef0f18b6cd1c2efe6 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 9 Nov 2016 14:29:06 +0100 Subject: [PATCH 43/44] Fix lint --- qutebrowser/browser/adblock.py | 2 +- qutebrowser/browser/qtnetworkdownloads.py | 3 +-- qutebrowser/utils/usertypes.py | 1 - 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/qutebrowser/browser/adblock.py b/qutebrowser/browser/adblock.py index 6197847d9..84d630ab6 100644 --- a/qutebrowser/browser/adblock.py +++ b/qutebrowser/browser/adblock.py @@ -28,7 +28,7 @@ import fnmatch from qutebrowser.browser import downloads from qutebrowser.config import config -from qutebrowser.utils import objreg, standarddir, log, message, usertypes +from qutebrowser.utils import objreg, standarddir, log, message from qutebrowser.commands import cmdutils, cmdexc diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index c72e60303..b75fee821 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -20,7 +20,6 @@ """Download manager.""" import io -import os.path import shutil import functools import collections @@ -440,7 +439,7 @@ class DownloadManager(downloads.AbstractDownloadManager): if not suggested_filename: try: suggested_filename = target.suggested_filename() - except usertypes.NoFilenameError: + except downloads.NoFilenameError: _, suggested_filename = http.parse_content_disposition(reply) log.downloads.debug("fetch: {} -> {}".format(reply.url(), suggested_filename)) diff --git a/qutebrowser/utils/usertypes.py b/qutebrowser/utils/usertypes.py index 8cac08fda..89480d241 100644 --- a/qutebrowser/utils/usertypes.py +++ b/qutebrowser/utils/usertypes.py @@ -23,7 +23,6 @@ Module attributes: _UNSET: Used as default argument in the constructor so default can be None. """ -import os.path import operator import collections.abc import enum as pyenum From c36edfb2ba28c658c1346ca0e627cf67a2f99a5b Mon Sep 17 00:00:00 2001 From: Kevin Velghe Date: Wed, 9 Nov 2016 14:36:20 +0100 Subject: [PATCH 44/44] Wait for text being inserted before testing --- tests/end2end/features/yankpaste.feature | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/end2end/features/yankpaste.feature b/tests/end2end/features/yankpaste.feature index b568f23fe..9a4bf53a8 100644 --- a/tests/end2end/features/yankpaste.feature +++ b/tests/end2end/features/yankpaste.feature @@ -263,6 +263,7 @@ Feature: Yanking and pasting. And I run :click-element id qute-textarea And I wait for "Clicked editable element!" in the log And I run :insert-text Hello world + And I wait for "Inserting text into element *" in the log And I run :jseval console.log("textarea contents: " + document.getElementById('qute-textarea').value); # Enable javascript again for the other tests And I set content -> allow-javascript to true