New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] add statusline widget for back/forward indicator #2738

Merged
merged 1 commit into from Jul 9, 2017

Conversation

Projects
None yet
3 participants
@blueyed
Contributor

blueyed commented Jun 18, 2017

Fixes #2737.

TODO:

  • tests

This change is Reviewable

@blueyed blueyed changed the title from [WIP] add statusline widget for back/forward indicator to [RFC] add statusline widget for back/forward indicator Jun 18, 2017

"""Shows navigation indicator (if you can go backward and/or forward)."""
def on_tab_changed(self, tab):
self.tab = tab

This comment has been minimized.

@The-Compiler

The-Compiler Jun 19, 2017

Collaborator

I'm not entirely happy with saving the tab here, but it's a bit of a tricky situation - what about connecting the signal above like tabs.cur_url_changed.connect(functools.partial(status.backforward.on_url_changed, tabs=tabs)), then taking a tabs in on_url_changed, and use tabs.currentWidget() to get the current tab? I haven't tested it, but I think it should work and it seems less error-prone to me.

This comment has been minimized.

@blueyed

blueyed Jun 19, 2017

Contributor

Seems to work well.

@@ -0,0 +1,42 @@
# vim: ft=python fileencoding=utf-8 sts=4 sw=4 et:
# Copyright 2015-2017 Florian Bruhin (The Compiler) <mail@qutebrowser.org>

This comment has been minimized.

@The-Compiler

The-Compiler Jun 19, 2017

Collaborator

nitpick: 2017 instead of 2015-2017

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jun 19, 2017

I've also added the @pyqtSlot() decorator, not sure if it is necessary? Maybe useful just for documentation / introspection alone?!

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jun 19, 2017

Any pointer for the tests?
A new file in  tests/unit/mainwindow/statusbar/?
How can I mock / provide tabs there?

"""Shows navigation indicator (if you can go backward and/or forward)."""
@pyqtSlot()

This comment has been minimized.

@The-Compiler

The-Compiler Jun 20, 2017

Collaborator

@pyqtSlot() on methods connected to PyQt signals is usually used because it has a memory/performance benefit (and also works around some bugs in older PyQt versions).

Here you're connecting a functools.partial object however, and never connect to the method directly - so I'd rather remove it.

@The-Compiler

This comment has been minimized.

Collaborator

The-Compiler commented Jun 20, 2017

Yeah, a new file there sounds good. When it has 100% coverage, please also add it to scripts/dev/check_coverage.py (like the other statusbar files there).

As for mocking tabs: There's a TabbedBrowserStub in stubs.py, which you can access via a stubs fixture.

In 994e8c6 I just did some changes related to that as I discovered there were various other TabbedBrowser stubs, so I merged them into one. If you're comfortable with git rebase, I suggest you rebase on the newest master first.

@The-Compiler

This comment has been minimized.

Collaborator

The-Compiler commented Jun 20, 2017

Just as a heads-up: I'm scheduled to have a (planned) eye surgery, so I probably won't respond to this anymore until somewhen next week.

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jun 20, 2017

@The-Compiler
Thanks for notifying me, and good luck with that!
Take care!

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jun 30, 2017

What do you think about having some additional indicator on how far you can go forward/backwards, e.g. the number if it is > 1, and < 10, and using << / >> in case it is > 10?

@The-Compiler

This comment has been minimized.

Collaborator

The-Compiler commented Jun 30, 2017

Dunno. It doesn't sound too useful I think, and it's not very intuitive what it'd mean. Either way, I don't think you can get this information easily with the API we get from QtWebKit/QtWebEngine.

@The-Compiler

This comment has been minimized.

Collaborator

The-Compiler commented Jul 5, 2017

Any update, @blueyed?

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jul 5, 2017

No, I am using this myself, but otherwise I have to find the time to finish this, i.e. write the tests.

@The-Compiler

This comment has been minimized.

Collaborator

The-Compiler commented Jul 5, 2017

I felt like writing some tests, so here they are:

From b6cd620a8635aad1cec325e9ef3cbfd15bec1766 Mon Sep 17 00:00:00 2001
From: Florian Bruhin <git@the-compiler.org>
Date: Wed, 5 Jul 2017 15:45:23 +0200
Subject: [PATCH 4/4] Add statusbar.backforward tests

---
 scripts/dev/check_coverage.py                      |  2 +
 tests/helpers/stubs.py                             | 20 ++++++++-
 .../unit/mainwindow/statusbar/test_backforward.py  | 50 ++++++++++++++++++++++
 3 files changed, 71 insertions(+), 1 deletion(-)
 create mode 100644 tests/unit/mainwindow/statusbar/test_backforward.py

diff --git a/scripts/dev/check_coverage.py b/scripts/dev/check_coverage.py
index 7e5a2d47a..b03fb7ef4 100644
--- a/scripts/dev/check_coverage.py
+++ b/scripts/dev/check_coverage.py
@@ -117,6 +117,8 @@ PERFECT_FILES = [
         'mainwindow/statusbar/textbase.py'),
     ('tests/unit/mainwindow/statusbar/test_url.py',
         'mainwindow/statusbar/url.py'),
+    ('tests/unit/mainwindow/statusbar/test_backforward.py',
+        'mainwindow/statusbar/backforward.py'),
     ('tests/unit/mainwindow/test_messageview.py',
         'mainwindow/messageview.py'),
 
diff --git a/tests/helpers/stubs.py b/tests/helpers/stubs.py
index 0551f5a86..788d3ace3 100644
--- a/tests/helpers/stubs.py
+++ b/tests/helpers/stubs.py
@@ -222,6 +222,22 @@ class FakeWebTabScroller(browsertab.AbstractScroller):
         return self._pos_perc
 
 
+class FakeWebTabHistory(browsertab.AbstractHistory):
+
+    def __init__(self, tab, *, can_go_back, can_go_forward):
+        super().__init__(tab)
+        self._can_go_back = can_go_back
+        self._can_go_forward = can_go_forward
+
+    def can_go_back(self):
+        assert self._can_go_back is not None
+        return self._can_go_back
+
+    def can_go_forward(self):
+        assert self._can_go_forward is not None
+        return self._can_go_forward
+
+
 class FakeWebTab(browsertab.AbstractTab):
 
     """Fake AbstractTab to use in tests."""
@@ -229,12 +245,14 @@ class FakeWebTab(browsertab.AbstractTab):
     def __init__(self, url=FakeUrl(), title='', tab_id=0, *,
                  scroll_pos_perc=(0, 0),
                  load_status=usertypes.LoadStatus.success,
-                 progress=0):
+                 progress=0, can_go_back=None, can_go_forward=None):
         super().__init__(win_id=0, mode_manager=None, private=False)
         self._load_status = load_status
         self._title = title
         self._url = url
         self._progress = progress
+        self.history = FakeWebTabHistory(self, can_go_back=can_go_back,
+                                         can_go_forward=can_go_forward)
         self.scroller = FakeWebTabScroller(self, scroll_pos_perc)
         wrapped = QWidget()
         self._layout.wrap(self, wrapped)
diff --git a/tests/unit/mainwindow/statusbar/test_backforward.py b/tests/unit/mainwindow/statusbar/test_backforward.py
new file mode 100644
index 000000000..15de7442f
--- /dev/null
+++ b/tests/unit/mainwindow/statusbar/test_backforward.py
@@ -0,0 +1,50 @@
+# vim: ft=python fileencoding=utf-8 sts=4 sw=4 et:
+
+# Copyright 2017 Florian Bruhin (The Compiler) <mail@qutebrowser.org>
+#
+# 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 <http://www.gnu.org/licenses/>.
+
+
+"""Test Progress widget."""
+
+import pytest
+
+from qutebrowser.mainwindow.statusbar import backforward
+
+
+@pytest.fixture
+def backforward_widget(qtbot):
+    widget = backforward.Backforward()
+    qtbot.add_widget(widget)
+    return widget
+
+
+@pytest.mark.parametrize('can_go_back, can_go_forward, expected_text', [
+    (False, False, ''),
+    (True, False, '<'),
+    (False, True, '>'),
+    (True, True, '<>'),
+])
+def test_backforward_widget(backforward_widget, stubs,
+                            fake_web_tab, can_go_back, can_go_forward,
+                            expected_text):
+    """Ensure the Backforward widget shows the correct text."""
+    tab = fake_web_tab(can_go_back=can_go_back, can_go_forward=can_go_forward)
+    tabbed_browser = stubs.TabbedBrowserStub()
+    tabbed_browser.current_index = 1
+    tabbed_browser.tabs = [tab]
+    backforward_widget.on_url_changed(tabbed_browser)
+    assert backforward_widget.text() == expected_text
-- 
2.13.2

I'll also add some more line comments.

"""Shows navigation indicator (if you can go backward and/or forward)."""
def on_url_changed(self, tabs):

This comment has been minimized.

@The-Compiler

The-Compiler Jul 5, 2017

Collaborator

It's a bit confusing still calling this on_url_changed I think - maybe a simple update_text or so would be better.

This comment has been minimized.

@blueyed

blueyed Jul 9, 2017

Contributor

It is still called through tabs.cur_url_changed?!

This comment has been minimized.

@The-Compiler

The-Compiler Jul 9, 2017

Collaborator

Yes, but if something is called on_url_changed, I'd expect it to be directly connected to a url_changed signal and get a QUrl as argument.

This comment has been minimized.

@blueyed

blueyed Jul 9, 2017

Contributor

on_tab_cur_url_changed then maybe?
I think it is better to have some explicit name, instead of update_text?!

This comment has been minimized.

@The-Compiler

The-Compiler Jul 9, 2017

Collaborator

Works for me 😉

def on_url_changed(self, tabs):
"""Called on URL changes."""
tab = tabs.currentWidget()

This comment has been minimized.

@The-Compiler

The-Compiler Jul 5, 2017

Collaborator

On Travis looks like this was invoked while tabs.currentWidget() was None and caused it to raise. I'm not 100% sure why it happened, but I think it'd be a good idea to add a:

if tab is None:
    return
text += '<'
if tab.history.can_go_forward():
text += '>'
self.setText(text)

This comment has been minimized.

@The-Compiler

The-Compiler Jul 5, 2017

Collaborator

Maybe add [] around the text so it fits in with the other statusbar widgets?

This comment has been minimized.

@blueyed

blueyed Jul 9, 2017

Contributor

It takes away some more space then, and IIRC the current behavior is what you see in Vimperator, too - but I will change it.

@@ -469,6 +469,7 @@ def _connect_signals(self):
tabs.tab_index_changed.connect(status.tabindex.on_tab_index_changed)
tabs.cur_url_changed.connect(status.url.set_url)
tabs.cur_url_changed.connect(functools.partial(status.backforward.on_url_changed, tabs=tabs))

This comment has been minimized.

@The-Compiler

The-Compiler Jul 5, 2017

Collaborator

Please break this after the partial( or so.

"""Navigation (back/forward) indicator displayed in the statusbar."""
from PyQt5.QtCore import pyqtSlot

This comment has been minimized.

@The-Compiler

The-Compiler Jul 5, 2017

Collaborator

unused import

This comment has been minimized.

@blueyed

blueyed Jul 9, 2017

Contributor

Why is F401 (this error) in .flake8? (since the beginning in 91e6f4c)

This comment has been minimized.

@The-Compiler

The-Compiler Jul 9, 2017

Collaborator

Because it's already checked by pylint and I don't want to do ignores twice everywhere for imports which aren't actually unused.

This comment has been minimized.

@blueyed

blueyed Jul 9, 2017

Contributor

Because pylint takes care of that?

This comment has been minimized.

@The-Compiler

The-Compiler Jul 9, 2017

Collaborator

I don't understand what you're asking? Yes, pylint shows an unused import error, see my link above.

This comment has been minimized.

@blueyed

blueyed Jul 9, 2017

Contributor

Yes, pylint does, but I was using only flake8 in the editor (which I will change for the project).
I think I answered my own question: F401 from flake8 is ignored, since pylint is supposed to lint this part.

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jul 9, 2017

@The-Compiler
Pushed some updates.
I assume we'll squash this altogether in the end, so I've just added your tests, without your authorship?!

Remaining: the renaming of the hook.

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jul 9, 2017

Why does Travis not run the PR?

@The-Compiler

This comment has been minimized.

Collaborator

The-Compiler commented Jul 9, 2017

Good question... let me try to close and reopen it.

@The-Compiler The-Compiler reopened this Jul 9, 2017

@The-Compiler

I'm fine with squashing things, yeah.

@@ -222,19 +222,37 @@ def pos_perc(self):
return self._pos_perc
class FakeWebTabHistory(browsertab.AbstractHistory):

This comment has been minimized.

@The-Compiler

The-Compiler Jul 9, 2017

Collaborator

My bad, but this needs a docstring (something like """Fake for Web{Kit,Engine}History.""" or so)

def on_url_changed(self, tabs):
"""Called on URL changes."""
tab = tabs.currentWidget()
if tab is None:

This comment has been minimized.

@The-Compiler

The-Compiler Jul 9, 2017

Collaborator

Hmm, probably needs a # pragma: no cover as it doesn't always happen.

@blueyed blueyed closed this Jul 9, 2017

@blueyed blueyed reopened this Jul 9, 2017

@The-Compiler The-Compiler merged commit b3a9e09 into qutebrowser:master Jul 9, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@The-Compiler

This comment has been minimized.

Collaborator

The-Compiler commented Jul 9, 2017

Thanks!

@blueyed

This comment has been minimized.

Contributor

blueyed commented Jul 9, 2017

You're welcome!
Thank you for helping me get this in!

@blueyed blueyed deleted the blueyed:backforward branch Jul 9, 2017

@The-Compiler

This comment has been minimized.

Collaborator

The-Compiler commented Jul 10, 2017

Pushed two small changes:

  • 5fb6cb7 because the gap when there's no history started to annoy me
  • e81dccc to add a test for the if tab is None: case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment