From fad9c902c932c62e5c95fe117bffec22899f9474 Mon Sep 17 00:00:00 2001 From: user202729 <25191436+user202729@users.noreply.github.com> Date: Fri, 12 Oct 2018 22:28:07 +0700 Subject: [PATCH 1/5] Fix _hint_scattered --- qutebrowser/browser/hints.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py index b93155255..cfa435b3e 100644 --- a/qutebrowser/browser/hints.py +++ b/qutebrowser/browser/hints.py @@ -454,21 +454,13 @@ class HintManager(QObject): # Determine how many digits the link hints will require in the worst # case. Usually we do not need all of these digits for every link # single hint, so we can show shorter hints for a few of the links. - needed = max(min_chars, math.ceil(math.log(len(elems), len(chars)))) + needed = max(min_chars, math.ceil( + math.log(len(elems), len(chars))-1e-13)) # Short hints are the number of hints we can possibly show which are # (needed - 1) digits in length. - if needed > min_chars: + if needed > min_chars and needed > 1: total_space = len(chars) ** needed - # Calculate short_count naively, by finding the avaiable space and - # dividing by the number of spots we would loose by adding a - # short element - short_count = math.floor((total_space - len(elems)) / - len(chars)) - # Check if we double counted above to warrant another short_count - # https://github.com/qutebrowser/qutebrowser/issues/3242 - if total_space - (short_count * len(chars) + - (len(elems) - short_count)) >= len(chars) - 1: - short_count += 1 + short_count = (total_space - len(elems)) // (len(chars) - 1) else: short_count = 0 From c2f027bf2be18936568f4e9173cf10f5b54156a4 Mon Sep 17 00:00:00 2001 From: Jay Kamat Date: Fri, 12 Oct 2018 19:37:59 -0700 Subject: [PATCH 2/5] Add tests for rounding error --- tests/unit/browser/test_hints.py | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/tests/unit/browser/test_hints.py b/tests/unit/browser/test_hints.py index 69be5c420..4663aefd6 100644 --- a/tests/unit/browser/test_hints.py +++ b/tests/unit/browser/test_hints.py @@ -19,6 +19,7 @@ import string import functools +import itertools import operator import pytest @@ -27,8 +28,8 @@ import qutebrowser.browser.hints @pytest.mark.parametrize('min_len', [0, 3]) -@pytest.mark.parametrize('num_chars', [9]) -@pytest.mark.parametrize('num_elements', range(1, 26)) +@pytest.mark.parametrize('num_chars', [5, 9]) +@pytest.mark.parametrize('num_elements', itertools.chain(range(1, 26), [125])) def test_scattered_hints_count(win_registry, mode_manager, min_len, num_chars, num_elements): """Test scattered hints function. @@ -62,7 +63,9 @@ def test_scattered_hints_count(win_registry, mode_manager, min_len, # 'ab' and 'c' can assert abs(functools.reduce(operator.sub, hint_lens)) <= 1 - longest_hints = [x for x in hints if len(x) == max(hint_lens)] + longest_hint_len = max(hint_lens) + shortest_hint_len = min(hint_lens) + longest_hints = [x for x in hints if len(x) == longest_hint_len] if min_len < max(hint_lens) - 1: # Check if we have any unique prefixes. For example, 'la' @@ -72,3 +75,17 @@ def test_scattered_hints_count(win_registry, mode_manager, min_len, prefix = x[:-1] count_map[prefix] = count_map.get(prefix, 0) + 1 assert all(e != 1 for e in count_map.values()) + + # Check that the longest hint length isn't too long + if longest_hint_len > min_len and longest_hint_len > 1: + assert num_chars ** (longest_hint_len - 1) < num_elements + + # Check longest hint is not too short + assert num_chars ** longest_hint_len >= num_elements + + if longest_hint_len > min_len and longest_hint_len > 1: + # Check that the longest hint length isn't too long + assert num_chars ** (longest_hint_len - 1) < num_elements + if shortest_hint_len == longest_hint_len: + # Check that we really couldn't use any short links + assert (num_chars ** longest_hint_len) - num_elements < len(chars) - 1 From 622ef9294c62ab61c150343425e930c96018c1bc Mon Sep 17 00:00:00 2001 From: user202729 <25191436+user202729@users.noreply.github.com> Date: Sat, 13 Oct 2018 13:49:08 +0700 Subject: [PATCH 3/5] Use integer arithmetic to compute ceil log --- qutebrowser/browser/hints.py | 6 ++---- qutebrowser/utils/utils.py | 13 +++++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py index cfa435b3e..c4b5f51af 100644 --- a/qutebrowser/browser/hints.py +++ b/qutebrowser/browser/hints.py @@ -21,7 +21,6 @@ import collections import functools -import math import os import re import html @@ -454,8 +453,7 @@ class HintManager(QObject): # Determine how many digits the link hints will require in the worst # case. Usually we do not need all of these digits for every link # single hint, so we can show shorter hints for a few of the links. - needed = max(min_chars, math.ceil( - math.log(len(elems), len(chars))-1e-13)) + needed = max(min_chars, utils.ceil_log(len(elems), len(chars))) # Short hints are the number of hints we can possibly show which are # (needed - 1) digits in length. if needed > min_chars and needed > 1: @@ -487,7 +485,7 @@ class HintManager(QObject): elems: The elements to generate labels for. """ strings = [] - needed = max(min_chars, math.ceil(math.log(len(elems), len(chars)))) + needed = max(min_chars, utils.ceil_log(len(elems), len(chars))) for i in range(len(elems)): strings.append(self._number_to_hint_str(i, chars, needed)) return strings diff --git a/qutebrowser/utils/utils.py b/qutebrowser/utils/utils.py index f2e19a743..a6c8e866c 100644 --- a/qutebrowser/utils/utils.py +++ b/qutebrowser/utils/utils.py @@ -712,3 +712,16 @@ def guess_mimetype(filename, fallback=False): else: raise ValueError("Got None mimetype for {}".format(filename)) return mimetype + + +def ceil_log(number, base): + """Compute max(1, ceil(log(number, base))). + + Use only integer arithmetic in order to avoid numerical error. + """ + result = 1 + accum = base + while accum < number: + result += 1 + accum *= base + return result From 97bc4fa6a2f3d05b25f22268236b5fc627eae7e5 Mon Sep 17 00:00:00 2001 From: user202729 <25191436+user202729@users.noreply.github.com> Date: Sat, 13 Oct 2018 15:13:26 +0700 Subject: [PATCH 4/5] Add comment for explanation --- qutebrowser/browser/hints.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/qutebrowser/browser/hints.py b/qutebrowser/browser/hints.py index c4b5f51af..cfd6f7c4e 100644 --- a/qutebrowser/browser/hints.py +++ b/qutebrowser/browser/hints.py @@ -454,10 +454,13 @@ class HintManager(QObject): # case. Usually we do not need all of these digits for every link # single hint, so we can show shorter hints for a few of the links. needed = max(min_chars, utils.ceil_log(len(elems), len(chars))) + # Short hints are the number of hints we can possibly show which are # (needed - 1) digits in length. if needed > min_chars and needed > 1: total_space = len(chars) ** needed + # For each 1 short link being added, len(chars) long links are + # removed, therefore the space removed is len(chars) - 1. short_count = (total_space - len(elems)) // (len(chars) - 1) else: short_count = 0 From 79f63b9e819ca2cd3696274d9b654ed21e3c9fd3 Mon Sep 17 00:00:00 2001 From: Jay Kamat Date: Sat, 13 Oct 2018 08:55:55 -0700 Subject: [PATCH 5/5] Fix line length warning in hint scatter test --- tests/unit/browser/test_hints.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/browser/test_hints.py b/tests/unit/browser/test_hints.py index 4663aefd6..c89d11e92 100644 --- a/tests/unit/browser/test_hints.py +++ b/tests/unit/browser/test_hints.py @@ -88,4 +88,5 @@ def test_scattered_hints_count(win_registry, mode_manager, min_len, assert num_chars ** (longest_hint_len - 1) < num_elements if shortest_hint_len == longest_hint_len: # Check that we really couldn't use any short links - assert (num_chars ** longest_hint_len) - num_elements < len(chars) - 1 + assert ((num_chars ** longest_hint_len) - num_elements < + len(chars) - 1)