Skip to content
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.

Sign up for GitHub

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

Implement backwards navigation by pressing N #809

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement backwards navigation by pressing N #809

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions buku
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4681,14 +4681,24 @@ def prompt(obj, results, noninteractive=False, deep=False, listtags=False, sugge

skip_print = False
while True:
if (new_results or nav == 'n') and not skip_print:
count = next_index

if (new_results or nav in ['n', 'N']) and not skip_print:
if results:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I suggest flipping the condition within this and the next loop (so that the branches will be swapped).

The else: branches in both of them take up merely 2 lines, and are barely noticeable after the much larger "then" branches; placing them first would make the logic of this entire block much clearer.

total_results = len(results)
cur_index = next_index
Copy link
Collaborator

@LeXofLeviafan LeXofLeviafan Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be moved to after the print loop; it'd make the code within this block much less confusing.

This would also remove the need of resetting next_index when replacing results (…so you should replace all instances of cur_index = next_index = 0 with just cur_index = 0, to simplify the code).

(Nevermind that, seems like there's logic elsewhere that depends on this particular order of actions… Though this line can still be moved out of the if.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…You can also move the total_results line above it – that'd allow to simplify the ifs here (potentially combining them into a single chain if you flip the topmost if at least).

If you want to avoid causing errors when None is passed, you can simply add a fallback value: len(results or []).

if cur_index < total_results:
next_index = min(cur_index + num, total_results)
if new_results and cur_index < total_results:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add new_results and to this condition? I can't think of any case where that would be useful 🤔

If we've reached the end of the list, cur_index will be >= total_results. And if we haven't, new_results won't be set to False.

If you had enabled navigation after reaching the end, it'd be different; but then, this additional condition would undo the effect of enabling it in the first place…

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…Incidentally, if you want to enable navigation after reaching the end, you'd need an or here instead, since cur_index < total_results would prevent evaluation otherwise (e.g. or nav == 'N').

if nav == 'N':
if cur_index <= num:
cur_index = count = 0
next_index = num
elif cur_index % num != 0:
next_index = cur_index - (cur_index % num)
count = cur_index = next_index - num
else:
cur_index = count = cur_index - num * 2
next_index -= num
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How… complicated.

How about you make a variable named prev_index and just set it to max(0, cur_index - num) before the print loop? (And instead of this whole conditional chain, you can simply set cur_index to min(cur_index, prev_index)… so that you don't need to reset prev_index when replacing results).

As for count and next_index, their values are actually not dependent on whether we're moving forwards or backwards (see below).

elif cur_index < total_results:
count = next_index
next_index = min(cur_index + num, total_results)
Copy link
Collaborator

@LeXofLeviafan LeXofLeviafan Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

count is the iteration index; it should always be set to cur_index before the loop (in the previous version of the code, that was done implicitly by the iteration). Doing so shouldn't be placed within an if.

This particular assignment of next_index is also universal (regardless of your cur_index, this value simply denotes position of the end of the page being printed). Placing it withing an if makes no sense either.

print()
for row in results[cur_index:next_index]:
count += 1
Expand All @@ -4714,7 +4724,7 @@ def prompt(obj, results, noninteractive=False, deep=False, listtags=False, sugge
return

# show the next set of results from previous search
if nav == 'n':
if nav == 'n' or nav =='N':
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint check fails on CI due to this line:

./buku.py:4727:32: E225 missing whitespace around operator

Please check.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use in here as well? It's literally the same condition.

continue

if (m := re.match(r'^R(?: (-)?([0-9]+))?$', nav.rstrip())) and (n := int(m[2] or 1)) > 0:
Expand Down