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.

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

Add Match/Page Counter(Addresses #204) #325

Merged
merged 8 commits into from
Dec 27, 2022
Merged

Conversation

merrittlj
Copy link
Contributor

@merrittlj merrittlj commented Dec 23, 2022

Add a match/page counter to the right side of the bar. I could add an option to disable this, but it shouldn't be required due to how minimal it is. Example: If there are 12 items showing on the bar and a total of 220 items in the system, the bar would show "[12/220]"

I forgot to sync my fork with the main branch before adding the counter commits, so for some reason, there are merge conflicts with the main branch, that I couldn't figure out how to fix. Fixed this, I'm not very smart. Ready to merge.

Closes #204

@Cloudef
Copy link
Owner

Cloudef commented Dec 24, 2022

Hello, can you rebase your changes as there seems to be conflicts

lib/menu.c Outdated Show resolved Hide resolved
@merrittlj
Copy link
Contributor Author

merrittlj commented Dec 24, 2022

The Ubuntu test doesn't appreciate that I used a char* in a const char* function, and from how long flatpak has taken, I assume the same too. I'll look into fixing the warnings, but besides that it's ready to merge.

Edit: Flatpak is fine, just took awhile.

@merrittlj
Copy link
Contributor Author

All checks passed, all unnecessary code removed, re-based, and ready to merge.

lib/menu.c Outdated Show resolved Hide resolved
@merrittlj
Copy link
Contributor Author

merrittlj commented Dec 24, 2022

Fixed the inconsistent state, if there are no other reviews then it should be ready. Apologies for the many problems my code brought up.

@Cloudef
Copy link
Owner

Cloudef commented Dec 24, 2022

Thanks, one more final thing. Can you expose the counter through CLI switch. You can look --scrollbar for example. I think some people may not want it default.

@merrittlj
Copy link
Contributor Author

Ah, ok, I mentioned something similar on the original issue. By that do you mean enable it through CLI, or disable it there(does the vast majority of people want it enabled or disabled, or would one or the other be more intuitive)?

@Cloudef
Copy link
Owner

Cloudef commented Dec 24, 2022

Based on experience before, it's better to have it disabled by default. Usually there always appears a new issue when something is changed by default.

@merrittlj
Copy link
Contributor Author

On a side note, as I mentioned in the issue, do you think the counter should read "[matched/total]"(current), "matched/total", or something similar?

@merrittlj
Copy link
Contributor Author

For the counter option, I would most likely have to add a variable in the API to control it? Does that sound right or am I over-complicating things?

@Cloudef
Copy link
Owner

Cloudef commented Dec 24, 2022

Yes, you can check the code for scrollbar option as for how it is done. There's API for setting scrollbar display setting, and the CLI flag handling code calls that. As for the display formatting, I leave that for you to decide.

@merrittlj
Copy link
Contributor Author

merrittlj commented Dec 24, 2022

While doing some testing, the counter seems to disappear when the -l option is specified. I'll look into this, but as I modified the code for the ">" in the right of the screen it is most likely a if statement or something that it should be in.

I'm also committing the changes for the option in the CLI, but let me know if I should replace the bool with a enum in the code(hard to explain, but hopefully you see what I mean when committed).

@merrittlj
Copy link
Contributor Author

I beg your pardon for all of this trouble, I didn't expect it to last this long.

@merrittlj
Copy link
Contributor Author

Fixed the -l issue, everything seems to be functional currently.

@Cloudef Cloudef merged commit 77000a6 into Cloudef:master Dec 27, 2022
@Cloudef
Copy link
Owner

Cloudef commented Dec 27, 2022

Thanks, merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Match counter and page counter
2 participants