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
Implements subpagination for ddgr #22
Conversation
I think we are on the same page. When we press I will review this in 1/2 days. |
The indices do not start from 0. We have to hide (and store) the true first real index of the current page (as offset)) and start from 1 in each page. When users enters the visible index to open a link in browser, we have to add that to current offset. If we don't do that, we will see this:
|
Why don't we update the index when we add it to our cache? |
We try to fetch whenever we hit the last page. As we are caching the results, can we store the index (initialized to a negative value) to indicate which is our last index the first time we hit it? That way we can save the redundant fetch every time user reaches the last page. |
Self note: document the option in README, manpage, add to completion-scripts. (owner @jarun) |
It would be a good idea to default num to 10. 30/40 results per page needs scrolling up/down every time. |
Let n=0 disable num as a fallback. |
…value; calculate true index when opening a url
I've made the following updates
There is a significant item remaining to be fixed: if pagination values are large, they can cause the index to increase to a value larger than the size of the results, and this causes problems with the output. |
Self-notes (owner @jarun):
|
@@ -1139,18 +1140,25 @@ class DdgCmd(object): | |||
parser = DdgParser() | |||
parser.feed(page.text) | |||
|
|||
self.results = parser.results | |||
if self.options.pagination: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this getting set???
@@ -1267,8 +1290,15 @@ class DdgCmd(object): | |||
@no_argument | |||
def do_next(self): | |||
# If no results were fetched last time, we have hit the last page already | |||
if self._ddg_url._qrycnt == 0: | |||
if self._ddg_url._qrycnt == 0 and self.index >= len(self.results): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this additional condition do???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will keep having these issues in deciphering stuff if we never get a single line of code comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You add the check self.options.pagination
in every other place and then fuse this kind of a condition without any check???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this line is the issue with the huge pagination value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have hidden it by limiting n to 25. If anyone is trying to set n greater than that he is trying to automate stuff. He should patch the code for himself. ;)
if self.options.pagination: | ||
self.results.extend(parser.results) | ||
offset = len(self._urltable) | ||
for r in self.results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are re-indexing results from offset??? Why???
I'll probably have to do a massive rework on this. Anyway... |
print('No results.', file=sys.stderr) | ||
else: | ||
sys.stderr.write(prelude) | ||
for r in self.results: | ||
r.print() | ||
for i, r in enumerate(results): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken for n=0
I think we are reasonably good now! 👍 |
print(json.dumps(results_object, indent=2, sort_keys=True, ensure_ascii=False)) | ||
else: | ||
# Regular output | ||
if not self.results: | ||
if not self.results or self.index > len(self.results): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the second check be self.index >= len(self.results)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get rid of the check completely.
Here's a provisional implementation of subpagination for ddgr.
Pagination is set by using the
-n
(or--num
) option which takes an integer N to paginate the results by. For example:ddgr python -n 5
will paginate by 5 results at a time.If the number of fetched results left to display is less than the pagination value, new results from the next Ddg results page are fetched and appended to the list of results already returned. Use of the
p
command in the omniprompt will show the previous N results, where N is the pagination value.If the
f
command is used in the omni prompt, the index is reset to 0 so that the first set of results are displayed.Please let me know if this is in line with what you are thinking/what changes should be made.