Cloudef / bemenu

Dynamic menu library and client program inspired by dmenu
GNU General Public License v3.0
1.22k stars 94 forks source link

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

Closed merrittlj closed 1 year ago

merrittlj commented 1 year ago

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 commented 1 year ago

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

merrittlj commented 1 year ago

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 commented 1 year ago

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

merrittlj commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

merrittlj commented 1 year ago

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

Cloudef commented 1 year ago

Thanks, merged!